Support for PHP 8 #467
Support for PHP 8 #467
Conversation
|
Hi there, what are the remaining steps before this PR can be merged? |
Hey @Nielsvanpach it's currently going through our internal review process. Hopefully not too much longer. |
|
@evansims Heads up that the dependency for |
|
Hey @johnvandeweghe 👋 Thanks for the heads up! We're in the process of moving the fixed fork branch you mentioned into the Auth0 org, so things are getting renamed and moved around a bit at the moment. The fork has now come to rest at it's final destination, https://github.com/auth0/php-jwt. I've updated this PR branch to reflect the new location. |
| "ext-json": "*", | ||
| "lcobucci/jwt": "^3.3", | ||
| "ext-openssl": "*", |
There was a problem hiding this comment.
is there a minimum version we can require here? better than using "any". Same for ext-json above.
There was a problem hiding this comment.
Ideally yes, I agree that would be preferred here. The problem with specifying versions in this manner for PHP extensions is that it will block the SDK from installing in environments running in-development/pre-release versions of PHP.
For example, we could specify "ext-json": ^1.7 | ^7.4 | ^8.0, and our SDK would install for all GA versions of PHP we support. However, it would block installation in any forthcoming snapshots, betas or RCs, such as imminent development around 7.5 or 8.1 releases. You'd be surprised how many people run those. We can get around this by specifying development versions but this will lead to a lot of maintenance on our end.
Besides, it isn't really terribly important what version of these extensions a user has installed here, as long as they have one and it's enabled. These extensions will only build and work for appropriate PHP versions, so it's impossible to have an out of date extension on an up to date and supported PHP version.
| "require": { | ||
| "php": "^7.1", | ||
| "guzzlehttp/guzzle": "~6.0 | ~7.0", | ||
| "php": "^7.3 | ^8.0", |
There was a problem hiding this comment.
how often do we do these changes, shouldn't these be on a major release instead of a minor?
There was a problem hiding this comment.
How often do we do these changes
Dropping PHP versions isn't often required, every 2-3 years generally. (Our SDK technically still runs fine on 7.1, but we have to accommodate dependency requirements.)
Moving forward, my goal is for us to follow the official PHP support schedule by deprecating (at year 2) and ultimately dropping (at year 3) support for EOL PHP versions in our SDK, following the official runtime support and security expirations. This would allow us to offer clarity and transparency as to our support windows for customers, and follows best practices in general. It would also have avoided us needing to leapfrog support for two PHP versions at once, as well. But this is a discussion for another day, I suppose.
Shouldn't these be on a major release instead of a minor?
Since there are no public API changes on our end, and therefor no breaking changes, this wouldn't require a major from us according to semver.
As our only supported installation method is, like nearly all PHP libraries, through Composer, the version bump won't break users environments as Composer will simply block them from upgrading.
In general, dropping PHP support isn't considered a breaking change in the PHP community. Here's how Doctrine puts it;
One question we frequently hear is, "isn't dropping support for a PHP version a BC break"? In a nutshell, no. A BC break happens when there is an incompatible change that your package manager can't handle. For example, changing a method signature in a minor version is a no-go, since the composer version constraints mentioned above assume any minor upgrade can safely be used.
However, when we drop support for an older version of PHP, composer will not consider the new version if the PHP version requirement is no longer fulfilled. Thus, you won't end up with a fatal error due to a wrong method signature, you just won't get the new version.
There was a problem hiding this comment.
I'd treat any requirement change as a breaking change that would require a major release if now the conditions are more restrictive than before.
As our only supported installation method is, like nearly all PHP libraries, through Composer, the version bump won't break users environments as Composer will simply block them from upgrading.
In this particular case, if you can ensure Composer will prevent users from installing this (now incompatible for older PHP setups) new version dependency, then that and the EOL of the previous version is enough to accept this change being released as a minor release.
| $this->assertContains( 'per_page=5', $api->getHistoryQuery() ); | ||
|
|
||
| $query = $api->getHistoryQuery(); | ||
| $this->assertStringContainsString( 'page=0', $query ); |
There was a problem hiding this comment.
As a general comment for all these query string assertions. If you match against page=5 it will also pass, because of the line below "per_page=5". I think it's better to test these individually, parsing the query and extracting each key/val into a dictionary/map structure first. No need to do that now unless that's a quick fix, but would like to see it tracked.
There was a problem hiding this comment.
That's a good suggestion, but I'd like to avoid getting refactoring tests in this PR. I was just aiming to get these working with minimum viable changes for the purposes of getting PHP 8 support shipped right away. I'll get a ticket going to improve this though, thank you.
| @@ -48,6 +49,23 @@ public function getMock(array $args = []) | |||
| return $mockStore; | |||
| } | |||
|
|
|||
| /** | |||
| * Gain access to PHPUnit's mock invocation stack for analyzing calls. | |||
| * PHPUnit 8.4 removed the native getInvocations property, requiring this workaround. | |||
There was a problem hiding this comment.
what is the official phpunit alternative to this, now removed, method?
There was a problem hiding this comment.
This test was created around a private API (getInvocations) in PHPUnit, which has since been revoked. Since it was a private API, no direct alternatives were provided. With some effort this test can be rewritten to use public APIs like expects() and exactly(), but rewriting tests felt outside the scope of introducing PHP 8 support so I went this route to accommodate us in the meantime.
Now that we're moving to a modern version of PHPUnit, this will allow us to pursue a goal I've had for awhile: a complete review and restructuring of our test suite, and the introduction of tools like Infection, PHPStan and PHPBench to give us a much more robust testing platform. This cookie test is probably my least favorite part of our current setup, so that will receive a major rework in those efforts.
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Description
This branch includes work necessary for the SDK to work with the new release of PHP 8. Most of this PR involves making accommodations for updated dependencies, as the library's own code is compatible with PHP 8.
Note: Our CircleCI tests will fail for this PR as we are testing against PHP versions that this PR drops support for.PHP 7.1 and 7.2 CI tests have been removed to make way for these changes.Testing these changes locally
CircleCI is already configured to run our standard tests using PHP 8.0 for this PR; you can review the results in the PR timeline. If you'd like to test these changes locally, you should: