Revert support for closure comparison

[theofidry]

Not sure if it is better to reply in the closed issue #127 or to open a new one.

Whilst the promise of #127 is interesting, I think the current implementation falls too short to be practical. A very simple example: https://3v4l.org/1E5Fj

So if your code is using some noop closures in various places, all tests previously passing will now fail. Worse, the actual user experience for those is completely broken, this is the diff I get from https://github.com/webmozarts/console-parallelization:

10) Webmozarts\Console\Parallelization\Input\ParallelizationInputTest::test_it_can_be_instantiated_from_an_input@child option with data (Symfony\Component\Console\Input\StringInput Object (...), Webmozarts\Console\Parallelization\Input\ParallelizationInput Object (...))
Failed asserting that two objects are equal.
--- Expected
+++ Actual

I am not too sure if it is ArrayComparator swallowing the inner comparison or the string represendation being too generic not allowing the diff to be exposed.

I honestly have no clue what the proper fix would be, but I feel like this is too much breakage (IMO) to land in a minor/patch version*.

*: actually digging a tiny bit more, PHPUnit 12 allows sebastianbergmann/comparator ^7.0.0 so technically if one upgrades to PHPUnit 12 now they will get it as early as 12.0.0, but you could previously upgrade to 12.0 without hitting this issue, and still can if you constraint sebastian/comparator to ~7.0.0

[theofidry]

Digging a bit further into this, actually the inner exception thrown by ClosureComparator is:

Failed asserting that closure declared at /path/to/console-parallelization/src/ParallelExecutorFactory.php:114 is equal to closure declared at /path/to/console-parallelization/src/ParallelExecutorFactory.php:114.

The relevant code is:

return new self(
            // ...
            new SymfonyProcessLauncherFactory(
                new StandardSymfonyProcessFactory(),
            ),
            static fn () => usleep(self::CHILD_POLLING_IN_MICRO_SECONDS), // <- the problematic closure
        );

Because unless the closure is assigned once and re-use the comparison will fail: https://3v4l.org/CnC7D

[sebastianbergmann]

CC @tstarling @gbirke

[tstarling]

Not sure if it is better to reply in the closed issue #127 or to open a new one.

Whilst the promise of #127 is interesting, I think the current implementation falls too short to be practical. A very simple example: https://3v4l.org/1E5Fj

So if your code is using some noop closures in various places, all tests previously passing will now fail.

This was discussed at length at #128. It was working before because every closure compared equal to every other closure. I don't think that was acceptable. There's no point doing an assertion if it can't fail. Tests relying on that will have to be migrated.

Worse, the actual user experience for those is completely broken, this is the diff I get from https://github.com/webmozarts/console-parallelization:

10) Webmozarts\Console\Parallelization\Input\ParallelizationInputTest::test_it_can_be_instantiated_from_an_input@child option with data (Symfony\Component\Console\Input\StringInput Object (...), Webmozarts\Console\Parallelization\Input\ParallelizationInput Object (...))
Failed asserting that two objects are equal.
--- Expected
+++ Actual

Fix proposed at #130.

I honestly have no clue what the proper fix would be, but I feel like this is too much breakage (IMO) to land in a minor/patch version*.

That's up to @sebastianbergmann. I don't need this urgently.

[tstarling]

One option for migrating tests is to use first class callable syntax, since closures generated that way are more loosely compared. For example https://3v4l.org/n0TOB

Another option is to use the library that Elias Häußler wrote for #128: https://github.com/eliashaeussler/deep-closure-comparator

[theofidry]

This was discussed at length at #128.

Thanks for the link, I missed that one.

One option for migrating tests is to use first class callable syntax

I could, in that project, migrate somewhat easily (see test: Fix the closure comparisons webmozarts/console-parallelization#342), but I admit I worry about this update in various projects. Maybe I'll have to resort to use a custom comparator.

I totally understand the rationale and desire behind having closures compared and I'm generally on board with the idea, I am less with the way this was introduced but maybe it's just a me problem.

[staabm]

This was discussed at length at #128. It was working before because every closure compared equal to every other closure. I don't think that was acceptable. There's no point doing an assertion if it can't fail. Tests relying on that will have to be migrated.

agree. tests comparing closures before did provide a false-sense of confidence/security.