php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #80758 version_compare does not behave as documented (or even consistently)
Submitted: 2021-02-16 19:13 UTC Modified: 2021-05-23 12:57 UTC
Votes:2
Avg. Score:3.0 ± 0.0
Reproduced:2 of 2 (100.0%)
Same Version:0 (0.0%)
Same OS:1 (50.0%)
From: brad dot jorsch at automattic dot com Assigned:
Status: Wont fix Package: *General Issues
PHP Version: 8.0Git-2021-02-16 (Git) OS: Linux
Private report: No CVE-ID: None
View Add Comment Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
You can add a comment by following this link or if you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: brad dot jorsch at automattic dot com
New email:
PHP Version: OS:

 

 [2021-02-16 19:13 UTC] brad dot jorsch at automattic dot com
Description:
------------
While I marked the version as 8.0.2, this seems to exist in many versions, in some forms back to 4.3.0 or earlier.

The documentation for version_compare says that "any string not found in this list" comes before various recognized strings, including numbers. But certain strings do not follow this behavior.

1. Any non-alphanumeric character has that character treated as a `.` rather than it being included in the string to be compared. For example, "1.0-alpha 1" < "1.0-alpha 2" because the space is considered a `.`, even though as documented this should not be the case.
2. Exception: If that non-alphanumeric character comes directly after a number, it is not considered as a `.`. So "1.0 alpha" == "1.0 beta" because neither " alpha" nor " beta" are recognized as special strings. (see #75805)
3. Strings beginning with "a", "b", or other recognized strings are considered as being those recognized strings. (see #75806)
4. An empty string as the last component compares as less than itself. This can combine with item 1 in unexpected ways. (see #69268, and https://stackoverflow.com/a/65118939)

IMO, item 1 makes sense and should be documented as such. Items 2 and 4 make no sense and should be fixed. Item 3 could go either way, but since the documentation calls out "a" being the same as "alpha" and so on that implies that "apple" should not be.

The attached patch fixes 2 and 4, and 3 with the interpretation that "apple" should not be the same as "alpha". I'll leave documentation updates to you, since that's done outside of the main repo.

P.S. I see cmb@php.net has a habit of closing reports like these as "not a bug" with the justification that a "PHP-standardized version string" has some sort of meaning that excludes these inputs. If you really want to continue considering this sort of thing "not a bug", you should actually document a "PHP-standardized version string" as only containing alphanumerics plus `.`, `-`, `_`, and `+` somewhere, as currently that is not defined anywhere. You should also reject invalid strings with an Error instead of silently handling them unexpectedly.

But IMO restricting this to only "PHP-standardized version strings" is going against how most people actually use the method.

Test script:
---------------
var_dump( version_compare( "1.0 alpha", "1.0 beta" ) );
var_dump( version_compare( "1.0.apple", "1.0.coconut" ) );
var_dump( version_compare( "1.0.", "1.0." ) );
var_dump( version_compare( "1.0.*", "1.0.*" ) );


Expected result:
----------------
int(-1)
int(0)
int(0)
int(0)


Actual result:
--------------
int(0)
int(1)
int(-1)
int(-1)

Patches

patch (last revision 2021-02-16 19:14 UTC by brad dot jorsch at automattic dot com)

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2021-02-16 19:14 UTC] brad dot jorsch at automattic dot com
The following patch has been added/updated:

Patch Name: patch
Revision:   1613502857
URL:        https://bugs.php.net/patch-display.php?bug=80758&patch=patch&revision=1613502857
 [2021-05-23 05:13 UTC] krakjoe@php.net
-Status: Open +Status: Wont fix
 [2021-05-23 05:13 UTC] krakjoe@php.net
Does it sound right to you that because people mis-use a function, we should change how it works to accommodate that misuse ?

We've been using the same versioning scheme forever, version_compare is intended to compare PHP versions and nothing else.

There's no generally applicable version of this function that could work for everyone, and that is not what we are trying to provide.

If version_compare doesn't work for your project, it's because it's not PHP, and doesn't use PHP standardized strings.
 [2021-05-23 07:04 UTC] rtrtrtrtrt at dfdfdfdf dot dfd
at least the php ivory tower in that context is consistent with what counts as security bug :-)
 [2021-05-23 12:57 UTC] brad dot jorsch at automattic dot com
krakjoe@php.net, I'm extremely disappointed that you ignored everything in the bug report except the very last sentence, and dismissed it entirely based on that.

As I said before, if you want version_compare to only compare "PHP-standardized version strings", that's fine. But then **document what that means** and **reject invalid input** instead of continuing to have inconsistent behavior.
 
PHP Copyright © 2001-2021 The PHP Group
All rights reserved.
Last updated: Sat Oct 16 13:03:34 2021 UTC