Warn about invalid strings in arithmetic #1718
Conversation
Zend/zend_operators.c
Outdated
There was a problem hiding this comment.
Might be worth making this into a zend_always_inline function and then have _zval_get_long_func and _zval_get_long_func_nonsilent or something. Dunno how compiler deals with this atm, haven't benchmarked.
Zend/zend_operators.c
Outdated
There was a problem hiding this comment.
this will convert the same string twice. This also misses incorrect double string usage, e.g. (5 & "12.5")
There was a problem hiding this comment.
"12.5" should probably still be allowed. After all, we also support 5 & 12.5. But agree that if we use is_numeric_string we no longer need the STRTOL call.
There was a problem hiding this comment.
This part is troublesome. If we use is_numeric_string to convert, then we start supporting scientific notation for numbers, which we don't at present because strtol() doesn't support it.
If we don't use it, though, then there's the problem Dmitry points out: is_numeric_string will accept things that strtol won't handle properly.
There was a problem hiding this comment.
Supporting floats and their notation (scientific) would make PHP more consistent. We currently support this for long parameters in argument parsing, e.g.
$ php -r 'var_dump(intdiv("1e3", 1));'
int(1000)
Yet due to the use of strtol() here, we don't for the integer operators, nor for (int) and intval(). So this means that 3e3 is considered to be 3 for some integer operations and 3000 for others, which is not ideal.
Would it cause backwards-compatibility issues if we started allowing floats here? I don't think it would.
There was a problem hiding this comment.
I think more consistent float support makes sense and worth the minor BC break in 7.1.
There was a problem hiding this comment.
I think so too. Anyway, this is a significant change, so I'll suspend the vote on the RFC and update it.
16047b5 to
6089355
Compare
be1e9e8 to
fbfcc8b
Compare
|
Okay, this now handles |
1dbf5bd to
08ca614
Compare
Zend/zend_compile.c
Outdated
| } | ||
|
|
||
| /* While basic arithmetic operators always produce numeric string errors, | ||
| * bitwise operators only produce errors when *both* operands are strings */ |
|
Because I rewrote history so many times, the commit timestamps are wrong, and also in a different order to how the commits are applied. |
32be4d6 to
429a75d
Compare
|
RFC has been accepted, so this should be merged sometime soon. |
|
I think that last Travis failure was some false positive, looks like a MySQL issue. |
429a75d to
6caf1d4
Compare
|
Merged via: 1e82ad8 |
Counterpart to this RFC: https://wiki.php.net/rfc/invalid_strings_in_arithmetic
Corresponding language spec pull request: php/php-langspec#155