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

Update to Guzzle 6+ #1146

Closed
swissspidy opened this issue Feb 19, 2020 · 14 comments
Closed

Update to Guzzle 6+ #1146

swissspidy opened this issue Feb 19, 2020 · 14 comments
Labels
Exp: SP P1 Medium priority PHP QA: Eng Requires specialized QA by an engineer Type: Infrastructure Engineering infrastructure & tooling

Comments

@swissspidy
Copy link
Contributor

swissspidy commented Feb 19, 2020

Feature Description

As mentioned a few times, most recently again in #223.

Perhaps it could just be bumped? google/apiclient supports it, so there shouldn't be a conflict in that regard.


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

Acceptance criteria

  • guzzlehttp/guzzle should be upgraded to the latest v6 version (but not v7)
    • All existing Guzzle client customizations must continue to work (see Client_Factory::create_client)

Implementation Brief

  • A working branch has been started in https://github.com/google/site-kit-wp/tree/enhance/1146-guzzle6 with a draft PR in Upgrade Guzzle to v6 #6581
  • Most of the exploration to understand what is needed here should be done, the main part remaining is refactoring the use of FakeHttpClient with Guzzle's MockHandler
    • The reason for this is because staring with Guzzle v6, the Client is designed to be immutable, so in order to make changes, a new instance must be set/returned. Because of this, an alternate client instance is lost as soon as such a modification is needed, and since this happens within the Google API client as part of every request when providing the access_token, it's no longer a viable strategy.
    • The MockHandler for overriding requests in tests will persist across Client instances via the handler request option.
  • Replace the use of FakeHttpClient with a MockHandler via the Guzzle client's handler configuration. See https://docs.guzzlephp.org/en/6.5/testing.html for details
    • This is not a 1-1 replacement as a MockHandler serves requests in order from a given queue whereas FakeHttpClient has full control over the response returned so we may want to look into extending the MockHandler to allow for conditional responses rather than a simple queue (a quick search yielded a package that provides something similar so it should at least be possible, or we can maybe even just use this one)
    • The flow to mock one or more responses generally looks like this
      • Get the Guzzle config from the Google API's http client (Google_Client->getHttpClient()->getConfig())
      • Modify the config to include the configured handler
      • Create a new Client using the new/modified config array
      • Set the new Client back in the Google API client (Google_Client->setHttpClient(newClientInstance))
  • Remove FakeHttpClient

QA Brief

  • This issue changes the main underlying library used for making API requests with Google APIs, including oAuth token requests
  • Setup flows and dashboard views should be thoroughly checked for potential regressions
  • We should test that things work on both oldest (5.6) and latest (8.0/8.1) supported versions of PHP, as well as PHP 7.4 (being the most common)

QA:Eng

  • Ensure HTTP proxy functionality still works
  • Ensure user-agent and other customizations to the client still work

Changelog entry

  • Upgrade Guzzle, with guzzlehttp/guzzle updated to v6.5.8.
@aaemnnosttv
Copy link
Collaborator

I tried bumping the version while working on #547 but as I recall it didn't work so we kept it at 5.x for now.

@aaemnnosttv aaemnnosttv self-assigned this Mar 30, 2021
@aaemnnosttv
Copy link
Collaborator

This issue was originally raised related to #41 due to conflicts when installing the plugin via Composer directly using google/google-site-kit. This has never really been supported as this repo is not installable as a working version of the plugin.

We can however keep this issue as an upgrade to do in the future, although there's really no urgency to do so. Moving to stalled.

@aaemnnosttv aaemnnosttv removed their assignment Mar 30, 2021
@aaemnnosttv aaemnnosttv added the Type: Infrastructure Engineering infrastructure & tooling label May 3, 2021
@aaemnnosttv aaemnnosttv added P1 Medium priority PHP labels May 25, 2022
@asvinb asvinb self-assigned this Aug 24, 2022
@asvinb asvinb removed their assignment Sep 14, 2022
@asvinb
Copy link
Collaborator

asvinb commented Sep 22, 2022

@aaemnnosttv Is there any reason why we are not upgrading to v7? The reason is we'll still have some compatibility issues with v6 and PHP 8.1, so why not use v7? 🤔

