Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » ksort breaking change »

ksort breaking change

php.internals

Go Kudo

4 years ago
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

Christoph Becker

4 years ago
On 26.08.2022 at 05:15, Go Kudo wrote:
> 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

Christian Schneider

4 years ago
Am 26.08.2022 um 14:19 schrieb Christoph M. Becker <cmbecker69@gmx.de>:
> On 26.08.2022 at 05:15, Go Kudo wrote: > >> 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.
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

juan carlos morales

4 years ago
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ó:

Tim Düsterhus

4 years ago
Hi On 8/26/22 16:15, juan carlos morales wrote:
> 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

juan carlos morales

4 years ago
Oh! My bad. Nice work! Cheers El vie., 26 de agosto de 2022 16:51, Tim Düsterhus <tim@bastelstu.be> escribió:

Sara Golemon

4 years ago
On Fri, Aug 26, 2022 at 7:19 AM Christoph M. Becker <cmbecker69@gmx.de> wrote:
> > On 26.08.2022 at 05:15, Go Kudo wrote: > > > 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. >
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.
> > 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. >
Don't forget Ben and Patrick as well, this impacts 8.1 equally. -Sara

Christian Schneider

4 years ago
Am 26.08.2022 um 18:28 schrieb Sara Golemon <pollita@php.net>:
> 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

Sara Golemon

4 years ago
On Fri, Aug 26, 2022 at 11:53 AM Christian Schneider <cschneid@cschneid.com> wrote:
> Am 26.08.2022 um 18:28 schrieb Sara Golemon <pollita@php.net>: > > 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). > >
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

Christoph Becker

4 years ago
On 26.08.2022 at 18:28, Sara Golemon wrote:
> On Fri, Aug 26, 2022 at 7:19 AM Christoph M. Becker <cmbecker69@gmx.de> > wrote: >> >> On 26.08.2022 at 05:15, Go Kudo wrote: >> >>> 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. > > 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.
>>> 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. > > 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

Ben Ramsey

4 years ago
> On Aug 29, 2022, at 05:48, Christoph M. Becker <cmbecker69@gmx.de> wrote: > > On 26.08.2022 at 18:28, Sara Golemon wrote: > >> On Fri, Aug 26, 2022 at 7:19 AM Christoph M. Becker <cmbecker69@gmx.de> >> wrote: >>> >>> On 26.08.2022 at 05:15, Go Kudo wrote: >>> >>>> 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. >> >> 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. > >>>> 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. >> >> 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.
I decided to revert this in PHP-8.1 and PHP-8.1.10 branches, as well. Cheers, Ben

Christoph Becker

4 years ago
On 30.08.2022 at 16:25, Ben Ramsey wrote:
>> On Aug 29, 2022, at 05:48, Christoph M. Becker <cmbecker69@gmx.de> wrote: >> >> On 26.08.2022 at 18:28, Sara Golemon wrote: >> >>> On Fri, Aug 26, 2022 at 7:19 AM Christoph M. Becker <cmbecker69@gmx.de> >>> wrote: >>>> >>>> On 26.08.2022 at 05:15, Go Kudo wrote: >>>> >>>>> 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. >>> >>> 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. >> >>>>> 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. >>> >>> 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. > > 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