CI review for PHP 8.1/decisions requested

[phpdocumentor]

CI review for PHP 8.1/decisions requested

[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.