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

ksort(..., SORT_REGULAR) behaves incorrectly on arrays with string and numeric keys since PHP 8.0 #9296

Closed
astrophysik opened this issue Aug 11, 2022 · 5 comments

Comments

@astrophysik
Copy link

Description

The behavior of the string and number comparison operators has been changed since php 8, but ksort and krsort functions still use the old rules.
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.

The following code:

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

Resulted in this output:

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

But I expected this output instead:

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

PHP Version

PHP 8.0

Operating System

No response

DrDet added a commit to DrDet/php-src that referenced this issue Aug 11, 2022
… 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 added a commit to DrDet/php-src that referenced this issue Aug 11, 2022
cmb69 added a commit that referenced this issue Aug 12, 2022
* PHP-8.0:
  Fix GH-9296: `ksort` behaves incorrectly on arrays with mixed keys
@cmb69 cmb69 closed this as completed in cd1aed8 Aug 12, 2022
cmb69 added a commit that referenced this issue Aug 12, 2022
* PHP-8.1:
  Fix GH-9296: `ksort` behaves incorrectly on arrays with mixed keys
@zeriyoshi
Copy link
Contributor

@cmb69
Going to fix this as a bug? This fix would make the behavior more intuitive, but it would break code that depends on the current behavior of ksort. (I have run into that).

Personally, I think this should be fixed in PHP 8.2 or later and marked as BC Break.

@damianwadley
Copy link
Member

The fix for this bug is already in commit cd1aed8 and will be present in PHPs 8.0.23, 8.1.10, and 8.2.0.

@zeriyoshi
Copy link
Contributor

Yes, I understand that.

However, I think you should reconsider whether this is appropriate. Fortunately, none of the versions have been released yet.

@damianwadley
Copy link
Member

Ah, I thought you were asking about whether it was going to be fixed at all.

Every bug fix breaks compatibility for someone. Personally, I don't think everybody should have to upgrade all the way to 8.2 to get this one just because there might be some people who have discovered and are making deliberate use of what is IMO objectively illogical behavior on PHP's part.

@zeriyoshi
Copy link
Contributor

I agree with your opinion, but I don't think this change should be made in a patch.
Patch versions are primarily for bug fixes for security and crashes, and I don't think changes that affect compatibility are appropriate.
On the other hand, I know that in PHP such changes have been incorporated into patch versions in the past.

This is a decision-making issue. Either way, I would like to get confirmation, so I will send an email to the ML. Thanks.

cmb69 added a commit that referenced this issue Aug 29, 2022
cmb69 added a commit that referenced this issue Aug 29, 2022
* PHP-8.0:
  Revert "Fix GH-9296: `ksort` behaves incorrectly on arrays with mixed keys"
cmb69 added a commit that referenced this issue Aug 29, 2022
* PHP-8.1:
  Revert "Fix GH-9296: `ksort` behaves incorrectly on arrays with mixed keys"
sgolemon pushed a commit that referenced this issue Aug 29, 2022
ramsey added a commit that referenced this issue Aug 30, 2022
ramsey added a commit that referenced this issue Aug 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants