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

Replace PHPUnit 6+ compat code with Yoast PHPUnit polyfills package #446

Merged
merged 20 commits into from
Jun 21, 2021

Conversation

schlessera
Copy link
Member

This PR replaces the few pieces of PHPUnit 6+ compact code with a more strategic polyfill library (https://github.com/Yoast/PHPUnit-Polyfills).

The PR also includes changes to adapt the tests to use PHPUnit 9+ syntax as needed, which is then polyfilled down to lower PHPUnit versions.

@schlessera schlessera added this to the 1.7.1 milestone Feb 9, 2021
@schlessera schlessera requested a review from jrfnl February 9, 2021 11:48
@jrfnl
Copy link
Member

jrfnl commented Feb 9, 2021

While I fully support this change, I hadn't done so myself previously as it will prohibit the tests from being run on PHP < 5.5 (PHPUnit Polyfills has a minimum supported PHP version of PHP 5.5), while Requests officially still supports PHP 5.2, 5.3 and 5.4.

I'm totally behind dropping support for PHP < 5.6, but I think we should do that in a major, not a patch version.

With that in mind, shall we milestone this PR for 2.0.0 ?

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Alain, notwithstanding my other remark, I've gone through the PR anyhow to verify it.

Aside from some small remarks I've left in-line:

  • Missing: the RequestsTest_TestCase class.
    All tests now extend from it, but the file itself is not included in this PR, so the tests can't be run at the moment.
    I'm presuming that that file extends the TestCase from PHPUnit Polyfills to actually make the polyfills available ?
  • Missing: the PHPUnit cache file which is generated when using PHPUnit 8/9 should be added to the .gitignore file.

Other than that and not for this PR, but just as a reminder for later:

  • The RequestsTests_Encoding::bin2hex() looks like it isn't used ?
  • When PHP < 5.5 is officially dropped, I'd recommend namespacing the tests according to PSR4, removing the custom autoload_tests() function from the test bootstrap and adding an autoload-dev section to the composer.json file to handle the autoloading.
    (would also be nice for the main library, but would be a big breaking change, so that needs a different consideration)

composer.json Outdated Show resolved Hide resolved
tests/Cookies.php Outdated Show resolved Hide resolved
tests/Requests.php Outdated Show resolved Hide resolved
tests/Transport/Base.php Outdated Show resolved Hide resolved
tests/Transport/Base.php Outdated Show resolved Hide resolved
tests/Transport/Base.php Outdated Show resolved Hide resolved
@jrfnl jrfnl removed this from the 1.7.1 milestone Feb 9, 2021
@jrfnl jrfnl added this to the 2.0.0 milestone Mar 19, 2021
@jrfnl jrfnl mentioned this pull request Jun 12, 2021
@schlessera schlessera force-pushed the add/phpunit-polyfills-library branch from f8bce92 to 0351570 Compare June 18, 2021 06:33
@schlessera
Copy link
Member Author

Missing: the RequestsTest_TestCase class.

Where did that one go? Seems like I forgot to add it to the commits, because I'm pretty sure this never worked without it. :)

@schlessera schlessera changed the base branch from stable to develop June 18, 2021 06:57
@schlessera schlessera requested a review from jrfnl June 18, 2021 06:58
@jrfnl
Copy link
Member

jrfnl commented Jun 18, 2021

Where did that one go? Seems like I forgot to add it to the commits, because I'm pretty sure this never worked without it. :)

It was in the branch which extended off this one with the initial GH Actions setup. (and why that branch wasn't deleted yet)

@jrfnl
Copy link
Member

jrfnl commented Jun 18, 2021

I've added one significant (and one insignificant) commit which fixes a problem when running the tests against a PHPUnit phar file.

If the composer install had been run against a high PHP version, running the tests using a PHPUnit Phar file against low PHP versions would error out:

image

( runtests is a script which will run the tests against every PHP minor using a PHPUnit phar for the appropriate PHPUnit version)

The errors are:

"PHP 5.6.40"
                                                                                                                                                                                                    
Warning: Unsupported declare 'strict_types' in path/to/Requests/vendor/phpunit/phpunit/src/Framework/Assert/Functions.php on line 1                                              
                                                                                                                                                                                                    
Fatal error: Default value for parameters with a class type hint can only be NULL in path/to/Requests/vendor/phpunit/phpunit/src/Framework/Assert/Functions.php on line 89
                                                                                                                                                                                                    
"PHP 7.0.33"                                                                                                                                                                                        
                                                                                                                                                                                                    
Parse error: syntax error, unexpected '?', expecting variable (T_VARIABLE) in path/to/Requests/vendor/phpunit/phpunit/src/Framework/Assert/Functions.php on line 177

Note: when the composer install would have been run on a low PHP version, there would have been no issues.

All the same, by reversing commit 07b71f0 and adding a require for the PHPUnit Polyfills autoloader to the bootstrap, the tests can be run with a PHPUnit Phar file without problems independently of what PHP version was used to run the composer install.

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good now!

@schlessera I'll leave this open for the moment to give you a chance to review the additional commits I added and to test this version locally.
Happy for it to be merged though.

@schlessera schlessera merged commit c637258 into develop Jun 21, 2021
@schlessera schlessera deleted the add/phpunit-polyfills-library branch June 21, 2021 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants