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

9 years ago by Andrea Faulds — view source

unread

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/

9 years ago by Andrea Faulds — view source

unread

...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/

9 years ago by Levi Morrison — view source

unread

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.

...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/

9 years ago by Bishop Bettini — view source

unread

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

9 years ago by Andrea Faulds — view source

unread

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/

9 years ago by Michael Wallner — view source

unread

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

9 years ago by Zeev Suraski — view source

unread

-----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

9 years ago by Andrea Faulds — view source

unread

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",

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,

and intval("123", 8) aren't equivalent, a violation of weak typing.

Anyway, please tell me your thoughts.

Thanks.

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

9 years ago by Andrea Faulds — view source

unread

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",

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,

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/

9 years ago by Michael Wallner — view source

unread

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",

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,

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