php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Doc Bug #80503 version_compare throws ValueError on invalid comparison operator in 8.0.0
Submitted: 2020-12-10 22:20 UTC Modified: 2020-12-12 22:24 UTC
From: john at zerocrates dot org Assigned:
Status: Verified Package: PHP options/info functions
PHP Version: 8.0.0 OS:
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: john at zerocrates dot org
New email:
PHP Version: OS:

 

 [2020-12-10 22:20 UTC] john at zerocrates dot org
Description:
------------
The documentation (just recently updated to include the information about version_compare returning null on an invalid comparison operator in prior versions, from doc bug #77838), doesn't mention the new behavior of the function in 8.0.0: it throws ValueError instead.

This also should possibly be mentioned as an incompatible change in the migration to 8.0.0 document.


Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2020-12-11 23:23 UTC] cmb@php.net
-Status: Open +Status: Verified
 [2020-12-11 23:23 UTC] cmb@php.net
Documenting all ValueErrors is work in progress[1].

It also needs to be documented that abbreviations of the allowed
operators are also supported; an empty string is the abbreviation
of '<'; or maybe that is a bug?

[1] <https://github.com/php/doc-en/pull/163>
 [2020-12-12 03:12 UTC] john at zerocrates dot org
The treatment of the empty string is a new one to me: I would say it's a bug, but probably a long-preexisting one.

The checks for the comparison operations for version_compare all take this basic form:

if (!strncmp(op, "<", op_len) || !strncmp(op, "lt", op_len)) {

repeated for each set of equivalent operations and abbreviations. op_len is the length of the passed operation string, so when the empty string is passed, op_len is zero and strncmp will always consider its arguments to be "matching" and return 0. So empty string is treated as a less-than operation simply because less-than is the first check.

This structure of the checks also means that "even more abbreviated" forms of the multi-character operations are also accepted: "l" will be treated as "lt", "g" as "gt", "e" as "eq" and both "n" and "!" as "ne"/"!=". Is this what you were referring to in your comment about "abbreviations"? 

I'd say those were probably not intended either (for example, the code explicitly checking both "=" and "==" would seem to indicate that this behavior wasn't expected, plus the "feature" not being documented), but they've probably been there untouched and working for something on the order of two decades. so I would understand that simply documenting their presence might be preferable. The empty string handling is probably just about (if not exactly) as old.

PHP 8 itself, as far as I can tell, made no changes to this behavior, buggy or not, beyond simply converting the null return to a ValueError.
 [2020-12-12 12:50 UTC] cmb@php.net
Yes, that is what I meant with "abbreviations".  I just submitted
<https://github.com/php/php-src/pull/6510> for discussion.

Regarding the ValueErrors: in most cases there was a warning in
PHP 7 (and likely in PHP 5 as well) about unsupported argument
values; not in this case, though, what is a bit unfortunate.
Still, I think throwing a ValueError is totally reasonable instead
of silently returning NULL, because that could easily mistaken for
FALSE.
 [2020-12-12 22:24 UTC] john at zerocrates dot org
I totally agree: usage of a nonexistent operator here is overwhelmingly likely to be a bug in the calling code which is unknowingly simply writing null in extreme longhand (case in point: the only reason I noticed this change was due to a preexisting but unnoticed bug where the invalid operator "gte" was being used).

Even constructing some situation where code *does* depend on the null return (say, if the calling code takes the operator as input and uses the null return to detect invalid operators... can't immediately think of anything else quasi-legitimate), that can be trivially retrofitted to work by catching the ValueError.

It is somewhat unfortunate that this wasn't a notice or warning previously, but once documented I don't see that it would be an issue.
 [2021-01-23 05:24 UTC] tonisander16 at gmail dot com
A Rust library to easily compare version numbers, and test them against various numbers in any format, and test them against various comparison operators. 


https://www.expresshr.me/
 
PHP Copyright © 2001-2021 The PHP Group
All rights reserved.
Last updated: Thu May 06 06:01:24 2021 UTC