Welcome! Log In Create A New Profile

Advanced

[PHP-DEV] [RFC] Base Conversion Clowniness

Posted by Sara Golemon 
Sara Golemon
[PHP-DEV] [RFC] Base Conversion Clowniness
December 22, 2013 08:50PM
https://wiki.php.net/rfc/base-convert

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Stas Malyshev
Re: [PHP-DEV] [RFC] Base Conversion Clowniness
December 22, 2013 11:30PM
Hi!

> https://wiki.php.net/rfc/base-convert

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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Andrea Faulds
Re: [PHP-DEV] [RFC] Base Conversion Clowniness
December 22, 2013 11:40PM
On 22/12/13 22:26, Stas Malyshev wrote:
> Hi!
>
>> https://wiki.php.net/rfc/base-convert
>
> 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/

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Stas Malyshev
Re: [PHP-DEV] [RFC] Base Conversion Clowniness
December 22, 2013 11:50PM
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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Sara Golemon
Re: [PHP-DEV] [RFC] Base Conversion Clowniness
December 23, 2013 12:20AM
> 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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Yasuo Ohgaki
Re: [PHP-DEV] [RFC] Base Conversion Clowniness
December 23, 2013 01:30AM
Hi Sara,

On Mon, Dec 23, 2013 at 4:46 AM, Sara Golemon <[email protected]> wrote:

> https://wiki.php.net/rfc/base-convert
>

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
Joe Watkins
Re: [PHP-DEV] [RFC] Base Conversion Clowniness
December 23, 2013 09:40AM
On 12/22/2013 11:10 PM, Sara Golemon wrote:
>> 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
>

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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Christoph M. Becker
[PHP-DEV] Re: [RFC] Base Conversion Clowniness
March 12, 2018 03:20PM
On 22.12.2013 at 20:46, Sara Golemon wrote:

> https://wiki.php.net/rfc/base-convert

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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Sara Golemon
[PHP-DEV] Re: [RFC] Base Conversion Clowniness
March 12, 2018 04:00PM
On Mon, Mar 12, 2018 at 10:10 AM, Christoph M. Becker <[email protected]> wrote:
> On 22.12.2013 at 20:46, Sara Golemon wrote:
>
>> https://wiki.php.net/rfc/base-convert
>
> 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.
>
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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Christoph M. Becker
[PHP-DEV] Re: [RFC] Base Conversion Clowniness
March 12, 2018 04:40PM
On 12.03.2018 at 15:49, Sara Golemon wrote:

> On Mon, Mar 12, 2018 at 10:10 AM, Christoph M. Becker <[email protected]> wrote:
>
>> On 22.12.2013 at 20:46, Sara Golemon wrote:
>>
>>> https://wiki.php.net/rfc/base-convert
>>
>> 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



--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Sara Golemon
[PHP-DEV] Re: [RFC] Base Conversion Clowniness
March 12, 2018 05:50PM
On Mon, Mar 12, 2018 at 11:32 AM, Christoph M. Becker <[email protected]> wrote:
> 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.
>
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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Alice Wonder
Re: [PHP-DEV] Re: [RFC] Base Conversion Clowniness
March 12, 2018 06:10PM
On 03/12/2018 09:43 AM, Sara Golemon wrote:
> On Mon, Mar 12, 2018 at 11:32 AM, Christoph M. Becker <[email protected]> wrote:
>> 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.
>>
> 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.


--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Stanislav Malyshev
Re: [PHP-DEV] Re: [RFC] Base Conversion Clowniness
March 18, 2018 11:00PM
Hi!

>> https://wiki.php.net/rfc/base-convert
>
> 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():

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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Sara Golemon
Re: [PHP-DEV] Re: [RFC] Base Conversion Clowniness
March 18, 2018 11:50PM
On Sun, Mar 18, 2018 at 5:49 PM, Stanislav Malyshev <[email protected]> wrote:
>>> https://wiki.php.net/rfc/base-convert
>>
>> 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():
>
> 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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Stanislav Malyshev
Re: [PHP-DEV] Re: [RFC] Base Conversion Clowniness
March 19, 2018 12:00AM
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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Sorry, only registered users may post in this forum.

Click here to login