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
Raise minimum required version of PHP to 7.4 #8030
Comments
@eclarke1 @bethanylang @ivonac4 this one must go in 1.125.0 for release on April 22 as noted above. I've pre-assigned the release accordingly but we could potentially get started on it in the previous sprint. |
Thanks, @aaemnnosttv! @ivonac4 has an Asana task that I created here for this. I've updated the due date for us to include this in Sprint 124 to be safe. |
@zutigrm this is something we'll want to make sure we understand the implications of. It looks like this change would only affect the non-proxy-connected case, but our other classes extend the base |
Yeah, we will need to implement the new endpoints on the service side. I have added an asana task for it. UPD: I don't expect there will be needed significant changes in the current oauth2 process on the service side. We will need to add a new endpoint and bind the existing handler to it. Some changes may be needed for the callback endpoint, but that will be more clear when we start working on it. |
Just to note that #8094 was merged very recently, it seems that package can be upgraded also as part of this issue |
@eugene-manuilov @zutigrm I'm not sure this requires any change for us, since the test referenced here is using a non-proxy-connected scenario. When a site is connected to the proxy, we override the OAuth2 service which is where our endpoints are defined. See My assumption is that we should be able to update that test to use the new URLs for the Google OAuth endpoints that are used when a site does not use the proxy, but we need to better understand the changes to know that no other updates would be necessary in the code base. We should probably also add a test case for the same test to use the proxy as that coverage may be incomplete. |
Assigning this task back to @zutigrm, since he worked on IB for it. |
Indeed, thanks. I just tested connecting the Site Kit, with proposed updates from IB, and I do not see any issue, regarding the OAuth, scopes, etc. I updated the IB to include the test with proxy per your suggestion |
Thanks @zutigrm – there are a few more changes we should make here. For example, we can raise our minimum versions for some dependencies to remove ranges that support older versions of PHP. E.g. PHPUnit < 9 support PHP 7.3 and below. We can also upgrade to phpunit-polyfills v2. The polyfills version change is more important since it's a major version change – Composer will handle the others where a range is wider than we need automatically but it helps to clean it up. Other things like Other than that, this is basically good to go. I'll move this forward though since the rest can be iterated on during execution. IB ✅ |
QA Update
|
@mohitwp There's nothing in our code that actually prevents Site Kit from running on older versions of PHP, I think it'd be up to WordPress. I think WordPress will prevent you from installing the plugin if your PHP version is outdated/unsupported, but I don't think it will uninstall any existing plugins. You can try deactivating the plugin then reactivating it AND/OR installing Site Kit on PHP 7.3, which WordPress might stop you from doing. But if it's already installed it shouldn't be an issue I think. |
QA Update ✅@tofumatt Thanks ! Currently, WP allow installing Site kit on PHP version 7.3.
cc @wpdarren @kelvinballoo Changes involved here make significant impact on plugin. Flagging here for keeping in mind this during release QA. |
@tofumatt We do guard against loading the plugin if minimum versions are not met. WP does more to handle this too but that was not always the case. site-kit-wp/google-site-kit.php Lines 120 to 125 in 6af3955
|
@mohitwp @tofumatt, did we complete any testing on a site on PHP 7.3 and look at the behavior on the develop/main branch? I assume the banner still appears, notifying them they need to upgrade. Does Site Kit still function for them? No errors occur on dashboards or any other functionality. I am assuming all they cannot do is update future versions of Site Kit, but everything else should function as normal. I couldn't see any reference to this testing in the QA update or any screenshots, so I wanted to double-check. |
Feature Description
While Site Kit has long supported the minimum version of PHP required by WP, recent changes in core dependencies require that we raise our minimum version to leverage features, fixes, and language compatibility updates that are only available in more recent releases.
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
Implementation Brief
google-site-kit.php
Requires PHP
to7.4
site-kit-wp/google-site-kit.php
Line 31 in 49c312c
composer.json
require.php
to>=7.4
config.platform.php
to7.4.33
phpcs.xml
, intestVersion
config increment php version to7.4-
site-kit-wp/composer.json
Line 31 in 49c312c
*
, it needs to have>=3.0.36
, as versions bellow are added to the conflict list of SecurityAdvisory andcomposer update
will failsymfony/polyfill-intl-idn
package can also be updated to higher version now, we were forced to go with current one in Update usage of abandoned package #8094 due to the php versionGoogle\Site_Kit\Tests\Core\Authentication\Clients\OAuth_ClientTest::test_get_authentication_url
test is failing asgoogle/apiclient
package version (updates tov2.14.0
) and in one of the versions it changes the OAuth url path to includev2
- and in this test we are not using proxy so it is expectinghttps://accounts.google.com/o/oauth2/auth
which will be nowhttps://accounts.google.com/o/oauth2/v2/auth
. You can see the change in this commitv2.12.2
andv2.14.0
in the change log. During the definition, I took a brief look, and v2 is the only one I saw, but more might surface during actual executionTest Coverage
QA Brief
Changelog entry
The text was updated successfully, but these errors were encountered: