Bump minimum PHP version requirement to 5.4 and include FasterImage library dynamically #1809

felixarntz · 2019-01-14T20:14:19Z

As pointed out in chat, we should address the unnecessary TLS verification being disabled in the FasterImage library. While doing that, a few other issues arised:

We currently also rely on the FastImage library, solely for the sake of supporting PHP 5.3.
We manually include the FasterImage library with custom tweaks made to it, with the TLS modification becoming just another one.

This PR implements the following changes:

Bump minimum required PHP version to 5.4 and thus ditches the FastImage library.
Pull in FasterImage library in the latest version via Composer, thus also getting rid of the need of bundling third-party code in the repository.
Apply custom modifications via https://github.com/cweagans/composer-patches, making it more obvious what has been changed by us and making it seamless to update the library while keeping our changes in place.
In addition to the previous modifications, TLS verification is enabled in the curl_setopt() calls made by the library.

Considerations:

What is the current process of packaging the plugin for release? We need to ensure the patches folder is ignored in that process.
As a more general observation not directly related to this change, we should ensure when packaging for release the dependencies are installed on the correct environment (PHP 5.4), otherwise we risk installing them in a higher version. Not sure whether this has been considered or tested before. Note that Composer takes the version specification in composer.json as minimum, not as "the dependencies have to be installed in a version compatible with that".

… patch file.

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

westonruter · 2019-01-14T20:30:56Z

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

@@ -172,38 +172,7 @@ private static function determine_which_images_to_fetch( &$dimensions, &$urls_to
 	 * @param string $mode Whether image dimensions should be extracted concurrently or synchronously.
 	 */
 	private static function fetch_images( $urls_to_fetch, &$images, $mode ) {
-		// Use FasterImage when for compatible PHP versions
-		if ( 'synchronous' === $mode ||
-			false === function_exists( 'curl_multi_exec' ) ||


This curl_multi_exec() call gives me pause. Is it possible for this function to not exist in 5.4, say if an older version of cURL is used? We've seen something like this when an older version of ICU is present: #1440

curl_multi_exec() should be available on any PHP5 version as the PHP docs state, at least their is no distinction between different sets of curl_*() functions. In other words, if curl_exec() exists, we can also assume curl_multi_exec() exists. Note that WordPress core does that as well, in their cURL implementation they use curl_multi_exec() too and don't specifically check its existance. I think we should move forward with this.

Apparently this turned out to not be true: #2183 (comment)

westonruter · 2019-01-14T20:32:19Z

The build is just failing due to a PHPCS error:

/includes/class-amp-autoloader.php:59:55: warning - Array double arrow not aligned correctly; expected 10 space(s) between "'AMP_YouTube_Embed_Handler'" and double arrow, but found 19. (WordPress.Arrays.MultipleStatementAlignment.DoubleArrowNotAligned)

westonruter · 2019-01-14T22:32:01Z

The PHPCS fixes are opening a snowballing can of worms 😄

/includes/sanitizers/class-amp-core-theme-sanitizer.php:684:9: error - Opening parenthesis of a multi-line function call must be the last content on the line (PEAR.Functions.FunctionCallSignature.ContentAfterOpenBracket)
/includes/sanitizers/class-amp-core-theme-sanitizer.php:684:43: error - Only one argument is allowed per line in a multi-line function call (PEAR.Functions.FunctionCallSignature.MultipleArguments)
/includes/sanitizers/class-amp-nav-menu-dropdown-sanitizer.php:67:9: error - Opening parenthesis of a multi-line function call must be the last content on the line (PEAR.Functions.FunctionCallSignature.ContentAfterOpenBracket)
/includes/sanitizers/class-amp-nav-menu-dropdown-sanitizer.php:67:49: error - Only one argument is allowed per line in a multi-line function call (PEAR.Functions.FunctionCallSignature.MultipleArguments)
/includes/sanitizers/class-amp-nav-menu-dropdown-sanitizer.php:125:12: error - Only one argument is allowed per line in a multi-line function call (PEAR.Functions.FunctionCallSignature.MultipleArguments)
/includes/sanitizers/class-amp-nav-menu-dropdown-sanitizer.php:125:16: error - Only one argument is allowed per line in a multi-line function call (PEAR.Functions.FunctionCallSignature.MultipleArguments)
/includes/sanitizers/class-amp-nav-menu-dropdown-sanitizer.php:125:18: error - Closing parenthesis of a multi-line function call must be on a line by itself (PEAR.Functions.FunctionCallSignature.CloseBracketLine)
/tests/test-class-amp-gfycat-embed-handler.php:23:9: error - Opening parenthesis of a multi-line function call must be the last content on the line (PEAR.Functions.FunctionCallSignature.ContentAfterOpenBracket)
/tests/test-class-amp-gfycat-embed-handler.php:23:42: error - Only one argument is allowed per line in a multi-line function call (PEAR.Functions.FunctionCallSignature.MultipleArguments)
/tests/test-class-amp-gfycat-embed-handler.php:28:12: error - Only one argument is allowed per line in a multi-line function call (PEAR.Functions.FunctionCallSignature.MultipleArguments)
/tests/test-class-amp-gfycat-embed-handler.php:28:16: error - Only one argument is allowed per line in a multi-line function call (PEAR.Functions.FunctionCallSignature.MultipleArguments)
/tests/test-class-amp-gfycat-embed-handler.php:28:18: error - Closing parenthesis of a multi-line function call must be on a line by itself (PEAR.Functions.FunctionCallSignature.CloseBracketLine)

westonruter · 2019-01-14T23:52:40Z

We should actually do a PHPCS cleanup task to get the entire repo up-to-date with the latest WPCS 1.2.x standard anyway.

westonruter · 2019-01-15T06:17:49Z

Question: Why go the route of patches vs forking the repo and commit directly do it? That's what I did for PHP-CSS-Parser. Seems it could be easier to maintain the patches that way?

felixarntz · 2019-01-15T11:35:30Z

@westonruter I opened #1810 to update and fix all coding standards errors. Let's merge that one first, then I'll pull it in here, fix merge conflicts to make it pass.

…s and deprecate it on the public method.

felixarntz · 2019-01-15T12:35:11Z

@westonruter

Why go the route of patches vs forking the repo and commit directly do it? That's what I did for PHP-CSS-Parser. Seems it could be easier to maintain the patches that way?

I think for simple tweaks the patching workflow is more pragmatic since that allows us to manage everything from this repository. Looking at the PHP-CSS-Parser, the custom tweaks there are more complex, so that justifies a fork.

I suggest we use the patching approach for now with this dependency. If and once we need to make more changes, we can use our best judgement to at some point decide the patching approach isn't viable anymore, and then we could move to a forked repository.

…rimage-security-fix * 'develop' of github.com:ampproject/amp-wp: Re-switch php-compatibility from wimg to phpcompatibility; pin at v9.1.1 Remove unnecessary CommentedOutCode ignore Revert "Update WPCS and PHPCompatibility to latest versions and rely on lock file for concrete versions." Fix all outstanding coding standards violations, via PHPCBF, adjustments to phpcs.xml and manual modifications. Remove outdated WPCS annotations that are no longer preferred. Update WPCS and PHPCompatibility to latest versions and rely on lock file for concrete versions.

westonruter · 2019-01-15T21:31:02Z

What is the current process of packaging the plugin for release? We need to ensure the patches folder is ignored in that process.

Fixed as of 69543ed. To run a build to create an amp.zip package you do: https://github.com/ampproject/amp-wp/blob/develop/contributing.md#creating-a-plugin-build

As a more general observation not directly related to this change, we should ensure when packaging for release the dependencies are installed on the correct environment (PHP 5.4), otherwise we risk installing them in a higher version. Not sure whether this has been considered or tested before. Note that Composer takes the version specification in composer.json as minimum, not as "the dependencies have to be installed in a version compatible with that".

This is a great question. This has not been a consideration that has been made when making a release. But which dependencies would be a problem here?

westonruter · 2019-01-15T21:31:29Z

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

 		$urls       = array_keys( $urls_to_fetch );
 		$user_agent = apply_filters( 'amp_extract_image_dimensions_get_user_agent', self::get_default_user_agent() );
-		$client     = new \FasterImage\FasterImage( $user_agent );
+		$client     = new \FasterImage\FasterImage( $user_agent ); // @todo The $user_agent is not actually able to be passed in this way to FasterImage. Needs another patch?


This is something we should fix as well.

@felixarntz What is the workflow to add a new patch?

I'm guessing we will want to do something like this here:

--- a/wp-content/plugins/amp/third_party/fasterimage/FasterImage.php +++ b/wp-content/plugins/amp/third_party/fasterimage/FasterImage.php @@ -25,7 +25,25 @@ class FasterImage */ protected $timeout = 10; - /** + /** + * User agent. + * + * @var string + */ + protected $user_agent = 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/27.0.1453.110 Safari/537.36'; + + /** + * FasterImage constructor. + * + * @param string $user_agent User agent. + */ + public function __construct( $user_agent = '' ) { + if ( $user_agent ) { + $this->user_agent = $user_agent; + } + } + + /** * Get the size of each of the urls in a list * * @param array $urls @@ -115,7 +133,7 @@ class FasterImage curl_setopt($ch, CURLOPT_TIMEOUT, $this->timeout); # Some web servers require the useragent to be not a bot. So we are liars. - curl_setopt($ch, CURLOPT_USERAGENT, 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/27.0.1453.110 Safari/537.36'); + curl_setopt($ch, CURLOPT_USERAGENT, $this->user_agent); curl_setopt($ch, CURLOPT_HTTPHEADER, [ "Accept: text/xml,application/xml,application/xhtml+xml,text/html;q=0.9,text/plain;q=0.8,image/png,*/*;q=0.5", "Cache-Control: max-age=0",

I fixed this now.

What I currently do is I have a local checkout of the FasterImage library in the tagged version that we're using. I apply the existing patch against it, change code, recreate the patch again.

Note that you can't just use the master branch since it contains code that isn't part of a release yet. However, for future releases, our patch file will just apply until something in that same area changes, which would naturally cause a merge conflict. Composer would notify us about that.

…amp-wp into fix/fasterimage-security-fix

felixarntz · 2019-01-16T09:43:05Z

@westonruter

This is a great question. This has not been a consideration that has been made when making a release. But which dependencies would be a problem here?

Generally, anything for which we don't specify a fixed version number is a problem. For example, if we say ^1 for a package, and version 1.2 relies on PHP 5.4 and 1.3 relies on PHP 5.5, and our setup is >= 5.5, it would use the later version, which is obviously a problem.

However, it is a good practice to not specify fixed versions, but rather rely on semantic versioning (I suggest we use ^8 for the PHP-CSS-Parser, ^1 for FasterImage, ^1 for WPCS, ^0.4 for the installer and so on). The specific versions are supposed to be managed via composer.lock, and developers should always use composer install unless they intend to update the dependencies for the whole project.

I added a config --> platform entry to composer.json which will ensure that Composer ignores the local setup it is run from and always installs dependencies in a way that are 5.4-compatible. This makes it bullet-proof, and furthermore helps making sure all of us contributing to the plugin are on the same page regarding dependencies as well.

westonruter · 2019-01-16T19:44:08Z

However, it is a good practice to not specify fixed versions, but rather rely on semantic versioning (I suggest we use ^8 for the PHP-CSS-Parser, ^1 for FasterImage, ^1 for WPCS, ^0.4 for the installer and so on). The specific versions are supposed to be managed via composer.lock, and developers should always use composer install unless they intend to update the dependencies for the whole project.

Normally yes, but we don't need to worry about that now because Renovate will take care of making sure that each dependency is updated. It will set the dependency on fixed versions so that we can be sure that a tagged version will always have the exact same versioned dependencies at the time of tagging. It's also true that the composer.lock would make the fixed version in composer.json a redundancy. So essentially we can delegate composer update to Renovate.

felixarntz · 2019-01-16T19:46:21Z

@westonruter That's fine with me then. We should still keep the config --> platform entry in composer.json to ensure we don't break anything. Even for the case that Renovate doesn't know that we always need to be 5.4-compatible. :)

westonruter · 2019-01-16T21:19:49Z

Great changes! 🎉 Good work.

…velop-to-1.0.2 * 'develop' of github.com:ampproject/amp-wp: (26 commits) Fix PEAR.Functions.FunctionCallSignature phpcs issues Strip old-school CDATA and HTML comments from XHTML-compatible style elements Re-simplify condition include_manifest_comment; remove undefined constant Add tests for style sanitizer include_manifest_comment option Introduce when_css_excluded option for include_manifest_comment arg Suppress style[amp-custom] manifest HTML comment when not WP_DEBUG Add missing vendor deps to build after #1809 Update dependency xwp/wp-dev-lib to v1.0.1 Add wp_generator to classic post template Remove Powered by WordPress in classic footer template Fix broken unit test as it is just as valid now. Remove unnecessary strlen() call. Ignore the home URL's path if present when normalizing image URLs. Go back to using site_url() in AMP_Style_Sanitizer and harden concatenation of relative URLs. Remove redundant composer install Only set CHECK_SCOPE=all on Travis (see #1822) Improve sanity check instructions in release steps Pin wp-dev-lib to 1.0.0 Eliminate references to submodules from contributing.md Remove obsolete DEV_LIB_SKIP of composer ...

…alidation-performance * 'develop' of github.com:ampproject/amp-wp: (107 commits) Remove unnecessary home option setting; only register theme dir when needed Fix tests related to home/siteurl when WP_HOME/WP_SITEURL are set Fix test_get_validated_url_file_path to account for custom content dir Fix PEAR.Functions.FunctionCallSignature phpcs issues Strip old-school CDATA and HTML comments from XHTML-compatible style elements Re-simplify condition include_manifest_comment; remove undefined constant Add tests for style sanitizer include_manifest_comment option Introduce when_css_excluded option for include_manifest_comment arg Suppress style[amp-custom] manifest HTML comment when not WP_DEBUG Add missing vendor deps to build after #1809 Update dependency xwp/wp-dev-lib to v1.0.1 Add wp_generator to classic post template Remove Powered by WordPress in classic footer template Fix broken unit test as it is just as valid now. Remove unnecessary strlen() call. Ignore the home URL's path if present when normalizing image URLs. Go back to using site_url() in AMP_Style_Sanitizer and harden concatenation of relative URLs. Update package-lock.json after re-install Fix readme date and typo Remove redundant composer install ...

…ce-worker * 'develop' of github.com:ampproject/amp-wp: (335 commits) Ensure that amp-fx-collection is included when amp-fx attribute is present Manually enqueue amp-lightbox-gallery component when lightbox attribute is present Add failing test for amp-img[lightbox] not causing amp-lightbox-gallery to be enqueued Remove unnecessary home option setting; only register theme dir when needed Fix tests related to home/siteurl when WP_HOME/WP_SITEURL are set Fix test_get_validated_url_file_path to account for custom content dir Fix PEAR.Functions.FunctionCallSignature phpcs issues Strip old-school CDATA and HTML comments from XHTML-compatible style elements Re-simplify condition include_manifest_comment; remove undefined constant Add tests for style sanitizer include_manifest_comment option Introduce when_css_excluded option for include_manifest_comment arg Suppress style[amp-custom] manifest HTML comment when not WP_DEBUG Add missing vendor deps to build after #1809 Update dependency xwp/wp-dev-lib to v1.0.1 Add wp_generator to classic post template Remove Powered by WordPress in classic footer template Fix broken unit test as it is just as valid now. Remove unnecessary strlen() call. Ignore the home URL's path if present when normalizing image URLs. Go back to using site_url() in AMP_Style_Sanitizer and harden concatenation of relative URLs. ...

felixarntz added 3 commits January 14, 2019 21:01

Update minimum required PHP version to 5.4.

a0537f6

Remove now unnecessary FastImage library.

04f44c6

Pull in FasterImage library via Composer and apply manual changes via…

5bff06b

… patch file.

googlebot added the cla: yes Signed the Google CLA label Jan 14, 2019

felixarntz requested a review from westonruter January 14, 2019 20:14

westonruter reviewed Jan 14, 2019

View reviewed changes

westonruter added 2 commits January 14, 2019 14:01

Fix MultipleStatementAlignment in autoloader manifest

bb2014a

Fix FunctionCallSignature for new WP code style

08ea6c6

Remove unnecessary support for argument when fetching image dimension…

a4b1aad

…s and deprecate it on the public method.

westonruter mentioned this pull request Jan 15, 2019

Use JSON_UNESCAPED_UNICODE for comment state if available #1811

Merged

westonruter added 5 commits January 15, 2019 13:05

Improve phpdoc; catch Exception from fetch_images; add todo

81ab98e

Exclude patches from build

69543ed

Restore linebreaks in readme.md

1dbd4e8

Ensure that Travis will npm install and composer install for each build

f51bfc5

Eliminate PHP 5.3-specific testsuite

50b3621

westonruter added this to the v1.1 milestone Jan 15, 2019

Eliminate PHP 5.3-compatibility workarounds for closures

939928c

westonruter reviewed Jan 15, 2019

View reviewed changes

felixarntz added 4 commits January 16, 2019 10:26

Ensure FasterImage allows to set its user agent.

1629db6

Ensure all dependencies are PHP 5.4 compatible.

35c33f1

Fix composer.json indentation.

2cd6669

Merge branch 'fix/fasterimage-security-fix' of github.com:ampproject/…

acc3f44

…amp-wp into fix/fasterimage-security-fix

Correct license in composer.json as GPL-2.0-or-later

39e68ea

westonruter approved these changes Jan 16, 2019

View reviewed changes

westonruter merged commit 84c1894 into develop Jan 16, 2019

westonruter deleted the fix/fasterimage-security-fix branch January 16, 2019 21:17

westonruter added a commit that referenced this pull request Jan 28, 2019

Add missing vendor deps to build after #1809

aeafce9

westonruter mentioned this pull request Jan 28, 2019

Add missing FasterImage and Stream vendor dependencies in builds #1841

Merged

westonruter added the changelogged label Mar 23, 2019

westonruter mentioned this pull request May 13, 2019

Add curl as required extension; refactor how dependency errors are communicated #2183

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump minimum PHP version requirement to 5.4 and include FasterImage library dynamically #1809

Bump minimum PHP version requirement to 5.4 and include FasterImage library dynamically #1809

felixarntz commented Jan 14, 2019

westonruter Jan 14, 2019

felixarntz Jan 15, 2019

westonruter May 13, 2019

westonruter commented Jan 14, 2019

westonruter commented Jan 14, 2019 •

edited

Loading

westonruter commented Jan 14, 2019

westonruter commented Jan 15, 2019

felixarntz commented Jan 15, 2019

felixarntz commented Jan 15, 2019

westonruter commented Jan 15, 2019

westonruter Jan 15, 2019

westonruter Jan 15, 2019

felixarntz Jan 16, 2019

felixarntz commented Jan 16, 2019

westonruter commented Jan 16, 2019

felixarntz commented Jan 16, 2019

westonruter commented Jan 16, 2019

Bump minimum PHP version requirement to 5.4 and include FasterImage library dynamically #1809

Bump minimum PHP version requirement to 5.4 and include FasterImage library dynamically #1809

Conversation

felixarntz commented Jan 14, 2019

westonruter Jan 14, 2019

Choose a reason for hiding this comment

felixarntz Jan 15, 2019

Choose a reason for hiding this comment

westonruter May 13, 2019

Choose a reason for hiding this comment

westonruter commented Jan 14, 2019

westonruter commented Jan 14, 2019 • edited Loading

westonruter commented Jan 14, 2019

westonruter commented Jan 15, 2019

felixarntz commented Jan 15, 2019

felixarntz commented Jan 15, 2019

westonruter commented Jan 15, 2019

westonruter Jan 15, 2019

Choose a reason for hiding this comment

westonruter Jan 15, 2019

Choose a reason for hiding this comment

felixarntz Jan 16, 2019

Choose a reason for hiding this comment

felixarntz commented Jan 16, 2019

westonruter commented Jan 16, 2019

felixarntz commented Jan 16, 2019

westonruter commented Jan 16, 2019

westonruter commented Jan 14, 2019 •

edited

Loading