CI review for PHP 8.1/decisions requested

[jrfnl]

Hi @jaapio,

As discussed, I've done a review of the CI setup with an eye on getting the tests running on PHP 8.1.

Aside from some obvious fixes which are needed, I also found a number of things for which there are multiple solution directions available and I would like to ask you for guidance as to what is your preferred solution.

I have a (passing) WIP branch prepared and will pull the various PRs once I receive your replies.

Psalm

At this moment, there is an inconsistency between the Psalm version for local dev use and the Psalm version used in CI.

For local dev, Psalm is installed via Phive and fixed at version 3.11.2 which yields no issues. An upgrade to version 3.18.2 (last 3.x release) would be needed due to the age of the version, but that is an update without consequence.

For CI, Psalm is installed via the tools option of the setup-php action without fixed version, which in practice means that the latest version of Psalm is used, which is version 4.6.1, which would yield one issue.

Options:

• Manage Psalm via Phive and change the CI workflow for Psalm to use the Phive installed version of Psalm.
• Use setup-php to install Psalm in CI, but fix the version being installed to the same version as installed via Phive.
  This would add a maintenance burden as the version numbers in phive.xml and in the push.yml workflow file would need to be manually kept in sync.
• Always use the latest version of Psalm, no changes needed in CI, one fix needed in the code base, update of Phive version for Psalm needed.
  Note: I'm not sure how to ensure that Phive will always install the latest version and haven't been able to find information on the XML schema to figure out if I can just remove the installed key or set it to latest or something.

Additional question:

• Do you want to upgrade from Psalm 3.x to Psalm 4.x ? (option 3 already implies this)

Test workflow

In the phpunit job, the actual running of the tests has a continue-on-error directive. This means that even when a test build fails, the workflow will continue and not be marked as "failed", nor will email notification about the failed build be send out.
Individual builds will still be marked as failed.

I suspect this may have been set-up this way to make sure that all matrix variations of test builds will actually be run ?

I'm proposing to change this now by:

• Removing the continue-on-error for the test run step.
• Adding the fail-fast key and setting it to false.
  By default this key is set to true, which means that if any individual build within the job fails, all other builds within the job will be cancelled.
  By setting it to false, all builds in the matrix will still be run, but if any of them fail, the workflow will be marked as "failed".

PHPUnit vs PHP 8.1

The latest release of the PHPUnit 9.5 Phar still contains an incompatibility with PHP 8.1-beta1 (and higher).
Using the PHPUnit Phar will therefore fail the build, even though the tests do actually pass on PHP 8.1.

The next Phar release of PHPUnit (9.5.8) will contain a fix for the above mentioned issue.

Options:

• For the time being, allow the test builds against PHP 8.1 to fail.
• For the time being, use a Composer installed version of PHPUnit for the test run against PHP 8.1.

General

In various jobs in the workflow, steps are included to cache the Composer downloads directory and run composer install. However, this package has no dependencies, so these steps are redundant.

Options:

• Remove these steps and replace them with a single step to run composer dumpautoload.
• Replace these steps with the ramsey/composer-install action to clean up the script and maintain the existing behaviour.
• Leave as-is.

Please let me know what action you'd prefer for each of these issues.

[jaapio]

Psalm

For phpdocumentor/typeresolver I recently switched to a composer installed psalm. "psalm/phar": "^4.8" which would be easier to use in ci and local. Phive is a great tool but it still has it's quirks. I think they are moving slowly towards a composer based install method. Since psalm is releasing this special package for this, let's use that.

And upgrade to the latest psalm version would be great.

Testworkflow

continue-on-error was introduced to be able to release for php8. I'm not sure if we accually need it right now. Not allowing it to fail is kind of conflicting with the missing php 8.1 support in phpunit at the moment :-).

phpunit

Since this package is part of the set of libraries used by phpunit, I think we have to allow failures on php 8.1 for now. We did the same think for php 8. Ignore the failures to allow phpunit to proceed in their workflow. And create a new release if bugfixes are needed. This library is so small that I just took that risk :-).

General

I didn't know ramsey/composer-install but it looks good to me. Since I would like to introduce psalm as a dev dependency composer is required again :-) I think I would like to keep all steps equal. Using composer cache to prevent too much downloads.

If it helps, I think it is Ok if we would drop php 7.2 from this lib. This library nearly gets any patches. It would allow you to upgrade to phpunit 9?

Thanks for all this input!

[jrfnl]

Psalm

For phpdocumentor/typeresolver I recently switched to a composer installed psalm. "psalm/phar": "^4.8" .... Since psalm is releasing this special package for this, let's use that.

And upgrade to the latest psalm version would be great.

Done - see PR #42

Testworkflow

continue-on-error was introduced to be able to release for php8.

In that case, I think it was applied incorrectly.

As things stand, ALL test builds would be allowed to continue-on-error, not just the PHP 8.0 builds (for which it should be removed by now anyway).

I'm going to suggest removing it for now. With optionally adding fail-fast: false instead. See PR #43

When the build against PHP 8.1 gets added, I will revisit this.

phpunit

Since this package is part of the set of libraries used by phpunit, I think we have to allow failures on php 8.1 for now. We did the same think for php 8. Ignore the failures to allow phpunit to proceed in their workflow. And create a new release if bugfixes are needed. This library is so small that I just took that risk :-).

I'm not sure I understand or rather, this doesn't seem to answer my question.

The tests for this package pass on PHP 8.1 when the tests are run via Composer or via bleeding-edge dev-9.x Phar of PHPUnit, so there isn't really any reason to allow the tests to fail, other than the (upstream) issue with the PHPUnit 9.5.7 Phar.

(Temporarily) installing PHPUnit via Composer for PHP 8.1 in CI would bypass that issue.

Alternatively, waiting a few more days/weeks for the PHPUnit 9.5.8 Phar to be released would also allow the test runs to pass.

As a last resort, I can configure the workflow to allow failing test runs for PHP 8.1 for the time being (but only for PHP 8.1, in contrast to how this was done before).

UPDATE: PHPUnit 9.5.8 has been released, so the Phar issue no longer exists.

General

I didn't know ramsey/composer-install but it looks good to me. Since I would like to introduce psalm as a dev dependency composer is required again :-) I think I would like to keep all steps equal. Using composer cache to prevent too much downloads.

Done. See PR #39

If it helps, I think it is Ok if we would drop php 7.2 from this lib. This library nearly gets any patches. It would allow you to upgrade to phpunit 9?

The library already has a minimum PHP requirement of PHP 7.3 and already uses PHPUnit 9.x.
This was done in PR #33 in response to issue #32 / commit fef7edb in October 2020.

The only question now still open is about the test runs. Other than that, I have lined up a number of PRs which should be good to go and all contribute to getting the CI to pass again.

[jaapio]

(Temporarily) installing PHPUnit via Composer for PHP 8.1 in CI would bypass that issue.

I'm not 100% sure if this will work... php will autoload the classes in a certain order, because this library is part of phpunit's dependencies. It might happen that this library version will conflict with phpunit constraints. We had this in the past. Using composer installed phpunit only works on the master branch or branches which are aliased in our composer.json.

Alternatively, waiting a few more days/weeks for the PHPUnit 9.5.8 Phar to be released would also allow the test runs to pass.

I think we have an issue here... this library is a dependency of prophecy, which is a dependency of phpunit. This library must support 8.1 in a tagged version to allow phpunit to release a phar for 8.1 :-)

As a last resort, I can configure the workflow to allow failing test runs for PHP 8.1 for the time being (but only for PHP 8.1, in contrast to how this was done before).

However, it sounds bad. I think the last resort is the best solution. We changed nothing since the last release, so it would be kind of safe to do this. So please allow php 8.1 to fail for now, once there is a official supported version of phpunit on php 8.1 we can remove the check and be strict again.

[jrfnl]

I'm not 100% sure if this will work... php will autoload the classes in a certain order, because this library is part of phpunit's dependencies. It might happen that this library version will conflict with phpunit constraints. We had this in the past. Using composer installed phpunit only works on the master branch or branches which are aliased in our composer.json.

Well, I tested it and it did work (with some tweaks for the circular dependency - basically you need to set a COMPOSER_ROOT_VERSION env variable with a version alias to get it working), but as PHPUnit 9.5.8 has been released, we can just use that version as a Phar.

I think we have an issue here... this library is a dependency of prophecy, which is a dependency of phpunit. This library must support 8.1 in a tagged version to allow phpunit to release a phar for 8.1 :-)

However, it sounds bad. I think the last resort is the best solution. We changed nothing since the last release, so it would be kind of safe to do this. So please allow php 8.1 to fail for now, once there is a official supported version of phpunit on php 8.1 we can remove the check and be strict again.

While PHPUnit 9.5.8 may not 100% officially support PHP 8.1 yet due to Prophecy, as you're not using Prophecy in your test suite, the tests will pass without problem, so no need.