Skip to content
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

Stripping image dimensions from file name. #43

Open
owaincuvelier opened this issue May 24, 2019 · 6 comments
Open

Stripping image dimensions from file name. #43

owaincuvelier opened this issue May 24, 2019 · 6 comments

Comments

@owaincuvelier
Copy link

owaincuvelier commented May 24, 2019

It seems tachyon is experiencing some odd behaviour when the file name include a dimension. If the origianl file had a dimension portion of the file name, the request 404s, but the stripping itself leaves a px" remnant.

@joehoyle
Copy link
Member

This is happening due to https://github.com/humanmade/tachyon-plugin/blob/master/inc/class-tachyon.php#L770, which will strip 300x600 from the end of a file name, because it "thinks" that is just a part added by WP, and that the original filename is not called that. As it happens, that's a bad assumption when the uploaded file name is actually my-image-300x600.jpg.

This is somewhat mitigated in Photon by a file_exists check in https://github.com/Automattic/jetpack/blob/master/class.photon.php#L1084, which maybe we removed for performance reasons. Note, that this fix doesn't totally work, as it you uploaded my-image-300x600.jpg and my-image.jpg, you'll now get the wrong image.

Maybe if we could somehow use the image ID to work out what the original image is better, then that would work? Just an idea.

@stuartshields
Copy link

Maybe if we could somehow use the image ID to work out what the original image is better, then that would work? Just an idea.

How do you propose doing that @joehoyle I know not all media has wp-image-* attached to it - we saw this on a client recently, it was being caused by Gutenberg, I'm sure we also will find other instances of this happening as well. So we can't always rely on that being available.

I also know that attachment_url_to_postid has performance issues... Maybe we do need that hm_attachment_url_to_postid was suggested last time attachment_url_to_postid was discussed.

Just a few thoughts. Also monitoring this issue to see what the solution will be out of interest :)

@joehoyle
Copy link
Member

I think we'd use wp-image-* where possible, and then maybe fallback to file_exists.

@roborourke
Copy link
Contributor

I think we should remove any file name suffixes like 1200x800 on upload. Obviously doesn't solve this for already uploaded files but the fix right now is to rename and reupload the file anyway, this at least would mitigate the issue for future uploads.

roborourke added a commit that referenced this issue Nov 2, 2020
Tachyon would previsouly assume that an image with a suffix like `-123x456.jpg` was from a thumbnail and attempt to infer the size from that. This update checks whether that suffix matches an image GUID, if it does then its an original file and should not have the dimensions inferred or have the suffix stripped off.

Fixes #43
@roborourke
Copy link
Contributor

As of WP 5.3.1 any files uploaded with a suffix like -123x456.jpg will have a -1 appended per wp_unique_filename(). That solves the issue for newer imports and projects more or less.

I have a proposed solution that tries matching on the attachment GUID to see if the image URL is the original one or not in #62 - it works but there are some performance concerns. In theory the cases where the lookup is used / needed should be somewhat limited.

Ideally we should recommend / provide a migration script to update image files with dimension suffixes.

@roborourke
Copy link
Contributor

We should document the use of this CLI command for fixing the issue:

https://github.com/humanmade/rename-images-command

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants