Open kristiaan-factorial opened 3 weeks ago
Seems to be fallout from the fix for this issue: https://github.com/sebastianbergmann/phpunit/issues/5019
The core issue seems to be a change to fix https://github.com/sebastianbergmann/comparator/issues/112 in release https://github.com/sebastianbergmann/comparator/releases/tag/5.0.2 Wether you agree or not with the fix, it also breaks when comparing numeric keys:
public function test_comparator(): void
{
$this->assertEqualsCanonicalizing(
[
0 => 1,
1 => 2,
2 => 3,
],[
0 => 1,
1 => 2,
3 => 3,
]
);
}
will fail with:
Failed asserting that two arrays are equal.
Array (
0 => 1
1 => 2
- 2 => 3
+ 3 => 3
)
Which is a breaking change. It kind of defeat the purpose of the Canonicalizing
@olivier-zenchef
Look at definition of assertEqualsCanonicalizing()
: https://docs.phpunit.de/en/10.5/assertions.html#assertequalscanonicalizing
We see there: For instance, when the two variables $expected and $actual are arrays, then these arrays are sorted before they are compared.
The main question is: what is an "array"?
"PHP array" is not an "array" known from other languages.
"PHP array" is an "array" only if its keys consist of consecutive numbers from 0
to count($array)-1
.
"PHP array" with non-number keys or numbers keys but not consecutive is a structure known as "HashMap".
Unfortunately, PHP developers decided to name "HashMap" as "array" and "array" as "list" - see: https://www.php.net/manual/en/function.array-is-list.php
In your example actual
value is [0 => 1, 1 => 2, 3 => 3,]
, so it is HashMap (not an array) and should not be sorted before comparison with expected
.
Of course @sebastianbergmann may change assertEqualsCanonicalizing()
definition to something like this:
For instance, when the two variables $expected and $actual are arrays with number keys (consecutive or not), then these arrays are sorted before they are compared.
Then [0 => 1, 1 => 2, 2 => 3,]
and [0 => 1, 1 => 2, 3 => 3,]
are equal in canonicalized form.
@mgleska one usage of assertEqualsCanonicalizing($array1, $array2)
is to compare 2 non-associative arrays in your test.
It's a shortcut to avoid manual sort + array_values
My point is that it's a breaking change because a key behaviour changed without a major step in version.
Our tests are roughly:
$expectation = [1, 2, 3];
$startingData = [1, 2, 3, 4, 5];
// Will produce [0 => 1, 1 => 2, 3 => 3] because possibility 2 was culled
$result = $myService->computeSomethingByCullingAnArray($startingData);
// Will now fail
$this->assertEqualsCanonicalizing($result, $expected);
During the computeSomethingByCullingAnArray()
, there are some manipulations that unset some keys.
Those array are massive, hence why we don't copy them and rather cull them little by little.
During the whole process, we only manipulate non-associative array so we do not care for keys.
Now we want to update our phpunit version but it breaks several key tests about our key calculations.
In both cases this is not what we expected when upgrading a minor version.
To make it clear, there are two points being argued for here:
We can still discuss what to do about point 2 and whether we want to revert the behavior to the old one for numerical arrays, even if their keys are out of order. But point 1 should not be up for discussion as that's simple fact.
For the record and google searchers: Temporary fix is to block phpunit/phpunit version to 10.5.29 and also the faulty sub-lib sebastian/comparator to 5.0.1:
composer require phpunit/phpunit:10.5.29 --dev
composer require sebastian/comparator:5.0.1 --dev
You have to do both as phpunit require comparator from 5.0.2 starting tag 10.5.30.
Summary
Before, assertEqualsCanonicalizing would succeed on the following code:
This was great when we were, for instance, comparing a list of permission names in Drupal. We don't care which numerical keys they have, we only care about all of the names being there. Now, it chokes on the numerical keys being different, when they don't neatly increment one after another.
Current behavior
How to reproduce
Try the code above
Expected behavior
That my tests would keep going green like they used to, rather than fail after updating phpunit and not seeing anything about this in the release notes :)