-
Notifications
You must be signed in to change notification settings - Fork 98
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove the extension "-jpg" from the generated file name of the full sized webp version #1037
base: trunk
Are you sure you want to change the base?
Remove the extension "-jpg" from the generated file name of the full sized webp version #1037
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @reteinformatica, @kenny-nt, @ddur, @jupitercow, @jamesozzie. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Co-authored-by: Mukesh Panchal <[email protected]>
@rilwis Thank you for the PR. However, I'm not sure we should apply this change. The reason the extension is added is to prevent conflicts. This was a conscious decision which actually came in response to bugs reported. For example, if a user uploads I realize this is a very specific scenario, but it is a bug that actually happened and was reported. So I'm not sure we should allow removing the original file extension suffix just for better looking file names - unless there is another reason that the suffix is a problem. |
@felixarntz Thanks for your comment. This PR doesn't change the default behavior that you described. When people making the change via the filter, they have to do that with a clear intention. So it is the user's problem when they upload images with the same file name like More details are discussed in the issue #545 and I think @mukeshpanchal27 can provide more info, because he suggested the filter. |
@rilwis Having a filter for that seems okay, but I don't find it a great solution. Filters can only be set by developers, but then users need to live with the consequences. If you're the developer and administrator of a site, that makes sense. But many sites are not like that, the people that use the site and the people that develop the site are different. I think a better solution would be to handle it automatically, in a smart way that only applies the suffix when needed:
This way, it just works, no developer intervention required, and satisfies both issues:
|
@felixarntz After checking the source code of WordPress, I found that WordPress already has a mechanism to prevent duplicated file names: This function checks only the file name without extension and append numbers if neccessary. So when a user uploads I've updated the code below. Please take a look. |
Thanks, @rilwis, for working on the PR. I appreciate the way you implemented the changes. However, as @felixarntz mentioned, we should go with the prefix to avoid conflicts. Since the PR generates image names like WP did, I'm happy to proceed with it, but we need to get Felix's feedback on it. Once we agree, we'll need to address the unit tests as some of them are failing now. Thanks! cc @adamsilverstein |
@mukeshpanchal27 Tests are updated. There's also a test for uploading |
I'm aware of the code that does this, but as far as I remember, that function doesn't consider But maybe I'm missing something. If so, can you point me to where that function detects those files to be the same file name even though their file extensions are different? When you tested this, did you test it in both configurations? See Settings > Media, it needs to work both when only generating WebP as well as when generating JPEG and WebP. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felixarntz: You're right. I looked at the wrong place. The code I refered above was about checking the image with suffix "-scaled" and rename if it already exists.
I tested again with the "Generate JPEG files in addition to WebP" settings enabled and saw if image.jpeg
is uploaded after image.jpg
, then they generate the same image.webp
.
If the settings "Generate JPEG files in addition to WebP" is disabled (unchecked), then WordPress auto appends -1
to the webp versions.
I've updated the PR with a check for file_exists
to rename the file if needed. Please take a look.
@rilwis Hi! Could you update the PR description to reflect the latest state of this PR? That will help us get new reviewers so we can get this merged for the next release. |
@westonruter Done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running unit tests for this locally, I'm getting a single failure:
There was 1 failure:
1) WebP_Uploads_Helper_Tests::it_should_add_original_image_extension_to_the_webp_file_name_to_ensure_it_is_unique with data set #0 ('/var/www/html/wp-content/plug...e.jpeg', '/var/www/html/wp-content/plug...ge.jpg')
Failed asserting that 'image-35-300x300.webp' ends with "-1-300x300.webp".
/var/www/html/wp-content/plugins/performance/tests/plugins/webp-uploads/helper-tests.php:576
This is confusing because there is no error in the GHA-run tests.
@adamsilverstein @thelovekesh This seems somehow related to WebP and/or env issues that I seem to recall you mentioning?
function webp_uploads_generate_filename( WP_Image_Editor $editor, string $file, string $size, string $extension ): string { | ||
$ext = pathinfo( $file, PATHINFO_EXTENSION ); | ||
if ( $extension === $ext ) { | ||
return $file; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could test coverage be added for this?
// Add "-{$ext}" to the filename if an image with the same name already exists. | ||
$file = "{$dir}{$name}{$suffix}.{$extension}"; | ||
if ( file_exists( $file ) ) { | ||
$file = "{$dir}{$name}{$suffix}-{$ext}.{$extension}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could test coverage be added for this as well?
@westonruter what environment are you using for this? |
@thelovekesh I'm testing locally with wp-env which is supposed to be the same as on GHA. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awaiting changes or refinement
Has this issue been resolved? |
NO. |
I think we should focus on solving this problem because it has existed for a long time. |
please update avif image format also |
@adamsilverstein please update more avif format images |
Summary
In #545, a problem when uploading a
image.jpg
file is that the plugin will create the full sized version of the image with the nameimage-jpg.webp
. This PR changes this behavior by not automatically append-jpg
to the file name. The file name is only appended when there's an existing image with the same name.Fixes #545
Relevant technical choices
Previously, the plugin relies on
$editor->generate_filename
. But this function always consider a suffix and add-{$suffix}
to the file name. There's no way to eliminate this. So I have to create a helper functionwebp_uploads_generate_filename
to handle this.There is 2 things that this function consider:
full
true
Checklist
[Focus]
orInfrastructure
label.[Type]
label.no milestone
label.