Skip to content

Conversation

nikic
Copy link
Member

@nikic nikic commented Mar 3, 2021

Infamously, md5('240610708') == md5('QNKCDZO') returns true in PHP. The reason is that ' 0e462097431906509019562988736854' == '0e830400451993494058024219903391' interprets the two hashes as floating point numbers in exponential notation, and any numbers of the form 0eNNNN evaluate to zero. Thus the strings are considered equal. This is usually not desirable.

This patch changes the string comparison rules to treat strings that contain an exponential of the form 0eNNNN differently: If both sides of the comparison are a zero-exponential, then the strings will be compared lexicographically instead (which means they are only equal if they're exactly equal). If at least one side is not a zero-exponential, then the usual rules apply. We already have a similar special case for integral numbers overflowing the integer space.

@chschneider
Copy link
Contributor

chschneider commented Mar 3, 2021

I noticed that the edge case "0e1" == "-0e1" also changed from true to false.
Maybe desired (can't make up my mind right now), but probably something that should be mentioned.

@nikic
Copy link
Member Author

nikic commented Mar 4, 2021

@chschneider Nice catch! I'm also not sure on that one. If we go by the original motivation of hash comparisons, then we should special-case the negative case...

@ramsey ramsey added the Feature label May 8, 2021
@ramsey
Copy link
Member

ramsey commented May 8, 2021

As someone who's had to work around this (after struggling for a while to figure out what was going on), I welcome this change. 😄

What's the status of this patch?

@nikic
Copy link
Member Author

nikic commented Jun 10, 2021

Mailing-list discussion (https://externals.io/message/113346) wasn't particularly enthusiastic about making this change, so I'm going to close this.

@nikic nikic closed this Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants