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
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
Am 26.08.2022 um 14:19 schrieb Christoph M. Becker cmbecker69@gmx.de:
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
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 cmbecker69@gmx.de:
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
--
To unsubscribe, visit: https://www.php.net/unsub.php
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
Oh! My bad. Nice work! Cheers
El vie., 26 de agosto de 2022 16:51, Tim Düsterhus tim@bastelstu.be
escribió:
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/filesBest regards
Tim Düsterhus
On Fri, Aug 26, 2022 at 7:19 AM Christoph M. Becker cmbecker69@gmx.de
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
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()andksort()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
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()andksort()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
On Fri, Aug 26, 2022 at 7:19 AM Christoph M. Becker cmbecker69@gmx.de
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()andksort()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
On Fri, Aug 26, 2022 at 7:19 AM Christoph M. Becker cmbecker69@gmx.de
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()andksort()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
On Fri, Aug 26, 2022 at 7:19 AM Christoph M. Becker cmbecker69@gmx.de
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()andksort()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