[go: up one dir, main page]

Page MenuHomePhabricator

PHP 8.2 failure for ApiResultTest::testTransformations for different order of the result
Closed, ResolvedPublic

Description

Running phpunit with php8.2 gives a different order of the array.

No idea what can cause that.

1) ApiResultTest::testTransformations with data set #8 ('Types: Normal transform', array(array('a', 'b', 'c'), array('a', 'b', 'c'), array('a', 'b', 'c'), array('a', 'b', 'c', 'array'), array('a', 'b', 'c', 'BCarray'), array('a', 'b', 'c', 'BCassoc'), array('a', 'b', 'c', 'assoc'), array('a', 'b', array('c'), 'kvp'), array('a', 'b', 'BCkvp', 'key'), array('a', array('b'), array('d'), 'kvp', true), array(1), array(1, 'assoc'), 1, array('_dummy')), array(array()), array(array('b', 'c', 'a', 'array'), array('a', 'b', 'c', 'assoc'), array('a', 'b', 'c', 'assoc'), array('a', 'c', 'b', 'array'), array('a', 'c', 'b', 'array'), array('a', 'b', 'c', 'assoc'), array('a', 'b', 'c', 'assoc'), array('a', 'b', array('c', 'array'), 'assoc'), array('a', 'b', 'assoc', 'key'), array('a', array('b', 'array'), array('d', 'assoc'), 'assoc', true), array(1, 'array'), array(1, 'assoc'), 1, array('_dummy'), 'assoc'))
Types: Normal transform
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
     'defaultAssoc' => Array (...)
     'defaultAssoc2' => Array (...)
     'array' => Array (
-        0 => 'a'
-        1 => 'c'
-        2 => 'b'
+        0 => 'c'
+        1 => 'b'
+        2 => 'a'
         '_type' => 'array'
     )
     'BCarray' => Array (
-        0 => 'a'
-        1 => 'c'
-        2 => 'b'
+        0 => 'c'
+        1 => 'b'
+        2 => 'a'
         '_type' => 'array'
     )
     'BCassoc' => Array (...)

/workspace/src/tests/phpunit/includes/api/ApiResultTest.php:712

Event Timeline

Change 888319 had a related patch set uploaded (by TK-999; author: TK-999):

[mediawiki/core@master] ApiResult: Make array ordering consistent across PHP versions

https://gerrit.wikimedia.org/r/888319

Please make sure any solution works for both int-like and float-like numeric key values. It would be bad if 9.1 and "9.1" no longer sorted as larger than 9 and "9".

Also ensure that any solution works for non-numeric strings. It would also be bad if all non-numeric strings sorted the same as 0, 0.3, "0" and "0.3" so never use non-numeric strings in a numeric context for comparison (e.g., casting to a numeric type like int).

Here is more information on numeric strings:

Notice the changes in 8.0, including but not limited to how non-numeric strings in a numeric context are handled (E_WARNING and value 0 vs. TypeError).

I too am not entirely sure what all causes the difference between PHP version, however some key changes that definitely affect the outcome are:

Both of these seem to be implemented in PHP 8.0+.

Based on reading these, I would prefer an implementation that changes the sort order to adhere to PHP 8.0+, despite the change across MediaWiki versions. It seems like trying to implement pre-PHP 8.0 behavior in PHP 8.0+ would be considerably more complex than the reverse. Implementing PHP 8.0+ behavior in pre-PHP 8.0 also seems like it might be more future proof.

Changelog contains

Fixed bug GH-9296 (ksort behaves incorrectly on arrays with mixed keys).

https://github.com/php/php-src/issues/9296 wants to enforce the "saner" comparision linked by you also be done for SORT_REGULAR places.

https://php.watch/versions/8.2/ksort-SORT_REGULAR-order-changes

Changelog contains

Fixed bug GH-9296 (ksort behaves incorrectly on arrays with mixed keys).

