site-kit-wp icon indicating copy to clipboard operation
site-kit-wp copied to clipboard
verified publisher icon google

Update to Guzzle 6+

Open swissspidy opened this issue 5 years ago • 9 comments

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

QA Brief

Changelog entry

swissspidy avatar Feb 19 '20 12:02 swissspidy

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 avatar Feb 19 '20 16:02 aaemnnosttv

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 avatar Mar 30 '21 14:03 aaemnnosttv

@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? 🤔

asvinb avatar Sep 22 '22 09:09 asvinb

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 avatar Sep 22 '22 18:09 aaemnnosttv

@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 avatar Sep 26 '22 08:09 asvinb

@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 avatar Sep 26 '22 17:09 aaemnnosttv

@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 avatar Sep 27 '22 07:09 asvinb

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

aaemnnosttv avatar Sep 27 '22 18:09 aaemnnosttv

@aaemnnosttv I can give it a try although last time i had a look, I was stuck on some composer issue.

asvinb avatar Sep 28 '22 08:09 asvinb

IB reviewer: please assign this back to me to work on when ready 👍

aaemnnosttv avatar Feb 11 '23 04:02 aaemnnosttv

IB ✔️

eugene-manuilov avatar Feb 13 '23 20:02 eugene-manuilov

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 avatar Mar 31 '23 07:03 mohitwp

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 avatar Apr 03 '23 09:04 tofumatt

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

👍🏻

tofumatt avatar Apr 06 '23 21:04 tofumatt