I fell down a WTF hole today that led me to zend_atol().
The end result is the PR which I'd like to present for discussion (I'll add
tests before I push anything, though it might necessitate a vote).
https://github.com/php/php-src/pull/4132The issue is explained in the commit message, but I'll copy here:
zend_ato[il] don't just do number parsing.
They also check for a 'K', 'M', or 'G' at the end of the string,
and multiply the parsed value out accordingly.Unfortunately, they ignore any other non-numerics between the
numeric component and the last character in the string.
This means that numbers such as the following are both valid
and non-intuitive in their final output.
- "123KMG" is interpreted as "123G" -> 132070244352
- "123G " is interpreted as "123 " -> 123
- "123GB" is interpreted as "123B" -> 123
- "123 I like tacos." is also interpreted as "123." -> 123
This diff primarily adds warnings for these cases when the output
would be a potentially unexpected, and unhelpful value.Additionally, several places in php-src use these functions
despite not actually wanting their KMG behavior such as
session.upload_progress.freq which will happily parse "1 banana"
as a valid value.For these settings, I've switched them to ZEND_STRTOL which preserves
their existing /intended/ behavior.
- It won't respect KMG suffixes, but they never really wanted that logic
anyway.- It will ignore non-numeric suffixes so as to not introduce new
failures.We should probably reexamine that second bullet point separately.
Lastly, with these changes, zend_atoi() is no longer used in php-src,
but I left it as a valid API for 3rd party extensions.
Note that I did make it a proxy to zend_atol() since deferring the
truncation till the end is effectively the same as truncation during
multiplication, but this avoid code duplication.I think we should consider removing zend_atoi() entirely (perhaps in 8.0?)
and rename zend_atol() to something reflecting it's KMG specific behavior.
Arnaud Le Blanc grabbed that up, and submitted
https://github.com/php/php-src/pull/7788 a while ago. I'm generally
in favor of this PR; are there any objections to merging it?
Christoph