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 https://github.com/sebastianbergmann/comparator/issues/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.

[jorgsowa]

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

That's true, but these changes moved towards the opposite extreme. It's almost impossible now to assert complex objects with dynamically created closures.

Available alternatives/workarounds:

1. Fix the version of PHPUnit to 12.1.x. Next versions require sebastian/comparator >= 7.1
2. Modify tests (only whenever possible - for my case, it's not possible). However, for the given example (https://github.com/webmozarts/console-parallelization/pull/342/files), we can see how unnecessary work it gave some of us.
3. Use external assertions libraries (like https://github.com/eliashaeussler/deep-closure-comparator). Not always feasible, as the comparison method in this case relies on serialization, which is not a perfect method of closure comparison.

Would it be a solution to create a pragmatic workaround that ignores closures until a good library is developed? Something ugly like assertEqualsIgnoringClosures(), or assertEquals(..., $options)? The taken approach practically blocks the update to PHPUnit 12 for my project for the foreseeable future.

[yurii-stickee-2023]

My subjective notes on the current change - this is VERY strange decision to drop it like that.

I hope you realize that PHP objects in most cases have closures, and a lot of objects are created with the same base closures all the time. With this change you basically made assertEquals() comparison for many objects useless, while assertSame() is not possible in most cases, because singleton in codebase has limited usage.

What way would you suggest to revert behavior for assertEquals() in 12.x to be as in 11.x in my custom setup? Use 3rd party package like DeepClosureAssert?

        $callback1 = fn () => 123;
        $callback2 = fn () => 123;

       // $this->assertEquals($callback1, $callback2); // <---- FAILS
        \EliasHaeussler\DeepClosureComparator\DeepClosureAssert::assertEquals($callback1, $callback2); // SUGGESTED SOLUTION?

Why cannot you add this DeepClosureAssert to PHP 12.x to have backwards compatibility in some form, like $this->assertLooselyEquals()? The package works (I have tested it) but it sounds very strange, as I compare objects with closures, not just closures.
Also it does not have custom message.

So far the only way I see is to port that package and add custom method to my base TestCase which is basically a workaround, something like this https://github.com/yurii-stickee-2023/bugged-phpunit12-closure-not-same2/commit/e4facf1ad65145143f5b81e53109fa6c2c65ab51

i do not understand why it cannot have native compatible solution for previous behavior.

Also, after reading https://github.com/sebastianbergmann/comparator/issues/128 I still do not understand what fake closure means, and how it affects equality of such closures

regards

-- p.s. some other idea may be to revert the change and add new method like ->assertEqualsWithPhp() that will contain new behavior. Subjectively, this looks better.

[DavideBicego]

This is blocking the upgrade to phpunit 12 for us. I understand that the issue is the previous comparison not being reliable but the way the comparison is now performed is for sure not the answer to the problem. Is someone working on this?

[Ocramius]

Data point here: in assertSame(), it would be acceptable to compare closures by their ID, but assertEquals() cannot do per-id comparison.

The only way I can see this working is by looking at the serialized comparison of closures, effectively using by-val comparison of closures.

EDIT: made this example in a call today:

$id = 123;

$closure1 = fn ($a) => $a === $id;

$id = 123;

$closure2 = fn ($a) => $a === $id; // same as $closure1

$id = 456;

$closure3 = fn ($a) => $a === $id; // not same as $closure1