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.

Sign up for GitHub

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
Closed

Ensure images with dimensions in name are supported #62

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 50 additions & 24 deletions inc/class-tachyon.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,24 @@ public static function parse_dimensions_from_filename( $src ) {
return array( false, false );
}

/**
* Get an attachment post ID by GUID.
*
* @param string $src The attachment source.
* @return int|false
*/
public static function get_attachment_by_guid( $src ) {
global $wpdb;

// Trim the host name portion of the source as this could potentially have changed.
$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.


return $id ?? false;
}

/**
* Identify images in post content, and if images are local (uploaded to the current site), pass through Tachyon.
*
Expand Down Expand Up @@ -192,29 +210,30 @@ public static function filter_the_content( $content ) {
if ( preg_match( '#height=["|\']?([\d%]+)["|\']?#i', $images['img_tag'][ $index ], $height_string ) )
$height = $height_string[1];

// Detect WP registered image size from HTML class
if ( preg_match( '#class=["|\']?[^"\']*size-([^"\'\s]+)[^"\']*["|\']?#i', $images['img_tag'][ $index ], $matches ) ) {
$size = array_pop( $matches );
}

// If image tag lacks width or height arguments, try to determine from strings WP appends to resized image filenames.
if ( ! $width || ! $height ) {
if ( ! isset( $size ) && ( ! $width || ! $height ) ) {
$size_from_file = static::parse_dimensions_from_filename( $src );
$width = $width ?: $size_from_file[0];
$height = $height ?: $size_from_file[1];
// Check if this isn't a full size URL before inferring dimensions.
if ( $size_from_file[0] !== false && $size_from_file[1] !== false ) {
$attachment_id = self::get_attachment_by_guid( $src );
if ( $attachment_id ) {
$fullsize_url = true;
} else {
$width = $size_from_file[0];
$height = $size_from_file[1];
}
}
}

// Can't pass both a relative width and height, so unset the height in favor of not breaking the horizontal layout.
if ( false !== strpos( $width, '%' ) && false !== strpos( $height, '%' ) )
$width = $height = false;

// Detect WP registered image size from HTML class
if ( preg_match( '#class=["|\']?[^"\']*size-([^"\'\s]+)[^"\']*["|\']?#i', $images['img_tag'][ $index ], $matches ) ) {
$size = array_pop( $matches );

if ( false === $width && false === $height && isset( $size ) && array_key_exists( $size, $image_sizes ) ) {
$size_from_wp = wp_get_attachment_image_src( $attachment_id, $size );
$width = $size_from_wp[1];
$height = $size_from_wp[2];
$transform = $image_sizes[ $size ]['crop'] ? 'resize' : 'fit';
}
}

// WP Attachment ID, if uploaded to this site
if (
preg_match( '#class=["|\']?[^"\']*wp-image-([\d]+)[^"\']*["|\']?#i', $images['img_tag'][ $index ], $class_attachment_id ) &&
Expand Down Expand Up @@ -334,15 +353,6 @@ public static function filter_the_content( $content ) {
$transform = 'fit';
}

// Detect if image source is for a custom-cropped thumbnail and prevent further URL manipulation.
if ( ! $fullsize_url && preg_match_all( '#-e[a-z0-9]+(-\d+x\d+)?\.(' . implode('|', self::$extensions ) . '){1}$#i', basename( $src ), $filename ) )
$fullsize_url = true;

// Build URL, first maybe removing WP's resized string so we pass the original image to Tachyon
if ( ! $fullsize_url ) {
$src = self::strip_image_dimensions_maybe( $src );
}

// Build array of Tachyon args and expose to filter before passing to Tachyon URL function
$args = array();

Expand Down Expand Up @@ -395,6 +405,15 @@ public static function filter_the_content( $content ) {
$size = [ $width, $height ];
}

// Detect if image source is for a custom-cropped thumbnail and prevent further URL manipulation.
if ( ! $fullsize_url && preg_match_all( '#-e[a-z0-9]+(-\d+x\d+)?\.(' . implode('|', self::$extensions ) . '){1}$#i', basename( $src ), $filename ) )
$fullsize_url = true;

// Build URL, first maybe removing WP's resized string so we pass the original image to Tachyon
if ( ! $fullsize_url ) {
$src = self::strip_image_dimensions_maybe( $src );
}

/**
* Filter the array of Tachyon arguments added to an image when it goes through Tachyon.
* By default, only includes width and height values.
Expand Down Expand Up @@ -826,6 +845,13 @@ public static function validate_image_url( $url ) {
* @return string
**/
protected static function strip_image_dimensions_maybe( $src ) {
// Check we can find the attachment by that name
$attachment_id = self::get_attachment_by_guid( $src );
if ( $attachment_id ) {
// Prevent stripping dimenions from src.
return $src;
}

// Build URL, first removing WP's resized string so we pass the original image to Tachyon
if ( preg_match( '#(-\d+x\d+)\.(' . implode( '|', self::$extensions ) . '){1}$#i', $src, $src_parts ) ) {
$src = str_replace( $src_parts[1], '', $src );
Expand Down
2 changes: 1 addition & 1 deletion tests/bootstrap.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
// 2. Plugin installed inside of WordPress.org developer checkout
// 3. Tests checked out to /tmp
if ( false !== getenv( 'WP_DEVELOP_DIR' ) ) {
$test_root = getenv( 'WP_DEVELOP_DIR' ) . '/tests/phpunit';
$test_root = getenv( 'WP_DEVELOP_DIR' );
} elseif ( file_exists( '../../../../tests/phpunit/includes/bootstrap.php' ) ) {
$test_root = '../../../../tests/phpunit';
} elseif ( file_exists( '/tmp/wordpress-tests-lib/includes/bootstrap.php' ) ) {
Expand Down
Binary file added tests/data/tachyon-1132x687.jpg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
73 changes: 73 additions & 0 deletions tests/tests/class-resizing.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,17 @@ class Tests_Resizing extends WP_UnitTestCase {
*
* tachyon.jpg: 1280x719
* tachyon-large.jpg: 5312x2988
* tachyon-1132x687.jpg: 1280x719
* Photo by Digital Buggu from Pexels
* @link https://www.pexels.com/photo/0-7-rpm-171195/
*/
static public function wpSetUpBeforeClass( $factory ) {
global $_wp_additional_image_sizes;
self::$wp_additional_image_sizes = $_wp_additional_image_sizes;

// Ensure pre WP 5.3.1 behaviour with image file having dimensions in file name.
add_filter( 'wp_unique_filename', __NAMESPACE__ . '\\Tests_Resizing::unique_filename_override' );

self::setup_custom_sizes();

self::$attachment_ids['tachyon'] = $factory->attachment->create_upload_object(
Expand All @@ -43,6 +47,10 @@ static public function wpSetUpBeforeClass( $factory ) {
self::$attachment_ids['tachyon-large'] = $factory->attachment->create_upload_object(
realpath( __DIR__ . '/../data/tachyon-large.jpg')
);

self::$attachment_ids['tachyon-1132x687'] = $factory->attachment->create_upload_object(
realpath( __DIR__ . '/../data/tachyon-1132x687.jpg')
);
}

/**
Expand Down Expand Up @@ -72,6 +80,25 @@ public static function wpTearDownAfterClass() {
}
} );
rmdir( $uploads_dir );

remove_filter( 'wp_unique_filename', __NAMESPACE__ . '\\Tests_Resizing::unique_filename_override' );
}

/**
* Prevents WP from fixing the file name during upload.
*
* This occurs if the file name contains dimensions as a suffix.
* This is to help test for backwards compat with WP 5.3.0 and earlier.
*
* @param string $filename
* @return string
*/
public static function unique_filename_override( $filename ) {
if ( strpos( $filename, 'tachyon-1132x687' ) === false ) {
return $filename;
}

return str_replace( '-1.jpg', '.jpg', $filename );
}

function setUp() {
Expand Down Expand Up @@ -411,6 +438,30 @@ function data_filtered_url() {
],
[ 1000, 1000 ],
],
[
'tachyon-1132x687',
'large',
[
'http://tachy.on/u/tachyon-1132x687.jpg?fit=1024,719',
'http://tachy.on/u/tachyon-1132x687.jpg?resize=1024,575',
'http://tachy.on/u/tachyon-1132x687.jpg?fit=1024,1024',
'http://tachy.on/u/tachyon-1132x687.jpg?w=1024&h=575',
'http://tachy.on/u/tachyon-1132x687-1.jpg?fit=1024,719',
'http://tachy.on/u/tachyon-1132x687-1.jpg?resize=1024,575',
'http://tachy.on/u/tachyon-1132x687-1.jpg?fit=1024,1024',
'http://tachy.on/u/tachyon-1132x687-1.jpg?w=1024&h=575',
],
[ 1024, 575 ],
],
[
'tachyon-1132x687',
'full',
[
'http://tachy.on/u/tachyon-1132x687.jpg',
'http://tachy.on/u/tachyon-1132x687-1.jpg',
],
[ 1280, 719 ],
],
];
}

Expand Down Expand Up @@ -720,6 +771,28 @@ function data_content_filtering() {
'http://tachy.on/u/tachyon.jpg?resize=150,150',
],
],
// Unknown attachment ID, unknown size, original file name contains dimensions.
[
'tachyon-1132x687',
'<p><img class="alignnone" src="%%BASE_URL%%/tachyon-1132x687.jpg" alt="" /></p>',
[
'http://tachy.on/u/tachyon-1132x687.jpg',
],
],
[
'tachyon-1132x687',
'<p><img class="alignnone" src="%%BASE_URL%%/tachyon-1132x687-150x150.jpg" alt="" width="150" height="150" /></p>',
[
'http://tachy.on/u/tachyon-1132x687.jpg?resize=150,150',
],
],
[
'tachyon-1132x687',
'<p><img class="alignnone" src="%%BASE_URL%%/tachyon-1132x687-150x150.jpg" alt="" /></p>',
[
'http://tachy.on/u/tachyon-1132x687.jpg?resize=150,150',
],
],
];
}
}