Hi!
Just a little note - I don't think any option that adds warnings where
there were not warnings is acceptable in this case for any stable
version. There are dozens of ways extra warning could break an existing
app.
Also, wouldn't simple regexp or filter or is_numeric check solve this
issue while allowing much more flexible reaction to wrong data? I'm not
sure that more warnings is better than more data checking.
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Hi!
Just a little note - I don't think any option that adds warnings where
there were not warnings is acceptable in this case for any stable
version. There are dozens of ways extra warning could break an existing
app.Also, wouldn't simple regexp or filter or is_numeric check solve this
issue while allowing much more flexible reaction to wrong data? I'm not
sure that more warnings is better than more data checking.
I think it depends which release. If next 5.x, then I think a warning
might be acceptable.
My personal preference would be to make it act largely like string to
int conversion works at present, where it stops at incorrect chars.
Though I think it's tolerant of leading whitespace, for some reason,
which I don't like.
Hence, for some sort of consistency, I like option C best. I don't think
throwing a warning in the next 5.x.y is a good idea though, but I
personally don't think it's a bad idea for 5.6. However, my preferred
behaviour would break B/C, so I'm not sure.
--
Andrea Faulds
http://ajf.me/
Hi!
I think it depends which release. If next 5.x, then I think a warning
might be acceptable.
I was talking about existing stable branches (5.4 and 5.5) of course.
5.6 still can be discussable.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Just a little note - I don't think any option that adds warnings where
there were not warnings is acceptable in this case for any stable
version. There are dozens of ways extra warning could break an existing
app.
I can see the argument for that, though anything depending on buggy
conversion is probably broken.
Also, wouldn't simple regexp or filter or is_numeric check solve this
issue while allowing much more flexible reaction to wrong data? I'm not
sure that more warnings is better than more data checking.
Sure, one could validate before conversion with something like:
if (strcmp($val, base_convert($val, $base, $base))) {
/* $val isn't purely in base $base */
} else {
$newval = base_convert($val, $base, $newbase);
}
And a proper app should have logic like that regardless.
However I do think that when apps don't apply such forward-thinking
logic, we should be prepared to be noisy about it (as we do with an
fopen()
call which wasn't preceeded by an is_readable()
/is_writable()
check)
-Sara
Just a little note - I don't think any option that adds warnings where
there were not warnings is acceptable in this case for any stable
version. There are dozens of ways extra warning could break an existing
app.I can see the argument for that, though anything depending on buggy
conversion is probably broken.Also, wouldn't simple regexp or filter or is_numeric check solve this
issue while allowing much more flexible reaction to wrong data? I'm not
sure that more warnings is better than more data checking.Sure, one could validate before conversion with something like:
if (strcmp($val, base_convert($val, $base, $base))) {
/* $val isn't purely in base $base */
} else {
$newval = base_convert($val, $base, $newbase);
}And a proper app should have logic like that regardless.
However I do think that when apps don't apply such forward-thinking
logic, we should be prepared to be noisy about it (as we do with an
fopen()
call which wasn't preceeded by anis_readable()
/is_writable()
check)-Sara
You can only reasonably prepare for that if you are aware of how the
implementation works, so that's about 30 of us ... before yesterday was
probably less than 10 ...
I'm with you, it's crap, fix it ...
Cheers
Joe
Hi Sara,
B. Throw a Warning and return FALSE
on unexpected characters
seems the best option in the long run.
There aren't much BC break as there's not much meaning
to supply strange string to base_convert()
.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Is there any chance to revive this RFC? There are at least three
tickets in the bug tracker regarding the sloppy behavior of base_convert()
:
- https://bugs.php.net/bug.php?id=55393
- https://bugs.php.net/bug.php?id=61740
- https://bugs.php.net/bug.php?id=65212
This clearly shows, that _php_math_basetozval() needs to be improved.
--
Christoph M. Becker
Is there any chance to revive this RFC? There are at least three
tickets in the bug tracker regarding the sloppy behavior ofbase_convert()
:
- https://bugs.php.net/bug.php?id=55393
- https://bugs.php.net/bug.php?id=61740
- https://bugs.php.net/bug.php?id=65212
This clearly shows, that _php_math_basetozval() needs to be improved.
Huh... It's got my name on it, but I can't for the life of me recall
starting this up.
I agree with past-me, however. This is some clowny behavior.
Let's consider the conversation reopened. Stas' opinion that the app
should be validating their input to base_convert()
being valid noted,
and move the targeting version to 7.3.
-Sara
Is there any chance to revive this RFC? […]
Huh... It's got my name on it, but I can't for the life of me recall
starting this up.I agree with past-me, however. This is some clowny behavior.
Let's consider the conversation reopened. […]
That's the spirit! Thanks.
I tend to prefer option C (throw a Warning, stop processing, and return
the value up to that point). Option B (throw a Warning and return FALSE
on unexpected characters) might be cleaner, but we're not that picky
elsewhere, and the return value might not even be checked, and thus
easily misinterpreted as zero.
--
Christoph M. Becker
I tend to prefer option C (throw a Warning, stop processing, and return
the value up to that point). Option B (throw a Warning and returnFALSE
on unexpected characters) might be cleaner, but we're not that picky
elsewhere, and the return value might not even be checked, and thus
easily misinterpreted as zero.
I've also added option D: Throw an exception. It's hard to miss, even
if it's inconsistent with the vast majority of our runtime library
(and thus, probably a bad idea).
C is certainly the most consistent with the behavior of (int) cast,
but it's also error prone in other ways. See also, literally anyone
who has ever been caught by invalid "numeric" user input ever.
B is at least consistent with the spirit of many other standard
library functions, but I agree with you that the output of it is
problematic.
I think we can all agree that A is a terrible option and if someone is
using this as a cheap version of preg_replace('/[^0-9]/', '', $in);
then they deserve to have their code break.
My vote atm, given all of the above would be B, but I'm not super
happy about it.
-Sara
I tend to prefer option C (throw a Warning, stop processing, and return
the value up to that point). Option B (throw a Warning and returnFALSE
on unexpected characters) might be cleaner, but we're not that picky
elsewhere, and the return value might not even be checked, and thus
easily misinterpreted as zero.I've also added option D: Throw an exception.
I'm not a computer science guru but that is what I was thinking should
happen.
The only way bad arguments should get to that point where this issue
occurs is if the programmer did not properly validate the data.
I don't like that PHP is often very forgiving and recasts types (I guess
what they call loose type), I personally think if it is appropriate to
recast the programmer should identify the that it is a safe scenario to
recast and recast intentionally.
I'm a huge fan of throwing \TypeError from within my own classes when a
parameter is incorrect and \InvalidArgumentException when the type is
correct but the argument is absurd.
If it would break code, keep the existing behavior during the next
release but log a warning - just like is done with deprecation, but the
release after, I agree throw an exception.
An exception is better than GIGO - an exception lets the coder know they
have a mistake.
Hi!
Is there any chance to revive this RFC? There are at least three
tickets in the bug tracker regarding the sloppy behavior ofbase_convert()
:
I think if we want to take this RFC forward it should be reduced to
either proposing one action, instead of four, or at the worst case,
choice of two for vote.
It also needs BC impact section added - AFAIK it may break some tests
and _php_math_basetozval is PHPAPI, which means besides 4 uses in core,
any extension could be using it and that extension may not be prepared
for it working differently. Which means if we change it, the failure
behavior that happens should be such that application ignoring it would
not have trouble - at least not worse than today.
--
Stas Malyshev
smalyshev@gmail.com
Is there any chance to revive this RFC? There are at least three
tickets in the bug tracker regarding the sloppy behavior ofbase_convert()
:I think if we want to take this RFC forward it should be reduced to
either proposing one action, instead of four, or at the worst case,
choice of two for vote.
Agreed, I presented it as more options initially so that we could talk
out what options might be preferred before going to any kind of
voting.
Given the lack of opinion on the matter, I'd probably be inclined to
present option B as the official proposal. Consistent with PHP
functional APIs.
It also needs BC impact section added - AFAIK it may break some tests
and _php_math_basetozval is PHPAPI, which means besides 4 uses in core,
any extension could be using it and that extension may not be prepared
for it working differently. Which means if we change it, the failure
behavior that happens should be such that application ignoring it would
not have trouble - at least not worse than today.
Technically, the internal API as it stands can return FAILURE
already, though I can see your argument that a caller might assume
that it'll never fail so long as they pass a string with a valid base.
I don't think I agree with your point, as it's reasonable to expect an
API caller to examine the return code of the function they're
calling,. but I can see where you're coming from.
-Sara
Hi!
Technically, the internal API as it stands can return FAILURE
already, though I can see your argument that a caller might assume
that it'll never fail so long as they pass a string with a valid base.
I don't think I agree with your point, as it's reasonable to expect an
API caller to examine the return code of the function they're
calling,. but I can see where you're coming from.
The problem is that if we return false, and the caller expects a number,
it may use the zval as numeric zval without conversion. I am not sure in
7.x using false as IS_LONG without conversion is safe anymore, but it is
possible that with proper initialization it would be. Other scenarios -
such as throwing without initializing the zval - may behave even worse.
Option C is relatively safe I think. Maybe B is safe too, and D can be
made safe with proper initialization - in any case that needs to be
checked.
it's reasonable to expect an API caller to examine the return code of
the function they're
calling
We aren't really good in documenting when API functions return
SUCCESS/FAILURE. In this particular case, the source indicates that if
you send it a string and the base is constant (common case), it would
never return FAILURE. So it's hard to blame extension writer who would
not check for FAILURE.
If we're changing the contract (or rather, having failed to document any
explicit contract, changing the implied contract defined by the source
code) we need at least to take some measures so that we don't create
subtle bugs in other extensions. Fortunately, in this case it seems to
be achievable.
Stas Malyshev
smalyshev@gmail.com