Fix PHPUnit comparator under PHPUnit 10

[Firehed]

Fixes #745

[Firehed]

I've discovered this change as-is doesn't actually run the tests under PHPUnit 10 as expected. There's also another breaking change in the ComparisonFailure that needs addressing.

[frederikbosch]

@Firehed Is it still a draft, or ready to merge?

[Firehed]

@frederikbosch It's still a draft for now; I haven't had a chance to revisit it yet. Currently the test suite still runs under PHPUnit 9 despite the widening of the versioning, so I'm not confident it covers all of the possible edge cases.

[Firehed]

@frederikbosch I finally got a chance to revisit this, and was able to make a couple of adjustments that indeed get PHPUnit 10 to install and run its test cases.

As you can see from the CI, there's a couple of red-flags in static analysis due to supporting the different constructor signatures of ComparisonFailure. There's also a giant pile of warnings in PHPUnit 10, though it seems they can be addressed out of band.

I'm not sure what the best way to move forward on that will be, since it raises a larger question around supported tooling versions. In my own projects that support multiple versions of PHP I tend to only run analysis on the highest supported versions. Here, ci/psalm is locked to PHP 8.0 in the standalone build, but in the Build matrix it'll still run on all versions.

Do you have any thoughts there? I think there are a handful of approaches that could be viable, but I'd like a second opinion before going too far.

[wjzijderveld]

Trying to revive this a bit I was also looking at what would needed to be done to get this over the line.

Trying to support a matrix of versions is one way to go, although it might be easier to drop support for phpunit 9 and with that support for php 8.0? Are there other current issues for which it would make sense to use the matrix approach? If it's only for the Comparator I'm honestly not sure it's worth the effort.

Besides that it's good to push people to upgrade their PHP versions and dependencies, I also believe that for a library like this it wouldn't be a big problem. People that really can't upgrade can keep using an older version without loosing a massive amount of functionality. And worse case new patch versions can be released for older versions in case of CVE kind of fixes come in.

[frederikbosch]

@wjzijderveld I am fine with dropping PHP 8.0. There is no support for it by PHP anymore. With that, I will add a message to our README that from now on we only support PHP-supported versions. For older versions, people can use older tags.

[frederikbosch]

But, I'd rather not drop PHPUnit 9 yet, it is still the most installed version. Is it impossible to create a comparator that supports both PHPUnit 9 and 10?

[wjzijderveld]

People relying on this comparator also can't upgrade to phpunit 10 at this moment :sweat_smile:

I'm not sure a single Comparator is possible, as it's not a runtime error. But 2 versions within a version constraint check could work I guess

[frederikbosch]

Ok, let's skip the version constraint check and only support PHPUnit 10 then.

[Firehed]

Hey folks - I've finally had a brief moment to revisit this. It's clear to me that trying to resolve all of the potential CI tooling issues is going to take more time than I currently have available. If anyone is able to take over from here and figure out how to land this, it would be extremely welcome!

Here's the executive summary:

• PHPUnit 10 + sebastian/comparitor:5 has a slight signature change. The actual fix to make this run is quite small - removing parent::__construct() and a false
• Trying to do this dynamically by reflection to support both versions is doable
• Such an implementation makes static analysis extremely unhappy
• Trying to solve that devolves into a series of wide-reaching updates

If we want to support only PHPUnit 10, there's a handful of changes to make:

• DataProvders become static: f5225e6
• PHPUnit's -v flag is gone and will error if you try to use it: 196a710
• PHPUnit's TestCase::setLocale() is deprecated: https://github.com/sebastianbergmann/phpunit/issues/5216
• A bunch of test workflows may need to get updated to PHP 8.1

I think these are all good changes to make, but trying to power through them all in this PR is, at best, going to result in a lot of confusion. There's also potential public API BC breaks to contend with, and that could impact versioning and branching strategies.

What I'd suggest as a rough strategy here (but feel free to take a different approach) is: 1) Drop PHP 8.0 support, and get all of the CI tooling updated to reflect the new minimum 2) Perform any forward-compatible updates for PHPUnit 10 (static dataproviders) while still on 9 3) Figure out the solve for the deprecated TestCase::setLocale() and apply it (which may be psalm-suppressing it for now) 4) Update to PHPUnit 10, including applying the original change from this PR. It should drop into place. TBD on whether it's the "bridged" approach (BC-safe) or a hard switch (BC break)

There may also be a path where both comparitor versions are supported through two different classes, but I expect this would lead to a lot of user confusion - not to mention it's unlikely to resolve the CI challenges.

I'm more than happy to chime in if people have additional thoughts, but for now I must leave it in the rest of your capable hands!

[frederikbosch]

@Firehed what if we do something like this?

<?php

namespace Money\PHPUnit;

if (str_starts_with(\PHPUnit\Runner\Version::id(), '10.')) {
    final class Comparator extends \SebastianBergmann\Comparator\Comparator {
    }
} else {
    final class Comparator extends \SebastianBergmann\Comparator\Comparator {
    }
}

It's not pretty, but I think it might be the best solution in this case. And it does work.

[Firehed]

@frederikbosch At runtime I think it will work - and it's certainly a nicer approach than the reflection-based one I'd originally attempted! (I'd suggest version_compare or similar so it should work on PHPUnit >=11 when that time comes)

I suspect that CI will still take issue (Psalm in particular). Worst case I suppose the whole file could get psalm-suppressed out - though without widening the phpunit range (and dealing with everything else that entails) I think the >= 10 path technically would no test coverage.

One other path - definitely a BC break! - would be to split the whole comparitor out into a sub-package, which itself would be conditional on the PHPUnit version. It mostly shifts the problem elsewhere, but would open up the possibility of having a pair of branches and corresponding tags that depend on the correct version of PHPUnit.

I don't particularly like that option, but it came to me while thinking about some additional enhancements that don't really fit in the main package (specifically, I've written a custom Doctrine type mapper for Currency on multiple projects, and having an officially-sanctioned one to pull in would be nice) that may make sense split out.

[wjzijderveld]

I might be able to spend some time on it this weekend, but no promises.

Would splitting it out actually be a BC break though? 🤔 As long as the dependency is declared in moneyphp it's still available, so as long as the namespace stays the same I don't see how it would break BC. (Disclaimer, I'm very tired, it's likely I'm missing something obvious 😂 ). I also wouldn't be a huge fan of splitting it out, but it would make it easier to manage the dependency matrix.

[frederikbosch]

If you rebase this PR against current master, the support for PHP 8.0 has been dropped. This should simplify upgrading to PHPUnit 10, and drop support for PHPUnit 9.

[frederikbosch]

Thanks for all your work and input. I have just merged #784.