Everything

php

[RFC] Warn about invalid strings in arithmetic (moving back to discussion)

php.internals

Andrew Faulds

10 years ago

Good evening,

I had opened the vote for this RFC, but then I was reminded by a
discussion in the pull request about a particular unresolved problem I
had forgotten about.

In order to deal with that issue, I have cancelled the vote and expanded
the RFC (both in length and scope) to deal with it.

The specific issue is detailed in a new section, "Fractional and
scientific notation strings with integer operators", but it affects
other sections as well.

So, the RFC is back under discussion.

Please read it and tell me your thoughts.

Thanks!

--
Andrea Faulds
https://ajf.me/

Andrew Faulds

10 years ago

...I proof-read my email, but I forgot the most important thing. The link!

https://wiki.php.net/rfc/invalid_strings_in_arithmetic

Sorry.

Andrea Faulds wrote:

> Good evening,
>
> I had opened the vote for this RFC, but then I was reminded by a
> discussion in the pull request about a particular unresolved problem I
> had forgotten about.
>
> In order to deal with that issue, I have cancelled the vote and expanded
> the RFC (both in length and scope) to deal with it.
>
> The specific issue is detailed in a new section, "Fractional and
> scientific notation strings with integer operators", but it affects
> other sections as well.
>
> So, the RFC is back under discussion.
>
> Please read it and tell me your thoughts.
>
> Thanks!

--
Andrea Faulds
https://ajf.me/

Levi Morrison

10 years ago

I personally consider the new changes with fractional and scientific
notation strings and integer operators to be the more important
change. Thanks to everyone who identified and fixed this
inconsistency.

Does anyone have any good ideas on how to prevent strtol and strtod
from being re-introduced to the repository? I could potentially see
this happening.

On Tue, Jan 26, 2016 at 2:36 PM, Andrea Faulds <ajf@ajf.me> wrote:

Bishop Bettini

10 years ago

On Tue, Jan 26, 2016 at 7:54 PM, Levi Morrison <levim@php.net> wrote:

> I personally consider the new changes with fractional and scientific
> notation strings and integer operators to be the more important
> change. Thanks to everyone who identified and fixed this
> inconsistency.
>

Seconded, thanks all!

> Does anyone have any good ideas on how to prevent strtol and strtod
> from being re-introduced to the repository? I could potentially see
> this happening.

Obviously not portable, but somewhere high up we could:

#pragma GCC poison strtol strtod

Andrew Faulds

10 years ago

Hi Levi,

Levi Morrison wrote:

> I personally consider the new changes with fractional and scientific
> notation strings and integer operators to be the more important
> change. Thanks to everyone who identified and fixed this
> inconsistency.
>
> Does anyone have any good ideas on how to prevent strtol and strtod
> from being re-introduced to the repository? I could potentially see
> this happening.

