Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Provide fallback JPEG images in frontend when WebP is not supported by the browser #360
Provide fallback JPEG images in frontend when WebP is not supported by the browser #360
Changes from 13 commits
36cd431
11197de
8262fca
e06409b
6637709
5cb1283
7044a6d
45a5fde
0d91c53
6ffde35
829390a
9b280a6
5365f64
d97c430
4fbb95e
49fca5e
e44a8a2
4db66ba
74fe886
f205815
ea38eae
a1fb857
a9bf111
6871972
9429f1b
696ade3
7e70d49
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Will the file name always match?
The thought behind the question: Someone uploads the image
hamilton-the-musical.jpg
, this will generate fallback imagehamilton-the-musical.webp
. Later the imagehamilton-the-musical.jpeg
is uploaded (note thee
), this will generate images of a different name for the fallback -- probablyhamilton-the-musical-2.webp
.🔢 The same question applies for
srcset
.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.
This is actually a good point but I think it is out of scope for this issue. The problem is bigger that just properly replacing webp files here. I tried to replicate your edge case and it looks like in this case webp images for the second image will override webp images for the first image 🤔.
I uploaded
20180607_144302.jpg
image first, webp images have been created correctly. Then I uploaded the same image but with another extension20180607_144302.jpeg
and in this time webp images overriden webp images generated for the first image. Here is what I got on the server:This doesn't look right although this is a very unlikely edge case. I think it is worth creating a separate issue for it 🤔.
cc @felixarntz @adamsilverstein
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.
This was previously discussed and should have been fixed in the plugin - see #358 - not sure why you still see this happening. I'm not sure this fix is great though, it skips the duplicate generation; instead behavior should be like or use
wp_unique_filename
.For core I am working on including this in the core patch as well, I tried one approach in 93542af (#2393) - that isn't quite finished - note there can actually be three variations - cat.jpe, cat.jpeg, and cat.jpg - yes ".jpe" is also valid!
Another option would be to incorporate the fix into
WP_Image_Editor::generate_filename
orsave
- again the tricky part is making sure overwriting works when you want it to (ie. regenerate images).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.
Thanks for testing this. I'm happy if y'all decide it is out of scope for this ticket. It seems like a much bigger problem than I thought when asking the question :)
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.
The problem of avoiding the override was indeed fixed in #358 - however that shouldn't have affected the PR here anyway, since realistically the problem is only on the JPEG (jpeg/jpg/jpe) side. Now that #358 is in place, no conflicting WebP image would be created so we should be good for now.
With that said, I agree this is a valid point, and I think a much more stable implementation for the replacement would be to replace the full file name for the
image/webp
version per size with its corresponding full file name for theimage/jpeg
version. Let's open a follow-up issue to add that.