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

Allow installation on PHP < 7.3 icw PHPUnit < 9 #48

Merged
merged 4 commits into from
Apr 20, 2021

Conversation

jrfnl
Copy link
Collaborator

@jrfnl jrfnl commented Apr 8, 2021

TL;DR:

This PR allows the package to be used with test suites which need to support multiple PHPUnit versions for PHP cross-version compatibility.

  • This PR will allow the package to be installed on PHP 5.4 - current in combination with PHPUnit 4.8.36/5.7.21 - current.
  • The polyfill provided with the package will only kick in when PHPUnit 9 is detected. On older PHPUnit versions, the package will fall through to using the PHPUnit native functionality.

Includes (availability) tests for the fall throughs.
Includes CI adjustments to safeguard this change.

Fixes #11

Note: naming is hard, so, I'm happy to adjust the filenames for the fallthrough functionality (or make other adjustments deemed necessary).

Commit details

Add custom autoloader/fall-through to PHPUnit native functionality

This commit:

  • Adds a custom autoloader which will either load the polyfills or will load "empty" replacements for the classes in this package which will fall through to using the PHPUnit native functionality for PHPUnit 4.x - 8.x.
  • Widens the version requirements for PHP and PHPUnit to allow installing of this package on PHP 5.4 - current in combination with PHPUnit 4.8.36/5.7.21 - current.
  • Updates the autoload section in the composer.json to load the custom autoloader instead of using PSR4 autoloading.
  • Makes a few select exceptions to the coding standards for the custom autoloader and the "fall through" files for CS rules which require a higher minimum PHP version.
    • strict_types is only available as of PHP 7.0.
    • scalar type declarations and return types are only available as of PHP 7.0, with void only being available as of PHP 7.1.
    • Importing functions and constants with use statements is only available as of PHP 5.6.
  • Updates the README with information about the fact that the package no longer restricts installation to PHPUnit 9.

Add availability tests for the fall-through functionality

... and run the tests against all PHP versions on which the package can be installed.

This commit:

  • Adds a test bootstrap.php file which allows for running the tests either via PHPUnit phar files or via a Composer installed version.
  • Adds two additional test classes which make sure that the assertArraySubset() functionality can be safely used and falls through to the PHPUnit native functionality for PHPUnit < 9.
  • Prevents the real unit tests from loading on PHP < 7.3 via an annotation in the phpunit.xml.dist file.
    If those files would be loaded on older PHP versions, the test run would break on the parse errors due to modern PHP syntax being used.
  • Prevents the real unit tests from running on PHPUnit < 9.
    These tests only need to be run on PHPUnit >= 9 as that is the only time when the polyfills will actually be used.
  • Makes a few select exceptions to the coding standards for the new test files for CS rules which require a higher minimum PHP version.

GH Actions: adjust the test workflow

... to:

  • Run the availability tests against all PHP versions on which the package can be installed with prefer-stable.
  • Run the availability tests as well as the real tests against lowest and stable for PHPUnit 9 on PHP 7.3 - 8.0.

The test selection is done automatically via the phpunit.xml.dist file as adjusted in the previous commit.

As for the running of the real tests against the lowest supported PHPUnit 9 version, this requires some tweaking to the workflow script.
As PHPUnit didn't always have an explicit requirement for PHP, running with prefer-lowest on PHP 7.3 - 8.0 with the current PHPUnit version constraints would install PHPUnit 4.8.
So, for the prefer-lowest runs, we need to make sure that lowest is correctly interpreted as "lowest" within the PHPUnit 9.x range.

Also:

  • As the coding standards package has a minimum PHP requirement of PHP 7.2, it would block the composer install for lower PHP versions to run the tests, so removing the package for this workflow.

🆕 Autoloader: add fall-back to PSR4 autoloading 🆕

This fallback will only allow loading additional files on PHPUnit >= 9.x and will throw an exception otherwise.

Includes dedicated test for the autoload exception.

This commit:
* Adds a custom autoloader which will either load the polyfills or will load "empty" replacements for the classes in this package which will fall through to using the PHPUnit native functionality for PHPUnit 4.x - 8.x.
* Widens the version requirements for PHP and PHPUnit to allow installing of this package on PHP 5.4 - current in combination with PHPUnit 4.8.36/5.7.21 - current.
* Updates the `autoload` section in the `composer.json` to load the custom autoloader instead of using PSR4 autoloading.
* Makes a few select exceptions to the coding standards for the custom autoloader and the "fall through" files for CS rules which require a higher minimum PHP version.
    - `strict_types` is only available as of PHP 7.0.
    - scalar type declarations and return types are only available as of PHP 7.0, with `void` only being available as of PHP 7.1.
    - Importing functions and constants with `use` statements is only available as of PHP 5.6.
* Updates the README with information about the fact that the package no longer restrict installation to PHPUnit 9.
... and run the tests against all PHP versions on which the package can be installed.

This commit:
* Adds a test `bootstrap.php` file which allows for running the tests either via PHPUnit phar files or via a Composer installed version.
* Adds two additional test classes which make sure that the `assertArraySubset()` functionality can be safely used and falls through to the PHPUnit native functionality for PHPUnit < 9.
* Prevents the _real_ unit tests from loading on PHP < 7.3 via an annotation in the `phpunit.xml.dist` file.
    If those files would be loaded on older PHP versions, the test run would break on the parse errors due to modern PHP syntax being used.
* Prevents the _real_ unit tests from running on PHPUnit < 9.
    These tests only need to be run on PHPUnit >= 9 as that is the only time when the polyfills will actually be used.
* Makes a few select exceptions to the coding standards for the new test files for CS rules which require a higher minimum PHP version.
    - `strict_types` is only available as of PHP 7.0.
    - The `void` return type is only available as of PHP 7.1.
... to:
* Run the availability tests against all PHP versions on which the package can be installed with `prefer-stable`.
* Run the availability tests as well as the _real_ tests against `lowest` and `stable` for PHPUnit 9 on PHP 7.3 - 8.0.

The test selection is done automatically via the `phpunit.xml.dist` file as adjusted in the previous commit.

As for the running of the _real_ tests against the `lowest` supported PHPUnit 9 version, this requires some tweaking to the workflow script.
As PHPUnit didn't always have an explicit requirement for PHP, running with `prefer-lowest` on PHP 7.3 - 8.0 with the current PHPUnit version constraints would install PHPUnit 4.8.
So, for the `prefer-lowest` runs, we need to make sure that `lowest` is correctly interpreted as "lowest" within the PHPUnit 9.x range.

Also:
* As the coding standards package has a minimum PHP requirement of PHP 7.2, it would block the `composer install` for lower PHP versions to run the tests, so removing the package for this workflow.
Copy link
Owner

@rdohms rdohms left a comment

Choose a reason for hiding this comment

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

Looks good, I just have one question for future:

What kind of impact, or how do you see the a future where we add new functions that are no longer like the previous ones, would we need to touch any of this, or would we just leave as is since it would only be targetted at PHPunit > 9 and PHP >= 8?

@jrfnl
Copy link
Collaborator Author

jrfnl commented Apr 11, 2021

What kind of impact, or how do you see the a future where we add new functions that are no longer like the previous ones, would we need to touch any of this, or would we just leave as is since it would only be targetted at PHPunit > 9 and PHP >= 8?

Hi Rafael, good question. There are two parts to the answer:

  1. I think if/when that happens, it should be made very, very clear in the README that the additional functionality is not part of the polyfill and only available for PHPUnit 9 and higher.
  2. If the additional functionality would lead to additional classes being added, the autoloader will need to be adjusted to allow for loading those classes, but only on PHPUnit 9 or higher. I imagine the simplest would be to add a default case which would check for PHPUnit 9 and would only load the classes in that case and I'd suggest throwing an exception if those classes would be called up in combination with PHPUnit < 9.
    For the tests associated with the new functionality, those can simply be added to the /tests/Unit directory, which will ensure they are only loaded on PHP 7.3+. The tests should have a @requires PHPUnit >= 9 annotation at class level though, to make sure they are only run on PHPUnit 9 or higher and not on PHPUnit 7 or 8 which can also run on PHP 7.3.

As far as I can see, that's would be all.

@jrfnl
Copy link
Collaborator Author

jrfnl commented Apr 11, 2021

P.S.: oh and if/when in doubt or you run into something with new functionality, feel free to ping me. I'll be happy to take a look. This is not meant as a drive-by and forget contribution.

@rdohms
Copy link
Owner

rdohms commented Apr 12, 2021

So the long term plan is to fix the shortfalls of these functions, which will change the API as we will need different methods for indexed/dictionary arrays.

I was thinking if we can have the autoloader fall back to regular PSR loading for PHPUnit > 9 scenarios it would be a lot simpler for the upkeep. Also, we can always start a new Major version that will drop all this and be PHPunit > 9 only since there is not gonna be much changing over the years in the polyfill area.

WDYT?

@jrfnl
Copy link
Collaborator Author

jrfnl commented Apr 12, 2021

[@jrfnl] I imagine the simplest would be to add a default case which would check for PHPUnit 9 and would only load the classes in that case and I'd suggest throwing an exception if those classes would be called up in combination with PHPUnit < 9.

[@rdohms] I was thinking if we can have the autoloader fall back to regular PSR loading for PHPUnit > 9 scenarios it would be a lot simpler for the upkeep.

Sounds like we're thinking along the same lines. Shall I just go ahead and add that to the autoloader now ? That way there will be one less thing to think about later.

This fallback will only allow loading additional files on PHPUnit >= 9.x and will throw an exception otherwise.

Includes dedicated test for the autoload exception.
@jrfnl
Copy link
Collaborator Author

jrfnl commented Apr 12, 2021

@rdohms Done. I've added the PSR-4 autoloading fallback in a separate commit, including a unit test verifying that an exception is thrown is any of those files would be requested in combination with PHPUnit < 9.x.

@rdohms
Copy link
Owner

rdohms commented Apr 12, 2021

@jrfnl thanks! That is great!

@jrfnl
Copy link
Collaborator Author

jrfnl commented Apr 20, 2021

@rdohms Anything I can do to move this forward ? Anything in particular this PR is waiting for ?

@rdohms
Copy link
Owner

rdohms commented Apr 20, 2021

Rafael having personal time to do releases after hitting merge :P

@jrfnl
Copy link
Collaborator Author

jrfnl commented Apr 20, 2021

Not sure about your process, but merging it in will give more people the chance to test it before a release 😀

Copy link
Owner

@rdohms rdohms left a comment

Choose a reason for hiding this comment

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

Thanks, was doing one last review of the last changes, managed to steal some time at the end of the day.

🚢 it!

@rdohms rdohms merged commit ab8c440 into rdohms:master Apr 20, 2021
@jrfnl jrfnl deleted the feature/allow-install-on-php-lt-7.3 branch April 20, 2021 20:10
@rdohms rdohms added this to the v0.3.0 milestone Apr 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support older versions of PHPUnit
2 participants