Warn about invalid strings in arithmetic #1718
Conversation
| @@ -726,7 +738,7 @@ ZEND_API void multi_convert_to_string_ex(int argc, ...) /* {{{ */ | |||
| } | |||
| /* }}} */ | |||
|
|
|||
| ZEND_API zend_long ZEND_FASTCALL _zval_get_long_func(zval *op) /* {{{ */ | |||
| static zend_long ZEND_FASTCALL _zval_get_long_func_ex(zval *op, zend_bool silent) /* {{{ */ | |||
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_error(E_WARNING, "A non-numeric value encountered"); | ||
| return 0; | ||
| } | ||
| } |
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.
hikari-no-yume
force-pushed
the
operator_numeric_string_notices
branch
from
16047b5 to
6089355
Compare
hikari-no-yume
force-pushed
the
operator_numeric_string_notices
branch
2 times, most recently
from
be1e9e8 to
fbfcc8b
Compare
|
Okay, this now handles |
hikari-no-yume
force-pushed
the
operator_numeric_string_notices
branch
6 times, most recently
from
1dbf5bd to
08ca614
Compare
| } | ||
|
|
||
| /* 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. |
hikari-no-yume
force-pushed
the
operator_numeric_string_notices
branch
2 times, most recently
from
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. |
hikari-no-yume
force-pushed
the
operator_numeric_string_notices
branch
from
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