Avoid fetching image dimensions when for internal image. #6448

dhaval-parekh · 2021-07-07T10:18:28Z

Summary

Fixes #5115

Checklist

My code is tested and passes existing tests.
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

includes/utils/class-amp-image-dimension-extractor.php

github-actions · 2021-07-22T10:27:56Z

Plugin builds for bec8f93 are ready 🛎️!

Download development build
Download production build

westonruter · 2021-07-27T23:16:15Z

includes/utils/class-amp-image-dimension-extractor.php

+			// Get image dimension from file name.
+			$basename                   = basename( $url );
+			$filename_without_extension = explode( '.', $basename );
+			$extension                  = array_pop( $filename_without_extension );
+			$filename_without_extension = implode( '.', $filename_without_extension );
+
+			$regex = '/-(?<width>\d+)x(?<height>\d+)(?:\.' . $extension . ')$/m';
+			preg_match( $regex, $basename, $possible_dimension );
+
+			if ( ! empty( $possible_dimension['width'] ) && ! empty( $possible_dimension['height'] ) ) {


I think this can be simplified:

Suggested change

// Get image dimension from file name.

$basename = basename( $url );

$filename_without_extension = explode( '.', $basename );

$extension = array_pop( $filename_without_extension );

$filename_without_extension = implode( '.', $filename_without_extension );

$regex = '/-(?<width>\d+)x(?<height>\d+)(?:\.' . $extension . ')$/m';

preg_match( $regex, $basename, $possible_dimension );

if ( ! empty( $possible_dimension['width'] ) && ! empty( $possible_dimension['height'] ) ) {

// Get image dimension from file name.

$path = wp_parse_url( $url, PHP_URL_PATH );

if ( empty( $path ) ) {

continue;

}

if ( preg_match( '/-(?<width>\d+)x(?<height>\d+)\.\w+$/', $path, $possible_dimension ) ) {

westonruter · 2021-07-27T23:19:50Z

includes/utils/class-amp-image-dimension-extractor.php

+			$url_hash       = md5( $url );
+			$transient_name = sprintf( 'amp_img_%s', $url_hash );


The logic for obtaining the cached dimensions transient is now duplicated in this class. There should be a helper method get_cached_dimensions_transient_name that contains the duplicated logic.

westonruter · 2021-07-27T23:21:20Z

includes/utils/class-amp-image-dimension-extractor.php

+			}
+
+			// Get media path.
+			$attached_path = ltrim( str_replace( $base_upload_uri, '', $url ), '/' );


The $url could have query parameters here. So those should get stripped off.

Also, instead of doing str_replace() I suggest using substr( $url, strlen( $base_upload_uri ) ) since we know we want to strip off a certain number of characters from the beginning as discovered above.

westonruter · 2021-07-27T23:24:39Z

includes/utils/class-amp-image-dimension-extractor.php

+			}
+
+			// Check if it's internal media or not. If it's not then bail out.
+			if ( 0 !== strpos( trim( $url ), $base_upload_uri ) ) {


Suggested change

if ( 0 !== strpos( trim( $url ), $base_upload_uri ) ) {

if ( 0 !== strpos( $url, $base_upload_uri ) ) {

includes/utils/class-amp-image-dimension-extractor.php

westonruter · 2021-07-27T23:30:46Z

includes/utils/class-amp-image-dimension-extractor.php

+				if ( function_exists( 'wp_getimagesize' ) ) {
+					$imagesize = wp_getimagesize( $image_file );
+				} else {
+					$imagesize = getimagesize( $image_file );


Suggested change

$imagesize = getimagesize( $image_file );

$imagesize = @getimagesize( $image_file ); // phpcs:ignore WordPress.PHP.NoSilencedErrors

westonruter · 2021-07-27T23:35:08Z

tests/php/test-amp-image-dimension-extractor.php

@@ -184,4 +184,61 @@ public function test__amp_wp_user_agent() {

 		$this->assertEquals( $expected, $user_agent );
 	}
+
+	/**
+	 * @covers \AMP_Image_Dimension_Extractor::extract_by_filename_or_filesystem


Suggested change

* @covers \AMP_Image_Dimension_Extractor::extract_by_filename_or_filesystem

* @covers \AMP_Image_Dimension_Extractor::extract_by_filename_or_filesystem()

includes/utils/class-amp-image-dimension-extractor.php

westonruter · 2021-07-27T23:42:32Z

tests/php/test-amp-image-dimension-extractor.php

+		// Check after removing attachment metadata, To check fallback to filesystem.
+
+		wp_update_attachment_metadata( $attachment_id, [] );


Suggested change

// Check after removing attachment metadata, To check fallback to filesystem.

wp_update_attachment_metadata( $attachment_id, [] );

// Check after removing attachment metadata, to check fallback to filesystem.

wp_update_attachment_metadata( $attachment_id, [] );

tests/php/test-amp-image-dimension-extractor.php

* Ensure file exists before we get the size info. * Prevent 0 from being a dimension in a file name. * Explain why we check for external object cache.

…image-dimension * 'develop' of github.com:ampproject/amp-wp: Allow the amp-carousel script to be on the page when there is just amp-lightbox-gallery (#6509) Mock HTTP request Supply non-empty body and headers for tests Only unserialize cached response if it is a string Update amp-twitter to use fixed-height layout with a default height matching minimal tweet height Bump eslint from 7.31.0 to 7.32.0 Use OVERFLOW constant Remove unused `use` statements Add overflow buttons to Twitter, Facebook, and Instagram embeds Pin amp-toolbox-php to specific commit in main Update amp-toolbox-php to 98b020d6 Remove unused AmpProject\Dom\Document imports Update references to renamed class constants and remove old deprecated code Update amp-toolbox-php to 0.7.0-alpha (main@a155c37) Update snapshot Use unique IDs for each SVG component instantiation Avoid redirect_extraneous_paired_endpoint from needlessly redirecting Revert "Prevent mutating URL when query var is absent" Prevent mutating URL when query var is absent

…onal array

westonruter

The changes check out to me. Good work.

I want to get a second review from @pierlon as well since this touches on some fundamental aspects of how the plugin has worked for a long time.

westonruter · 2021-08-04T01:22:44Z

Note this will specifically reduce data being stored in options as is a valid complaint about the plugin: https://wordpress.org/support/topic/this-plugin-inserted-a-lot-of-data-in-the-wp_options-table-2/

includes/utils/class-amp-image-dimension-extractor.php

pierlon · 2021-08-04T14:01:23Z

includes/utils/class-amp-image-dimension-extractor.php

+			// Verify that the URL is for an uploaded file.
+			if ( 0 !== strpos( $url_without_query_fragment, $base_upload_uri ) ) {
+				continue;
+			}
+			$upload_relative_path = substr( $url_without_query_fragment, strlen( $base_upload_uri ) );
+
+			// Bail if the URL contains relative paths.
+			if ( 0 !== validate_file( $upload_relative_path ) ) {
+				continue;
+			}


I think these should be the first set of checks before parsing the URL for dimensions.

I'm not sure because the function expects a file path as input, not a URL. So I think we should make sure it looks like a file path, or else this could break if validate_file() changes its behavior in the future which isn't compatible with URLs.

I meant moving both checks. So it would look like:

diff --git a/includes/utils/class-amp-image-dimension-extractor.php b/includes/utils/class-amp-image-dimension-extractor.php --- a/includes/utils/class-amp-image-dimension-extractor.php (revision 35310f1ff829be5a145d14e72af27a28b2cfc0c6) +++ b/includes/utils/class-amp-image-dimension-extractor.php (date 1628092492414) @@ -184,6 +184,17 @@ $url_without_query_fragment = strtok( $url, '?#' ); + // Verify that the URL is for an uploaded file. + if ( 0 !== strpos( $url_without_query_fragment, $base_upload_uri ) ) { + continue; + } + $upload_relative_path = substr( $url_without_query_fragment, strlen( $base_upload_uri ) ); + + // Bail if the URL contains relative paths. + if ( 0 !== validate_file( $upload_relative_path ) ) { + continue; + } + // Parse info out of the URL, including the file extension and (optionally) the dimensions. if ( ! preg_match( '/(?:-(?<width>[1-9][0-9]*)x(?<height>[1-9][0-9]*))?\.(?<ext>\w+)$/', $url_without_query_fragment, $matches ) ) { continue; @@ -202,17 +213,6 @@ ]; continue; } - - // Verify that the URL is for an uploaded file. - if ( 0 !== strpos( $url_without_query_fragment, $base_upload_uri ) ) { - continue; - } - $upload_relative_path = substr( $url_without_query_fragment, strlen( $base_upload_uri ) ); - - // Bail if the URL contains relative paths. - if ( 0 !== validate_file( $upload_relative_path ) ) { - continue; - } // Get image dimension from file system. $image_file = $base_upload_dir . $upload_relative_path;

Ah, I just realized that this needs to be ordered in this way. The reason is that we will use the dimensions in the filename for remote files which have URLs that don't start with $base_upload_uri.

So the order is correct as it stands.

Ah I see, makes sense 👍

includes/utils/class-amp-image-dimension-extractor.php

pierlon · 2021-08-04T15:55:51Z

includes/utils/class-amp-image-dimension-extractor.php

+			// Verify that the URL is for an uploaded file.
+			if ( 0 !== strpos( $url_without_query_fragment, $base_upload_uri ) ) {
+				continue;
+			}
+			$upload_relative_path = substr( $url_without_query_fragment, strlen( $base_upload_uri ) );
+
+			// Bail if the URL contains relative paths.
+			if ( 0 !== validate_file( $upload_relative_path ) ) {
+				continue;
+			}


I meant moving both checks. So it would look like:

diff --git a/includes/utils/class-amp-image-dimension-extractor.php b/includes/utils/class-amp-image-dimension-extractor.php --- a/includes/utils/class-amp-image-dimension-extractor.php (revision 35310f1ff829be5a145d14e72af27a28b2cfc0c6) +++ b/includes/utils/class-amp-image-dimension-extractor.php (date 1628092492414) @@ -184,6 +184,17 @@ $url_without_query_fragment = strtok( $url, '?#' ); + // Verify that the URL is for an uploaded file. + if ( 0 !== strpos( $url_without_query_fragment, $base_upload_uri ) ) { + continue; + } + $upload_relative_path = substr( $url_without_query_fragment, strlen( $base_upload_uri ) ); + + // Bail if the URL contains relative paths. + if ( 0 !== validate_file( $upload_relative_path ) ) { + continue; + } + // Parse info out of the URL, including the file extension and (optionally) the dimensions. if ( ! preg_match( '/(?:-(?<width>[1-9][0-9]*)x(?<height>[1-9][0-9]*))?\.(?<ext>\w+)$/', $url_without_query_fragment, $matches ) ) { continue; @@ -202,17 +213,6 @@ ]; continue; } - - // Verify that the URL is for an uploaded file. - if ( 0 !== strpos( $url_without_query_fragment, $base_upload_uri ) ) { - continue; - } - $upload_relative_path = substr( $url_without_query_fragment, strlen( $base_upload_uri ) ); - - // Bail if the URL contains relative paths. - if ( 0 !== validate_file( $upload_relative_path ) ) { - continue; - } // Get image dimension from file system. $image_file = $base_upload_dir . $upload_relative_path;

Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>

westonruter reviewed Jul 20, 2021

View reviewed changes

includes/utils/class-amp-image-dimension-extractor.php Outdated Show resolved Hide resolved

includes/utils/class-amp-image-dimension-extractor.php Outdated Show resolved Hide resolved

dhaval-parekh self-assigned this Jul 21, 2021

dhaval-parekh force-pushed the add/5115-image-dimension branch from b00cbe7 to f4e3898 Compare July 22, 2021 10:10

dhaval-parekh requested a review from westonruter July 22, 2021 10:23

dhaval-parekh marked this pull request as ready for review July 22, 2021 10:23

westonruter requested changes Jul 27, 2021

View reviewed changes

dhaval-parekh added 9 commits July 29, 2021 12:17

Add image dimension extractor by filename and from file system

371e02b

Add fallback for image dimension if attachment metadata is not available

ae99bf4

Add caching which fetching attachment ID by attached path

ce58f9c

Add unit test cases

4c5e520

Fix unit test cases

cf41aee

Add check for function exist for wp_getimagesize()

a8bf41d

Apply suggestions from PR and add transient cache for image dimention

0bb00d6

Update unit test cases

4474983

Apply suggestion from PR

cdaa764

dhaval-parekh force-pushed the add/5115-image-dimension branch from f4e3898 to cdaa764 Compare July 29, 2021 11:07

update unit test cases

e6af9f2

dhaval-parekh requested a review from westonruter August 3, 2021 14:16

westonruter added 7 commits August 3, 2021 16:00

Harden extract_by_filename_or_filesystem logic

45689e5

* Ensure file exists before we get the size info. * Prevent 0 from being a dimension in a file name. * Explain why we check for external object cache.

Restore wp_using_ext_object_cache() in tearDown(); use dataProvider

b1267a8

Privatize helper method for getting transient names and return positi…

2fffaae

…onal array

Add testing for when dimensions get stored in transients

523be81

Add sanity checking for image filename extensions

38355ea

Add tests for fragment identifiers on URLs

35310f1

westonruter approved these changes Aug 4, 2021

View reviewed changes

westonruter requested a review from pierlon August 4, 2021 00:27

westonruter added this to the v2.2 milestone Aug 4, 2021

pierlon suggested changes Aug 4, 2021

View reviewed changes

pierlon reviewed Aug 4, 2021

View reviewed changes

westonruter and others added 2 commits August 4, 2021 13:44

Use strtolower() and add strict param for in_array()

682d516

Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>

Increase filter priority of extract_by_filename_or_filesystem to 100

bec8f93

pierlon approved these changes Aug 5, 2021

View reviewed changes

westonruter merged commit 851d64b into develop Aug 5, 2021

westonruter deleted the add/5115-image-dimension branch August 5, 2021 02:42

westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Dec 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid fetching image dimensions when for internal image. #6448

Avoid fetching image dimensions when for internal image. #6448

dhaval-parekh commented Jul 7, 2021

github-actions bot commented Jul 22, 2021 •

edited

Loading

westonruter Jul 27, 2021

westonruter Jul 27, 2021

westonruter Jul 27, 2021

westonruter Jul 27, 2021

westonruter Jul 27, 2021

westonruter Jul 27, 2021

westonruter Jul 27, 2021

westonruter Jul 27, 2021

westonruter left a comment

westonruter commented Aug 4, 2021

pierlon Aug 4, 2021

westonruter Aug 4, 2021

pierlon Aug 4, 2021

westonruter Aug 5, 2021

pierlon Aug 5, 2021

pierlon Aug 4, 2021

		$url_hash = md5( $url );
		$transient_name = sprintf( 'amp_img_%s', $url_hash );

	if ( 0 !== strpos( trim( $url ), $base_upload_uri ) ) {
	if ( 0 !== strpos( $url, $base_upload_uri ) ) {

	$imagesize = getimagesize( $image_file );
	$imagesize = @getimagesize( $image_file ); // phpcs:ignore WordPress.PHP.NoSilencedErrors

	* @covers \AMP_Image_Dimension_Extractor::extract_by_filename_or_filesystem
	* @covers \AMP_Image_Dimension_Extractor::extract_by_filename_or_filesystem()

		// Check after removing attachment metadata, To check fallback to filesystem.

		wp_update_attachment_metadata( $attachment_id, [] );

Avoid fetching image dimensions when for internal image. #6448

Avoid fetching image dimensions when for internal image. #6448

Conversation

dhaval-parekh commented Jul 7, 2021

Summary

Checklist

github-actions bot commented Jul 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter left a comment

Choose a reason for hiding this comment

westonruter commented Aug 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 22, 2021 •

edited

Loading