They're the correct tools to use in some cases. We have a special class
of numeric strings for array indexes, for example (which can't be fixed).

For the cases this RFC would fix, the tests would spot if the behaviour
changed.

Thanks.

--
Andrea Faulds
https://ajf.me/

Michael Wallner

10 years ago

On 27/01/16 01:54, Levi Morrison wrote:

> I personally consider the new changes with fractional and scientific
> notation strings and integer operators to be the more important
> change. Thanks to everyone who identified and fixed this
> inconsistency.
>
> Does anyone have any good ideas on how to prevent strtol and strtod
> from being re-introduced to the repository? I could potentially see
> this happening.
>

Pardon? There's no need to "ban" any standard functions, just make sure
you always use is_numeric_string when dealing with userland. It's
perfectly fine to use strtol, when you know what you're doing.
Unfortunately, it seems, we don't have a suitable place to put that notice.

Kudos to everyone discovering that.

--
Regards,
Mike

Zeev Suraski

10 years ago

> -----Original Message-----
> From: Andrea Faulds [mailto:ajf@ajf.me]
> Sent: Tuesday, January 26, 2016 11:36 PM
> To: internals@lists.php.net
> Subject: [PHP-DEV] Re: [RFC] Warn about invalid strings in arithmetic (moving
> back todiscussion)
>
> ...I proof-read my email, but I forgot the most important thing. The link!
>
> https://wiki.php.net/rfc/invalid_strings_in_arithmetic
>
> Sorry.
>
> Andrea Faulds wrote:
> > Good evening,
> >
> > I had opened the vote for this RFC, but then I was reminded by a
> > discussion in the pull request about a particular unresolved problem I
> > had forgotten about.
> >
> > In order to deal with that issue, I have cancelled the vote and
> > expanded the RFC (both in length and scope) to deal with it.
> >
> > The specific issue is detailed in a new section, "Fractional and
> > scientific notation strings with integer operators", but it affects
> > other sections as well.
> >
> > So, the RFC is back under discussion.
> >
> > Please read it and tell me your thoughts.

Andrea,

I think this is a good proposal.  Slightly worried about the E_NOTICE for things like "5px" + "10px", which I think are probably not uncommon, and would have been better served by a separate E_STRICT, but that's no longer (practically) available.

Either way seems like a good idea.

Thanks,

Zeev

Andrew Faulds

10 years ago

Hi everyone,

There is one unresolved issue with the current patch that the RFC
doesn't address, so I'll ask about it here.

As part of supporting exponent notation in all remaining integer
operations (casts, operators), I would like to have intval() support it,
to match the `(int)` cast.

For strings, intval() doesn't use the normal zval-to-long conversion
functions (zval_get_long/convert_to_long), but instead uses strtol
directly. This is so it can support multiple bases, e.g. intval("10",
12) results in 12.

In order for intval() to support exponent notation, I'd have to change
it to now call zval_get_long, but that function doesn't support
non-decimal bases, nor does is_numeric_string_ex, which underlies it.

So, we'd either have to make intval() only support exponent notation for
base 10, an unfortunate inconsistency, or we could not touch intval(),
but then intval($a, 10) would no longer act the same as (int)$a.

I'm leaning towards the first choice, but I'd like to hear what the
mailing list thinks. Either way we have a new inconsistency. But then,
intval() has unfortunate behaviour with its base parameter anyway, in
that ignores the base for non-string input. That means that intval(123,
8) and intval("123", 8) aren't equivalent, a violation of weak typing.

Anyway, please tell me your thoughts.

Thanks.

--
Andrea Faulds
https://ajf.me/

Andrew Faulds

10 years ago

Hi again,

Andrea Faulds wrote:

> There is one unresolved issue with the current patch that the RFC
> doesn't address, so I'll ask about it here.
>
> As part of supporting exponent notation in all remaining integer
> operations (casts, operators), I would like to have intval() support it,
> to match the `(int)` cast.
>
> For strings, intval() doesn't use the normal zval-to-long conversion
> functions (zval_get_long/convert_to_long), but instead uses strtol
> directly. This is so it can support multiple bases, e.g. intval("10",
> 12) results in 12.
>
> In order for intval() to support exponent notation, I'd have to change
> it to now call zval_get_long, but that function doesn't support
> non-decimal bases, nor does is_numeric_string_ex, which underlies it.
>
> So, we'd either have to make intval() only support exponent notation for
> base 10, an unfortunate inconsistency, or we could not touch intval(),
> but then intval($a, 10) would no longer act the same as (int)$a.
>
> I'm leaning towards the first choice, but I'd like to hear what the
> mailing list thinks. Either way we have a new inconsistency. But then,
> intval() has unfortunate behaviour with its base parameter anyway, in
> that ignores the base for non-string input. That means that intval(123,
> 8) and intval("123", 8) aren't equivalent, a violation of weak typing.
>
> Anyway, please tell me your thoughts.

There hasn't been any response to this so far on the mailing list,
unfortunately. In discussions I've had elsewhere, making it apply to
intval() but only for base 10 has seemed to make sense. Both ways result
in inconsistency.

So, I've updated the RFC to clarify that it does apply to intval(), but
only where $base is 10, and also to clarify that settype() should be
affected.

Thanks.

--
Andrea Faulds
https://ajf.me/

Michael Wallner

10 years ago

> On 05 02 2016, at 11:49, Andrea Faulds <ajf@ajf.me> wrote:
>
> Hi again,
>
> Andrea Faulds wrote:
>> There is one unresolved issue with the current patch that the RFC
>> doesn't address, so I'll ask about it here.
>>
>> As part of supporting exponent notation in all remaining integer
>> operations (casts, operators), I would like to have intval() support it,
>> to match the `(int)` cast.
>>
>> For strings, intval() doesn't use the normal zval-to-long conversion
>> functions (zval_get_long/convert_to_long), but instead uses strtol
>> directly. This is so it can support multiple bases, e.g. intval("10",
>> 12) results in 12.
>>
>> In order for intval() to support exponent notation, I'd have to change
>> it to now call zval_get_long, but that function doesn't support
>> non-decimal bases, nor does is_numeric_string_ex, which underlies it.
>>
>> So, we'd either have to make intval() only support exponent notation for
>> base 10, an unfortunate inconsistency, or we could not touch intval(),
>> but then intval($a, 10) would no longer act the same as (int)$a.
>>
>> I'm leaning towards the first choice, but I'd like to hear what the
>> mailing list thinks. Either way we have a new inconsistency. But then,
>> intval() has unfortunate behaviour with its base parameter anyway, in
>> that ignores the base for non-string input. That means that intval(123,
>> 8) and intval("123", 8) aren't equivalent, a violation of weak typing.
>>
>> Anyway, please tell me your thoughts.
>
> There hasn't been any response to this so far on the mailing list, unfortunately. In discussions I've had elsewhere, making it apply to intval() but only for base 10 has seemed to make sense. Both ways result in inconsistency.
>
> So, I've updated the RFC to clarify that it does apply to intval(), but only where $base is 10, and also to clarify that settype() should be affected.

To be honest, I had not even memorised intval() supports a base, but that we have base_convert().

I’d opt for dropping base convert support from intval(), but that’s obviously not possible, because PHP-7.1 is the target for this RFC.

Cheers,
Mike

Andrew Faulds

10 years ago

Hi Mike,

Michael Wallner wrote:

>
> To be honest, I had not even memorised intval() supports a base, but that we have base_convert().
>
> I’d opt for dropping base convert support from intval(), but that’s obviously not possible, because PHP-7.1 is the target for this RFC.

Yeah, it's easy to forget about it. And as I've mentioned before, it
violates weak typing, unfortunately. base_convert() is more uniform, but
it's horrible in its own way (ignores any characters in any position
which aren't of the expected base). The bin/hex/dec/oct family have
similar issues.

Alas, you're correct that we can't really do much about this in 7.1.

--
Andrea Faulds
https://ajf.me/