Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix GH-9296: incorrect ksort(..., SORT_REGULAR) behaviour on arrays with numeric and string keys #9293

Closed
wants to merge 4 commits into from

Conversation

DrDet
Copy link
Contributor

@DrDet DrDet commented Aug 10, 2022

The comparator function used at ksort or krsort in SORT_REGULAR mode need to be consistent with basic comparison rules.
These rules were changed in PHP-8.0 for numeric strings, but comparator used at ksort mistakenly kept the old behaviour.
It leads to inconsistent situations, when after ksort the first key is GREATER than some of the next ones by according to the basic comparison operators.

Simple example:

<?php

$array = ["600" => 0, "aaa" => 1];
ksort($array, SORT_REGULAR);
var_dump($array);
var_dump(array_key_first($array) <=> array_key_last($array));

In php7.4 we get:

array(2) {
  ["aaa"]=>
  int(1)
  [600]=>
  int(0)
}
int(-1)

That's OK, because it's consistent with php7.4 comparison rules.

In php8.0 the output is:

array(2) {
  ["aaa"]=>
  int(1)
  [600]=>
  int(0)
}
int(1)

Which is not OK, because it is not consistent with new php8.0 comparison rules.

After this fix the output is:

array(2) {
  [600]=>
  int(0)
  ["aaa"]=>
  int(1)
}
int(-1)

Which is consistent with php8.0 comparison rules.

See #9296

Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh! I'm pretty confused that this apparently has not been reported before.

@DrDet DrDet force-pushed the dvaksman/fix-php-array-key-compare branch from 007484a to a12ec5e Compare August 10, 2022 22:58
@dstogov
Copy link
Member

dstogov commented Aug 11, 2022

I think this is a bug and it's better to report it at https://github.com/php/php-src/issues

I would prefer, to keep the old (optimized) comparison code at least for numeric+numeric and string+string keys, and fallback to zend_compare() only for rare numeric+string and string+numeric cases.

It would be also great to have a separate test case with a reference to the issue report.

@DrDet DrDet changed the title Fix ksort(..., SORT_REGULAR) behaviour on arrays with numeric and string keys Fix GH-9296: incorrect ksort(..., SORT_REGULAR) behaviour on arrays with numeric and string keys Aug 11, 2022
DrDet added 4 commits August 11, 2022 18:11
… numeric keys

The comparator function used at ksort in SORT_REGULAR mode
need to be consistent with basic comparison rules. These rules
were changed in PHP-8.0 for numeric strings, but comparator
used at ksort kept the old behaviour. It leads to inconsistent
situations, when after ksort the first key is GREATER than some
of the next ones by according to the basic comparison operators.
@DrDet DrDet force-pushed the dvaksman/fix-php-array-key-compare branch from a12ec5e to 23fc6d4 Compare August 11, 2022 15:55
@DrDet
Copy link
Contributor Author

DrDet commented Aug 11, 2022

I think this is a bug and it's better to report it at https://github.com/php/php-src/issues

Done with the bug report, see #9296

I would prefer, to keep the old (optimized) comparison code at least for numeric+numeric and string+string keys, and fallback to zend_compare() only for rare numeric+string and string+numeric cases.

Fixed at aa125c8

It would be also great to have a separate test case with a reference to the issue report.

Added at 23fc6d4

Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now this looks fine.

@cmb69 cmb69 closed this in cd1aed8 Aug 12, 2022
@cmb69
Copy link
Member

cmb69 commented Aug 12, 2022

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants