Fix Hash::sort() 'natural' type fallback on PHP < 5.4 #1089

Merged
merged 1 commit into from Jan 26, 2013

Conversation

Projects
None yet
5 participants
@majna
Contributor

majna commented Jan 25, 2013

This commit also change the method behavior. I tested this on PHP 5.3 where

Hash::sort($a, '{n}.Person.name', 'asc', 'natural');

throws notice and warning and result is not sorted at all.

@markstory

This comment has been minimized.

Show comment Hide comment
@markstory

markstory Jan 25, 2013

Member

Won't this make the same application behave differently on 5.3 and 5.4? With that said, this change seems to be the original intent of the 7700a02 as setting $type = true isn't overly helpful.

Member

markstory commented Jan 25, 2013

Won't this make the same application behave differently on 5.3 and 5.4? With that said, this change seems to be the original intent of the 7700a02 as setting $type = true isn't overly helpful.

@milesj

This comment has been minimized.

Show comment Hide comment
@milesj

milesj Jan 25, 2013

Why don't you just let the client use the constant they want? Solves all the problems.

milesj commented Jan 25, 2013

Why don't you just let the client use the constant they want? Solves all the problems.

@majna

This comment has been minimized.

Show comment Hide comment
@majna

majna Jan 25, 2013

Contributor

Actually there are two things to consider here:

  • obviously there is a bug in Hash::sort()
  • with or without this fix, the behavior is different on 5.3 and 5.4 as this fallback to 'regular' can produce unexpected results

I agree that constants are more verbose and consistent with array_multisort() but it can be changed in 3.0 only, right?

So this commit fixes the behavior that shouldn't be there at all? It's not even documented and it's wrong imo.
Any suggestions on how this can be resolved without breaking BC?

Contributor

majna commented Jan 25, 2013

Actually there are two things to consider here:

  • obviously there is a bug in Hash::sort()
  • with or without this fix, the behavior is different on 5.3 and 5.4 as this fallback to 'regular' can produce unexpected results

I agree that constants are more verbose and consistent with array_multisort() but it can be changed in 3.0 only, right?

So this commit fixes the behavior that shouldn't be there at all? It's not even documented and it's wrong imo.
Any suggestions on how this can be resolved without breaking BC?

+ );
+ $sorted = Hash::sort($a, '{n}.Person.name', 'asc', 'natural');
+ $this->assertEquals($sorted, $b);
+ }

This comment has been minimized.

Show comment Hide comment
@dereuromark

dereuromark Jan 25, 2013

Member

$expected, $result (I think you switched the order here)

@dereuromark

dereuromark Jan 25, 2013

Member

$expected, $result (I think you switched the order here)

This comment has been minimized.

Show comment Hide comment
@majna

majna Jan 25, 2013

Contributor

Right. I just copied that from ´testSort()´ method :)

@majna

majna Jan 25, 2013

Contributor

Right. I just copied that from ´testSort()´ method :)

@dereuromark

This comment has been minimized.

Show comment Hide comment
@dereuromark

dereuromark Jan 25, 2013

Member

I agree, this is the original intent of the previous (and faulty) change.
👍

Member

dereuromark commented Jan 25, 2013

I agree, this is the original intent of the previous (and faulty) change.
👍

@ceeram

This comment has been minimized.

Show comment Hide comment
@ceeram

ceeram Jan 25, 2013

Member

It is documented that natural is only supported in 5.4 here:
http://book.cakephp.org/2.0/en/core-utility-libraries/hash.html#Hash::sort
When using it on earlier versions it is like it being ignored and use default regular sort

iirc back then was decided not to throw an exception when trying to use natural on earlier php versions, this change was made to prevent errors on <5.4
And i'm sorry for the double ==
😳

Member

ceeram commented Jan 25, 2013

It is documented that natural is only supported in 5.4 here:
http://book.cakephp.org/2.0/en/core-utility-libraries/hash.html#Hash::sort
When using it on earlier versions it is like it being ignored and use default regular sort

iirc back then was decided not to throw an exception when trying to use natural on earlier php versions, this change was made to prevent errors on <5.4
And i'm sorry for the double ==
😳

@majna

This comment has been minimized.

Show comment Hide comment
@majna

majna Jan 25, 2013

Contributor

5.4 requirement is documented but that fallback to 'regular' is not mentioned.

Maybe this kind of comparison assignment can be caught by PHPCS sniff like this one:
https://github.com/squizlabs/PHP_CodeSniffer/blob/master/CodeSniffer/Standards/Squiz/Sniffs/PHP/DisallowComparisonAssignmentSniff.php

Contributor

majna commented Jan 25, 2013

5.4 requirement is documented but that fallback to 'regular' is not mentioned.

Maybe this kind of comparison assignment can be caught by PHPCS sniff like this one:
https://github.com/squizlabs/PHP_CodeSniffer/blob/master/CodeSniffer/Standards/Squiz/Sniffs/PHP/DisallowComparisonAssignmentSniff.php

@ceeram

This comment has been minimized.

Show comment Hide comment
@ceeram

ceeram Jan 25, 2013

Member

yes, now looking back at it, id say i shouldnt have changed Hash class at all, and just skip the test on <5.4 which would, now you might get caught by surprise without knowing why. Just let php generate its error when wuold be best perhaps. @markstory why did you initially choose not to use the php constants, makes sense to me just to accept those as argument

Member

ceeram commented Jan 25, 2013

yes, now looking back at it, id say i shouldnt have changed Hash class at all, and just skip the test on <5.4 which would, now you might get caught by surprise without knowing why. Just let php generate its error when wuold be best perhaps. @markstory why did you initially choose not to use the php constants, makes sense to me just to accept those as argument

@dereuromark

This comment has been minimized.

Show comment Hide comment
@dereuromark

dereuromark Jan 25, 2013

Member

You can still apply those constants now and deprecate the strings (maybe for 3.0?).

Member

dereuromark commented Jan 25, 2013

You can still apply those constants now and deprecate the strings (maybe for 3.0?).

@markstory

This comment has been minimized.

Show comment Hide comment
@markstory

markstory Jan 25, 2013

Member

I don't remember the exact reason, but probably had something to do with maintaining compatibility with Set. However, its not hard to accept constants + string names. The constants are all integers. Deprecating the strings might make sense. The constants are more transparent about what is actually going on.

Member

markstory commented Jan 25, 2013

I don't remember the exact reason, but probably had something to do with maintaining compatibility with Set. However, its not hard to accept constants + string names. The constants are all integers. Deprecating the strings might make sense. The constants are more transparent about what is actually going on.

markstory added a commit that referenced this pull request Jan 26, 2013

Merge pull request #1089 from majna/2.3-sort-natural
Fix Hash::sort() 'natural' type fallback on PHP < 5.4

@markstory markstory merged commit 1fc9641 into cakephp:2.3 Jan 26, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment