Upgrade Guzzle to v6 by aaemnnosttv · Pull Request #6581 · google/site-kit-wp

aaemnnosttv · 2023-02-11T03:26:31Z

Summary

Addresses issue:

Update to Guzzle 6+ #1146

Relevant technical choices

Relevant docs

https://github.com/guzzle/guzzle/blob/master/UPGRADING.md#50-to-60 (for reference – most of this shouldn't affect us because we don't use the client directly outside of a few customizations – most of the client handling is done in the Google\Client)
https://docs.guzzlephp.org/en/6.5/request-options.html

PR Author Checklist

My code is tested and passes existing unit tests.
My code has an appropriate set of unit tests which all pass.
My code is backward-compatible with WordPress 5.2 and PHP 5.6.
My code follows the WordPress coding standards.
My code has proper inline documentation.
I have added a QA Brief on the issue linked above.
I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

Do not alter or remove anything below. The following sections will be managed by moderators only.

Code Reviewer Checklist

Run the code.
Ensure the acceptance criteria are satisfied.
Reassess the implementation with the IB.
Ensure no unrelated changes are included.
Ensure CI checks pass.
Check Storybook where applicable.
Ensure there is a QA Brief.

Merge Reviewer Checklist

Ensure the PR has the correct target branch.
Double-check that the PR is okay to be merged.
Ensure the corresponding issue has a ZenHub release assigned.
Add a changelog message to the issue.

aaemnnosttv · 2023-02-11T04:13:44Z

composer.json

    "true/punycode": "^2.0 <2.1.1"
  },
  "replace": {
+    "paragonie/random_compat": ">=2",


random_compat is bundled in WordPress so this avoids loading via Composer, potentially loading an older version.

The version here is significant as any version below version 2 cannot be installed due to roave/security-advisories, so the constraint must only target versions which could actually be installed. This configuration causes the package not to be installed.

aaemnnosttv · 2023-02-11T04:15:05Z

includes/Core/Authentication/Clients/Client_Factory.php

 		$client->setPrompt( 'consent' );
 		$client->setRedirectUri( $args['redirect_uri'] );
 		$client->setScopes( (array) $args['required_scopes'] );
-		$client->prepareScopes();


This method didn't actually do anything meaningful as it has no side-effects, it only returns the list of requested scopes as a single string.

aaemnnosttv · 2023-02-11T04:16:47Z

scoper.inc.php


 		// This dependency is a global function which should remain global.
 		'vendor/ralouphie/getallheaders/src/getallheaders.php',
+		'vendor/symfony/polyfill-intl-normalizer/bootstrap.php',


There may be others that are needed here, this raised a failure without it so it was the only one added. See the generated third-party/vendor/autoload_files.php for other possible entries here.

I'm interested to know where the failure was raised, as looking at the resulting third-party/symfony/polyfill-intl-normalizer/bootstrap.php, the generated normalizer_is_normalized() / normalizer_normalize() functions don't work properly as they refer to Symfony\Polyfill\Intl\Normalizer, whereas as can be seen examining third-party/symfony/polyfill-intl-normalizer/Normalizer.php, this Normalizer class is now defined within the Google\Site_Kit_Dependencies\Symfony\Polyfill\Intl\Normalizer namespace.

I think it would be a more correct fix to instead use Scoper to modify the resulting bootstrap.php file to simply avoid adding a namespace statement. I dug into this a bit and was able to test the current generated normalizer_is_normalized() function, which threw a PHP Fatal error: Uncaught Error: Class 'Symfony\\Polyfill\\Intl\\Normalizer\\Normalizer' not found error, whereas a modded version of bootstrap.php as generated without being in the files-whitelist array, but with the namespace line removed, resulted in a working normalizer_is_normalized()...

Given that the normalizer_is_normalized() / normalizer_normalize() functions are called in the WP core remove_accents() function which is in turn called by Tag_Manager::sanitize_container_name(), we need to be a bit careful here...

Good call, this was the part I think was most likely to introduce problems, but I think removing the namespace from the global bootstrap.php files for polyfills should fix it 👍

I wasn't able to reproduce the initial error that caused me to add the one path to the whitelist initially, but it shouldn't be needed anymore anyways with the more robust solution.

aaemnnosttv · 2023-02-11T04:18:30Z

tests/phpunit/includes/FakeHttpClient.php

-	 * @return \Google\Site_Kit_Dependencies\GuzzleHttp\Message\ResponseInterface
+	 * @return ResponseInterface
 	 */
-	public function send( RequestInterface $request ) {


This class is no longer useful for its intended purpose and needs to be replaced.

aaemnnosttv · 2023-02-11T04:19:52Z

tests/phpunit/integration/Core/Authentication/Clients/OAuth_ClientTest.php

 		$fake_http_client->set_request_handler(
 			function ( Request $request ) use ( $activity_metrics ) {
-				if ( 0 !== strpos( $request->getUrl(), 'https://oauth2.googleapis.com/token' ) ) {
+				if ( 0 !== strpos( $request->getUri(), 'https://oauth2.googleapis.com/token' ) ) {


Naming difference in the new interface.

aaemnnosttv · 2023-02-11T04:21:52Z

tests/phpunit/integration/Modules/Analytics_4Test.php

+		$this->authentication->get_oauth_client()->get_client()->setHttpClient( $http_client );
+		$this->analytics->get_owner_oauth_client()->get_client()->setHttpClient( $http_client );


This looks like it was missing initially but may have been working by coincidence. This will need to change going forward as using an alternate HTTP client won't work for this with Guzzle 6.

aaemnnosttv

Note to the reviewer: the diff will be easiest to review with whitespace ignored/hidden as indentation has changed in some chunks but otherwise most of the content has been preserved with minimal changes.

aaemnnosttv · 2023-03-03T19:05:53Z

tests/phpunit/integration/Modules/Tag_ManagerTest.php


 use Google\Site_Kit\Context;
 use Google\Site_Kit\Core\Modules\Module;
+use Google\Site_Kit\Core\Modules\Module_With_Service_Entity;


There are a few instances like this where unused imports were cleaned up as well.

techanvil

Nice work, @aaemnnosttv. I have left a couple of comments, a suggestion and a couple of questions, please take a look.

techanvil · 2023-03-06T18:57:08Z

scoper.inc.php


 		// This dependency is a global function which should remain global.
 		'vendor/ralouphie/getallheaders/src/getallheaders.php',
+		'vendor/symfony/polyfill-intl-normalizer/bootstrap.php',


I'm interested to know where the failure was raised, as looking at the resulting third-party/symfony/polyfill-intl-normalizer/bootstrap.php, the generated normalizer_is_normalized() / normalizer_normalize() functions don't work properly as they refer to Symfony\Polyfill\Intl\Normalizer, whereas as can be seen examining third-party/symfony/polyfill-intl-normalizer/Normalizer.php, this Normalizer class is now defined within the Google\Site_Kit_Dependencies\Symfony\Polyfill\Intl\Normalizer namespace.

I think it would be a more correct fix to instead use Scoper to modify the resulting bootstrap.php file to simply avoid adding a namespace statement. I dug into this a bit and was able to test the current generated normalizer_is_normalized() function, which threw a PHP Fatal error: Uncaught Error: Class 'Symfony\\Polyfill\\Intl\\Normalizer\\Normalizer' not found error, whereas a modded version of bootstrap.php as generated without being in the files-whitelist array, but with the namespace line removed, resulted in a working normalizer_is_normalized()...

Given that the normalizer_is_normalized() / normalizer_normalize() functions are called in the WP core remove_accents() function which is in turn called by Tag_Manager::sanitize_container_name(), we need to be a bit careful here...

techanvil · 2023-03-06T19:14:29Z

tests/phpunit/integration/Modules/Analytics_4Test.php

+		FakeHttp::fake_google_http_handler(
+			$this->authentication->get_oauth_client()->get_client(),
+			$fake_http_handler
+		);
+		FakeHttp::fake_google_http_handler(
+			$this->analytics->get_owner_oauth_client()->get_client(),
+			$fake_http_handler
+		);


This is a slight departure from the existing logic, where just setting the handler for $this->analytics->get_client() as below would be a direct mapping of it. I seem to recall you mentioned something about this, though, where we should have been updating the authentication client as well as the Analytics client. Therefore, I'm pretty confident your update here will be the right thing to do, but would you mind clarifying it for my understanding?

FakeHttp::fake_google_http_handler( $this->analytics->get_client(), $fake_http_handler );

Ah, maybe it was this comment I was thinking of, which I didn't spot at the time of writing the above...

Yes, I mentioned this above in https://github.com/google/site-kit-wp/pull/6581/files/b58274c240118142014bca49d98ad01c00d754a8#r1103512495.

I thought it was missing the handler set on the module owners client instance since the test uses the data_access_token data provider, which includes the empty token, so I probably assumed the test was intended to run in a sharing context as well, but this one is not so it's all good. I've restored the original form 👍

techanvil

Looks like this needs a bit more attention, as the E2E tests are failing... Please see my separate comment with a bit of detail on this.

scoper.inc.php

techanvil

LGTM, nice one @aaemnnosttv!

aaemnnosttv added 7 commits February 10, 2023 20:52

Upgrade Guzzle and its dependencies.

a7e4795

Include symfony packages in third-party prefixed.

4ffbbe5

Update Guzzle specific options in Client_Factory.

f3da221

Update FakeHttpClient::send for compatibility with parent.

47d3b30

Refactor custom response in GTM test.

25b47a4

Add missing import.

6fccd19

Refactor with new classes.

b58274c

aaemnnosttv commented Feb 11, 2023

View reviewed changes

aaemnnosttv mentioned this pull request Feb 11, 2023

Update to Guzzle 6+ #1146

Closed

aaemnnosttv added 4 commits March 3, 2023 12:28

Merge branch 'develop' into enhance/1146-guzzle6.

92e907f

Add FakeHttp helper class.

cc28550

Refactor use of FakeHttpClient with FakeHttp.

0db2b41

Remove FakeHttpClient.

ba141cf

aaemnnosttv marked this pull request as ready for review March 3, 2023 18:58

aaemnnosttv commented Mar 3, 2023

View reviewed changes

aaemnnosttv requested a review from techanvil March 3, 2023 19:17

techanvil requested changes Mar 6, 2023

View reviewed changes

aaemnnosttv added 3 commits March 6, 2023 17:26

Remove namespace from polyfill bootstrapping.

673469a

Restore original form in test_get_report.

bc54417

Merge branch 'develop' into enhance/1146-guzzle6.

d1e94e5

techanvil requested changes Mar 7, 2023

View reviewed changes

scoper.inc.php Show resolved Hide resolved

aaemnnosttv added 3 commits March 9, 2023 16:51

Merge branch 'develop' into enhance/1146-guzzle6.

a8b919b

Update scoper patching to allow global class stubs.

4a1abaa

Remove SK prefix constraint from autoloader.

42c6b7e

techanvil approved these changes Mar 10, 2023

View reviewed changes

techanvil merged commit 61404a7 into develop Mar 10, 2023

techanvil deleted the enhance/1146-guzzle6 branch March 10, 2023 13:16

		$this->authentication->get_oauth_client()->get_client()->setHttpClient( $http_client );
		$this->analytics->get_owner_oauth_client()->get_client()->setHttpClient( $http_client );

Conversation

aaemnnosttv commented Feb 11, 2023 • edited by techanvil Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Relevant technical choices

PR Author Checklist

Code Reviewer Checklist

Merge Reviewer Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaemnnosttv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

techanvil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

techanvil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

techanvil left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aaemnnosttv commented Feb 11, 2023 •

edited by techanvil

Loading