# ksort breaking change - Thread: https://externals.io/message/118483 - Original archive: https://news-web.php.net/php.internals/118483 - Started by Go Kudo on 2022-08-26T03:15:44+00:00 - Messages: 12 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #118483 — Go Kudo — 2022-08-26T03:15:44+00:00 URL: https://externals.io/message/118483#118483 Hi Internals. In the actively supported version of PHP, `ksort()` has been modified to include BC Break. https://github.com/php/php-src/issues/9296 This may seem like an appropriate bug fix, but it is a clear BC Break. I think this change should only be introduced in PHP 8.2 and later. Fortunately, there is not yet a release in each version that incorporates this change. I think it is possible to revert now. What do you think? Best regards, Go Kudo --- ## #118507 — Christoph M. Becker — 2022-08-26T12:19:40+00:00 URL: https://externals.io/message/118483#118507 · In reply to: #118483 > In the actively supported version of PHP, `ksort()` has been modified to > include BC Break. > > https://github.com/php/php-src/issues/9296 > > This may seem like an appropriate bug fix, but it is a clear BC Break. I > think this change should only be introduced in PHP 8.2 and later. In this case, the functions didn't behave as documented, namely to conform to the general conversion rules, which had a relevant change in PHP 8.0. Apparently, this case has been overlooked when the change had been implemented, and only been noticed recently (what still surprises me). Anyway, fixing the issue now is not really introducing a BC break, since code relying on the previous behavior did not conform to the documentation. > Fortunately, there is not yet a release in each version that incorporates > this change. I think it is possible to revert now. Well, the fix is part of the currents RCs; that doesn't make it impossible to revert, but RMs should have a say in that. Thus I'm adding Sara and Gabriel as recepients. -- Christoph M. Becker --- ## #118509 — Christian Schneider — 2022-08-26T13:01:07+00:00 URL: https://externals.io/message/118483#118509 · In reply to: #118507 Am 26.08.2022 um 14:19 schrieb Christoph M. Becker : > > > > […] > > In this case, the functions didn't behave as documented, namely to > conform to the general conversion rules, which had a relevant change in > PHP 8.0. Apparently, this case has been overlooked when the change had > been implemented, and only been noticed recently (what still surprises > me). Anyway, fixing the issue now is not really introducing a BC break, > since code relying on the previous behavior did not conform to the > documentation. I don't really agree with your definition of BC break. The behavior of the function with a mix of numeric/non-numeric string keys changes from 8.1.9 to 8.1.10 which in my world qualifies as a BC break. Regards, - Chris --- ## #118510 — juan carlos morales — 2022-08-26T14:15:00+00:00 URL: https://externals.io/message/118483#118510 · In reply to: #118509 Seems like we Don't Have tests for the function or we should enhance the existing ones. If We would Have proper tests the FIX would break it (as I Don't see tests in the fix itself).....That is why I say so. I Am not in my computer atm. I Will check this later El vie., 26 de agosto de 2022 15:01, Christian Schneider < cschneid@cschneid.com> escribió: > Am 26.08.2022 um 14:19 schrieb Christoph M. Becker : > > […] > > > I don't really agree with your definition of BC break. > > The behavior of the function with a mix of numeric/non-numeric string keys > changes from 8.1.9 to 8.1.10 which in my world qualifies as a BC break. > > Regards, > - Chris > > -- > > To unsubscribe, visit: https://www.php.net/unsub.php --- ## #118512 — tim@bastelstu.be — 2022-08-26T14:51:31+00:00 URL: https://externals.io/message/118483#118512 · In reply to: #118510 Hi > Seems like we Don't Have tests for the function or we should enhance the > existing ones. If We would Have proper tests the FIX would break it (as I > Don't see tests in the fix itself).....That is why I say so. > Various tests were adjusted in: https://github.com/php/php-src/pull/9293/files Best regards Tim Düsterhus --- ## #118513 — juan carlos morales — 2022-08-26T14:59:20+00:00 URL: https://externals.io/message/118483#118513 · In reply to: #118512 Oh! My bad. Nice work! Cheers El vie., 26 de agosto de 2022 16:51, Tim Düsterhus escribió: > Hi > > > > […] > > Various tests were adjusted in: > https://github.com/php/php-src/pull/9293/files > > Best regards > Tim Düsterhus --- ## #118515 — Sara Golemon — 2022-08-26T16:28:11+00:00 URL: https://externals.io/message/118483#118515 · In reply to: #118507 On Fri, Aug 26, 2022 at 7:19 AM Christoph M. Becker wrote: > > > > > […] > > In this case, the functions didn't behave as documented, namely to > conform to the general conversion rules, which had a relevant change in > PHP 8.0. Apparently, this case has been overlooked when the change had > been implemented, and only been noticed recently (what still surprises > me). Anyway, fixing the issue now is not really introducing a BC break, > since code relying on the previous behavior did not conform to the > documentation. > What I can see is two noble, but conflicting ideals: 1/ `sort()` and `ksort()` should be consistent about their sorting algorithms. I think we can all agree about that in the ideal case, at least. 2/ Behavior within a minor release should be self-consistent and predictable. Knowing the discrepancy exists in current releases of 8.0.x and 8.1.x, we're currently in a position where ideal #2 is satisfied, but ideal #1 is failing in a small, and subtle way. The potential consequence of this is sites out there where the inconsistency shows through to end users (though I wouldn't actually expect any breakages as a result, just clowniness). With the recently applied patch, we would resolve ideal #1, but we would do so at the cost of ideal #2. It's doubtful this will actually break any code either, but potentially user-facing affects could be visible, especially in a mixed-version environment (such as during a rolling upgrade). Given the above, my initial inclination is to err on the side of conservatism for 8.0.x at the least (we're nearly at the end of our primary bug-fix cycle anyway) by reverting the fix on our branch. For 8.1, I think we have a more difficult decision to make with over a year of bug-fix releases to go, and I might be swayed to keep the fix around there. > > […] incorporates > > […] > > Well, the fix is part of the currents RCs; that doesn't make it > impossible to revert, but RMs should have a say in that. Thus I'm > adding Sara and Gabriel as recepients. > Don't forget Ben and Patrick as well, this impacts 8.1 equally. -Sara --- ## #118516 — Christian Schneider — 2022-08-26T16:53:44+00:00 URL: https://externals.io/message/118483#118516 · In reply to: #118515 Am 26.08.2022 um 18:28 schrieb Sara Golemon : > What I can see is two noble, but conflicting ideals: > 1/ `sort()` and `ksort()` should be consistent about their sorting algorithms. > I think we can all agree about that in the ideal case, at least. > 2/ Behavior within a minor release should be self-consistent and > predictable. > > Given the above, my initial inclination is to err on the side of > conservatism for 8.0.x at the least (we're nearly at the end of our primary > bug-fix cycle anyway) by reverting the fix on our branch. > For 8.1, I think we have a more difficult decision to make with over a year > of bug-fix releases to go, and I might be swayed to keep the fix around > there. I agree with the description of the ideals but I'm not sure why you think the resolution should be different of 8.0 than 8.1. We already transitioned our existing code base from 8.0 to 8.1, including testing for changes due to the way numeric string are handled. I think it is reasonable to adapt it for 8.2 (where another round of breaking changes will have to be tested anyway) but I would not expect to having to do this for a bug-fix release 8.1.x. That's why I'd rather have this change postponed to 8.2 (which is not that far off anyway). Regards, - Chris --- ## #118517 — Sara Golemon — 2022-08-26T16:58:06+00:00 URL: https://externals.io/message/118483#118517 · In reply to: #118516 On Fri, Aug 26, 2022 at 11:53 AM Christian Schneider wrote: > Am 26.08.2022 um 18:28 schrieb Sara Golemon : > > […] > algorithms. > > […] > primary > > […] > year > > […] > > I agree with the description of the ideals but I'm not sure why you think > the resolution should be different of 8.0 than 8.1. > > We already transitioned our existing code base from 8.0 to 8.1, including > testing for changes due to the way numeric string are handled. I think it > is reasonable to adapt it for 8.2 (where another round of breaking changes > will have to be tested anyway) but I would not expect to having to do this > for a bug-fix release 8.1.x. > > That's why I'd rather have this change postponed to 8.2 (which is not that > far off anyway). > > Honestly, I vacillated on this one for awhile, having started from a "we should really revert on 8.1 as well". I flipped when I decided that the breakage potential was honestly very small. That said, no real complaints either way. -Sara --- ## #118538 — Christoph M. Becker — 2022-08-29T10:48:04+00:00 URL: https://externals.io/message/118483#118538 · In reply to: #118515 > On Fri, Aug 26, 2022 at 7:19 AM Christoph M. Becker > wrote: > > […] > > What I can see is two noble, but conflicting ideals: > 1/ `sort()` and `ksort()` should be consistent about their sorting algorithms. > I think we can all agree about that in the ideal case, at least. > 2/ Behavior within a minor release should be self-consistent and > predictable. > > Knowing the discrepancy exists in current releases of 8.0.x and 8.1.x, > we're currently in a position where ideal #2 is satisfied, but ideal #1 is > failing in a small, and subtle way. The potential consequence of this is > sites out there where the inconsistency shows through to end users (though > I wouldn't actually expect any breakages as a result, just clowniness). > > With the recently applied patch, we would resolve ideal #1, but we would do > so at the cost of ideal #2. It's doubtful this will actually break any > code either, but potentially user-facing affects could be visible, > especially in a mixed-version environment (such as during a rolling > upgrade). > > Given the above, my initial inclination is to err on the side of > conservatism for 8.0.x at the least (we're nearly at the end of our primary > bug-fix cycle anyway) by reverting the fix on our branch. > For 8.1, I think we have a more difficult decision to make with over a year > of bug-fix releases to go, and I might be swayed to keep the fix around > there. I have reverted the commit on PHP-8.0 just now. Please don't forget to revert on PHP-8.0.23. > > […] > incorporates > > […] > > Don't forget Ben and Patrick as well, this impacts 8.1 equally. Yeah, would be good to hear whether the commit should be reverted on PHP-8.1 as well. -- Christoph M. Becker --- ## #118541 — Ben Ramsey — 2022-08-30T14:25:07+00:00 URL: https://externals.io/message/118483#118541 · In reply to: #118538 > > > > > > […] > > I have reverted the commit on PHP-8.0 just now. Please don't forget to > revert on PHP-8.0.23. > > > […] > > Yeah, would be good to hear whether the commit should be reverted on > PHP-8.1 as well. I decided to revert this in PHP-8.1 and PHP-8.1.10 branches, as well. Cheers, Ben --- ## #118542 — Christoph M. Becker — 2022-08-30T15:01:00+00:00 URL: https://externals.io/message/118483#118542 · In reply to: #118541 > > […] > > I decided to revert this in PHP-8.1 and PHP-8.1.10 branches, as well. Thank you! A PR for the documentation is welcome. -- Christoph M. Becker