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

Ensure images with dimensions in name are supported #62

Closed
wants to merge 1 commit into from

Conversation

roborourke
Copy link
Contributor

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

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
Copy link

@hm-linter hm-linter bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linting failed (146 errors).

(132 notices occurred in your codebase, but were on files/lines not included in this PR.)

@roborourke
Copy link
Contributor Author

I'm a little hesitant as to this exact solution due to the DB lookups but I can't think of another way to determine if it's a valid original image URL.

$src_path = wp_parse_url( $src, PHP_URL_PATH );

// Try to fetch the post ID by GUID.
$id = $wpdb->get_var( $wpdb->prepare( "SELECT ID FROM {$wpdb->posts} WHERE post_type = 'attachment' AND guid LIKE '%%%s' LIMIT 1;", $src_path ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be a like query? AFAIK $src should be the full URL? can we just match against that?

I think a LIKE on GUID is going to be too poor for performance to take this route.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we can do, I just opted for path matching instead as the URL in the content might not match whatever the original GUID was. Could potentially look for _wp_attached_file in post meta for a direct match but that will probably have a similar issue as it's a much bigger table.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm yeah good thinking / point. I think if we go GUID we should go with URL. I think _wp_attached_file is probably the closest thing we have to "correct" and we'll likely get a better hitrate on this case, but it's going to be a tablescan alteast on the amount of rows that is the number of attachments in the DB.

The alternative would be a file_exists on the original file, though I'm not sure if that's better or worse.

Do we know how common this case is? I'm thinking atleast any non-altis authored content will hit this condition, as g-berg or the Classic Editor will be inserting -300x200.jpg suffixed images, so I'm thinking this condition is actually hit quite a lot.

We could also add object cacing for a url -> ID map in to this function or somewhere else.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah you're right, it'll be hit continually. Seems there's not going to be a consistently performant way of handling this.

Closing in favour of writing a WP CLI migration command instead to update old upload file names.

@roborourke roborourke closed this Nov 2, 2020
@roborourke roborourke deleted the fix-images-with-dims branch November 2, 2020 16:11
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 this pull request may close these issues.

Stripping image dimensions from file name.
2 participants