Doc #80503 [Ver]: version_compare throws ValueError on invalid comparison operator in 8.0.0

Edit report at https://bugs.php.net/bug.php?id=80503&edit=1 ID: 80503 User updated by: john at zerocrates dot org Reported by: john at zerocrates dot org Summary: version_compare throws ValueError on invalid comparison operator in 8.0.0 Status: Verified Type: Documentation Problem Package: PHP options/info functions PHP Version: 8.0.0 Block user comment: N Private report: N New Comment: 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. Previous Comments: ------------------------------------------------------------------------ [2020-12-12 12:50:15] 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 03:12:40] 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-11 23:23:51] 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-10 22:20:37] 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. ------------------------------------------------------------------------ -- Edit this bug report at https://bugs.php.net/bug.php?id=80503&edit=1