@aaemnnosttv
Copy link
Collaborator

aaemnnosttv commented Sep 22, 2022

Is there any reason why we are not upgrading to v7?

@asvinb yes, v7 requires PHP 7.2+ https://packagist.org/packages/guzzlehttp/guzzle#7.0.0

Can you elaborate on what the specific issues would be with v6?

@aaemnnosttv aaemnnosttv assigned asvinb and unassigned aaemnnosttv Sep 22, 2022
@asvinb
Copy link
Collaborator

asvinb commented Sep 26, 2022

@aaemnnosttv I thought with the minimum WP version we support being bumped to 5.2, we could use Guzzle 7 but turns out that the minimum supported version is 5.6.20. I posted my comment while working on #5110 where with Guzzle 7, we'll still have some warnings on PHP 8.1, but this is irrelevant now 😅

@asvinb asvinb assigned aaemnnosttv and unassigned asvinb Sep 26, 2022
@aaemnnosttv
Copy link
Collaborator

@asvinb yes, our minimum supported version of PHP isn't changing at the same time as WP. I didn't quite follow your conclusion though – is it not possible to upgrade to v6 for now?

@aaemnnosttv aaemnnosttv assigned asvinb and unassigned aaemnnosttv Sep 26, 2022
@asvinb
Copy link
Collaborator

asvinb commented Sep 27, 2022

@aaemnnosttv Apologies for the confusion. All good to upgrade to the latest v6 👍 Guess I was confused with another ticket I was working on.

@asvinb asvinb assigned aaemnnosttv and unassigned asvinb Sep 27, 2022
@aaemnnosttv
Copy link
Collaborator

@asvinb great! Were you going to write the IB or is someone else best suited for that?

@eugene-manuilov eugene-manuilov self-assigned this Feb 13, 2023
@eugene-manuilov
Copy link
Collaborator

IB ✔️

@eugene-manuilov eugene-manuilov removed their assignment Feb 13, 2023
@aaemnnosttv aaemnnosttv removed their assignment Mar 3, 2023
@techanvil techanvil assigned techanvil and aaemnnosttv and unassigned techanvil Mar 6, 2023
@aaemnnosttv aaemnnosttv assigned techanvil and unassigned aaemnnosttv Mar 7, 2023
@techanvil techanvil assigned aaemnnosttv and unassigned techanvil Mar 7, 2023
@aaemnnosttv aaemnnosttv assigned techanvil and unassigned aaemnnosttv Mar 9, 2023
@techanvil techanvil removed their assignment Mar 10, 2023
@techanvil techanvil added the QA: Eng Requires specialized QA by an engineer label Mar 10, 2023
@wpdarren wpdarren assigned wpdarren and mohitwp and unassigned wpdarren Mar 15, 2023
@mohitwp
Copy link
Collaborator

mohitwp commented Mar 31, 2023

QA Update ✅

  • Tested on dev environment.
  • Verified all modules set up process and dashboard widgets on PHP versions - 5.6, 7.4 and 8.2.
  • Set up OAuth flow is working as expected.
  • Dashboard widgets are appearing as expected.

Require QA :Eng review as per QAB.

@mohitwp mohitwp removed their assignment Mar 31, 2023
@tofumatt tofumatt self-assigned this Apr 3, 2023
@tofumatt
Copy link
Collaborator

tofumatt commented Apr 3, 2023

Took a bit of work to QA the requests since it seems like our requests aren't sent through macOS's networking stack such that apps like Proxyman can pick them up, but got through it in the end 😅

QA ✅

@tofumatt tofumatt removed their assignment Apr 3, 2023
@tofumatt
Copy link
Collaborator

tofumatt commented Apr 6, 2023

Just as another QA note: @aaemnnosttv reminded me how to configure the WordPress proxy and it worked fine:

CleanShot 2023-04-06 at 22 24 15

👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Exp: SP P1 Medium priority PHP QA: Eng Requires specialized QA by an engineer Type: Infrastructure Engineering infrastructure & tooling
Projects
None yet
Development

No branches or pull requests

8 participants