Fixed PHP 8 #242
Fixed PHP 8 #242
Conversation
|
Rebased this. This avoids the messy nesting from the other PR, and enables testing on PHP 8 properly. The lock file enables safe ignoring of platform reqs, however the lockfile will definitely prove to be a curse in the long run when it will turn out that the dependencies that support PHP 8 in the future don't also support PHP 7.2... |
|
I'm not comfortable with the ignoring of platform requirements; this will hide issues from us with libraries that are incompatible with newer versions of PHP. If our dependencies are not usable; this library is not usable. Right? |
|
I could change the test config to only ignore on PHP 8, if that helps? It's only the dev requirements that are not compatible with PHP 8. |
|
And while you insist on having a lock file, we'd never be able to run the tests on PHP 8 (without ignoring platform reqs), without dropping support for PHP 7.2 and committing a lock file with dependencies that require PHP 7.3+ |
|
NB I think dropping PHP 7.2 would be silly. |
|
I will review your comments later as I am now at work. I would need to verify what you are saying and why as the goal of the lock file is to describe a stable set of requirements that we can guarantee proper operation with. If the only way to get this to work is to use ignore-platform-reqs; it feels to me that we cannot support PHP 8 without changing dependencies. And possibly doing a BC break that way. As I have trouble understanding what you mean, I need to spend more time to check the setup and lock file to see what is wrong. |
PHPUnit 9.3 will be the first version to support PHP 8.0. It does not support PHP 7.2. So either we have a lock file which is broken on PHP 7.2, or is broken on PHP 8.0. There are three possible solutions:
|
|
Note that option 1 will only work once PHPUnit 9.3 is released, and all its dependencies mark themselves as PHP 8 compatible. This PR implements option 2, which I think is the best idea for now. |
|
As I said, I will look into this later. We don't install PHPUnit using composer but with Phive and PHPUnit shouldn't care whether we support PHP 7.2 as long as we support PHP 8. So I still don't get what exactly is the point. What problem is it solving? Is it an error because a dependency does not update or do we use a dependency that is not compatible with 8? Are we unable to test or is it simply impossible to use a new PHPUnit version because we want to test our code in 7.2? |
|
Oh, yes. PHPUnit is not installed using composer. Maybe there is some hope, then, actually. I'll see what I can do. |
|
I think we actually can have our cake, and eat it, as it were, as long as we are happy with a dev version of |
|
I can fool composer into resolving these for the purposes of the lock file, until the real deal is tagged. |
|
Done! Let's see what happens on GitHub Actions now... |
|
Hey Graham, I have been checking this and the reverts you did and the subsequent implementation is not needed for the PHP 8 upgrade. As far as I can see you implemented the exact same behaviour (correct me if I am wrong). As a contributor courtesy, we have a rule not to revert commits for a refactoring. But instead to build on that code so that it is clear for the contributor what was changed, and I believe reverting gives the wrong signal (your code is bad); a signal that I never want to give. Also: I am quite confused by the dev-master reference in the composer.lock and the 1.9 reference for the Assertion library in the .json. I definitely do not want to rely on a dev-master or non-stable dependency in the non-dev part of the composer.json. Did you edit the composer.lock file or built it with an alternate composer.json? Because this way, it still won't work in 8.0 consumers since they will still try to use the 1.9 version that is not compatible with PHP 8.0. Can you reopen a new PR with just what is needed to get PHP 8 working, without reverting other people's commits and, if I interpreted this correctly, without hackery in the composer files? |
No description provided.