https://github.com/php/php-src/issues/9296 wants to enforce the "saner" comparision linked by you also be done for SORT_REGULAR places.

https://php.watch/versions/8.2/ksort-SORT_REGULAR-order-changes

Yes, and the fix was delayed until 8.2:

Which is why we are not seeing this issue until now (PHP 8.2).

According to @TK-999 in a comment on his 888319 gerrit patch (see T326480#8607976):

[...]the sorting behavior seems to come from https://gerrit.wikimedia.org/r/c/mediawiki/core/+/182858, where it was introduced without much context. Neither the formatversion=2 docs nor the 1.25 release notes seem to set any expectations as to this implicit sorting behavior, nor does it seem to be documented for developers (apart from the test case at hand). So perhaps it wouldn't be the end of the world if the sorting method was to change.[...]

I am of the thinking that since the introduced order was not documented and barely tested, we not be concerned about maintaining the default (aka SORT_REGULAR) ksort behavior of pre-PHP 8.2. SORT_REGULAR is documented to adhere to the semantics of the non-strict comparison operators which along with is_numeric changed in 8.0, however, the sorting behavior erroneously does not actually adhere to this until 8.2.

We have a number of options:

  1. implement pre-PHP 8.2 ksort SORT_REGULAR semantics so that it works across PHP versions, including pre-PHP 8.0, PHP 8.0+ (where the operators and functions we might use to implement things changed) and PHP 8.2+ (where SORT_REGULAR actually changes)
  2. implement PHP 8.2+ ksort SORT_REGULAR semantics so that it works across PHP versions, including pre-PHP 8.0 and PHP 8.0+
  3. implement/use a different sort; perhaps one that has not changed across PHP versions, like SORT_NATURAL

The first would avoid a breaking change across Mediawiki versions but it would be complex to implement and I am not sure it is the right thing to do. The second would probably be less tricky to implement and is likely a better thing to do, however, we can probably go with something simpler like option three.

Option three should be quite simple to implement just needing to change ksort( $data ); to ksort( $data, SORT_NATURAL );. As far as I know this has not changed across PHP versions.

Of course options two or three imply an albeit minor breaking change across Mediawiki versions and the tests and documentation should be updated to reflect that.

Sorry about the delay in following up, I was busy with some other work.

You're right, I think changing the sort behavior is the way to go. It's also worth noting that this problem only seems to occur if a caller explicitly overrides the inferred type for an associative array containing mixed keys. This scenario may be rare.

The test is skipped under php8.2 since 1b9809b3, but should be fixed to get reenabled.

Change #888319 merged by jenkins-bot:

[mediawiki/core@master] ApiResult: Make array ordering consistent across PHP versions

https://gerrit.wikimedia.org/r/888319

Change #1017145 had a related patch set uploaded (by Reedy; author: TK-999):

[mediawiki/core@REL1_41] ApiResult: Make array ordering consistent across PHP versions

https://gerrit.wikimedia.org/r/1017145

Change #1017386 had a related patch set uploaded (by Reedy; author: TK-999):

[mediawiki/core@REL1_40] ApiResult: Make array ordering consistent across PHP versions

https://gerrit.wikimedia.org/r/1017386

Change #1017387 had a related patch set uploaded (by Reedy; author: TK-999):

[mediawiki/core@REL1_39] ApiResult: Make array ordering consistent across PHP versions

https://gerrit.wikimedia.org/r/1017387

Change #1017145 merged by jenkins-bot:

[mediawiki/core@REL1_41] ApiResult: Make array ordering consistent across PHP versions

https://gerrit.wikimedia.org/r/1017145

Change #1017387 merged by jenkins-bot:

[mediawiki/core@REL1_39] ApiResult: Make array ordering consistent across PHP versions

https://gerrit.wikimedia.org/r/1017387

Change #1017386 merged by jenkins-bot:

[mediawiki/core@REL1_40] ApiResult: Make array ordering consistent across PHP versions

https://gerrit.wikimedia.org/r/1017386