search

phpDocumentor / ReflectionDocBlock

MIT License
9.35k stars 119 forks source link

Fixed PHP 8 #242

Closed GrahamCampbell closed 4 years ago

GrahamCampbell commented 4 years ago

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

mvriel commented 4 years ago

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?

GrahamCampbell commented 4 years ago

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.

GrahamCampbell commented 4 years ago

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+

GrahamCampbell commented 4 years ago

NB I think dropping PHP 7.2 would be silly.

mvriel commented 4 years ago

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.

GrahamCampbell commented 4 years ago

As I have trouble understanding what you mean

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:

  1. Delete the lock file.
  2. Ignore platform reqs on PHP 8.
  3. Drop either PHP 7.2 or 8.0.
GrahamCampbell commented 4 years ago

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.

mvriel commented 4 years ago

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?

GrahamCampbell commented 4 years ago

Oh, yes. PHPUnit is not installed using composer. Maybe there is some hope, then, actually. I'll see what I can do.

GrahamCampbell commented 4 years ago

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 webmozart/assert.

GrahamCampbell commented 4 years ago

I can fool composer into resolving these for the purposes of the lock file, until the real deal is tagged.

GrahamCampbell commented 4 years ago

Done! Let's see what happens on GitHub Actions now...

mvriel commented 4 years ago

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?