RFC: Stop to automatically cast numeric-string to int when using them as array-key

Lynn

4 years ago

On Wed, Dec 29, 2021 at 4:42 PM Vincent Langlet <misterdeviling@gmail.com>
wrote:

> Hi,
>
> I recently discovered that an array was automatically casting
> numeric-string keys to int if it was possible. For instance, the following
> array:
>
> $a = ['01' => '01', '10' => '10'];
>
> Is not an array with the key '01' and '10' but instead consider the second
> key as an int.
>
> This has some implications like the fact that
> - array_flip(array_flip($a)) !== $a
> - array_search('10', $a) is an int when array_search('01', $a) is still a
> string. Someone using strict types and passing the result to a function
> expecting a string could end with an unexpected crash.
>
> I've created an issue about this
> https://github.com/php/php-src/issues/7845
> but it was recommended to me to send a mail to this mailing list instead.
>
> I don't think this behavior should be considered as "normal" and would like
> to propose to change this for PHP 9, as it's a BC-break. To me it can and
> be considered as a follow-up of
> https://wiki.php.net/rfc/string_to_number_comparison and
> ttps://wiki.php.net/rfc/saner-numeric-strings
> <https://wiki.php.net/rfc/saner-numeric-strings>. This is still a
> "concept"
> since I never code with C and know nothing about the PHP implementation and
> if this change would be possible. Any help is welcome then.
>
> Thanks
>

Hi,

While I'd love for this inconsistency to go away, I also know that this is
most likely such a big change that it causes months of work and broken code
because it relies on this behavior. It wouldn't surprise me if this
singular change would cause more work than all previous BC breaks combined.
This behavior is documented: PHP: Arrays - Manual
<https://www.php.net/manual/en/language.types.array.php>

The key can either be an int or a string. The value can be of any type.

Rowan Collins

4 years ago

On 29/12/2021 16:58, Lynn wrote:

> While I'd love for this inconsistency to go away, I also know that this is
> most likely such a big change that it causes months of work and broken code
> because it relies on this behavior. It wouldn't surprise me if this
> singular change would cause more work than all previous BC breaks combined.

I was about to say the same thing - this is one of those ingrained
behaviours that is very hard to analyse use of without running the code,
so only someone with an extraordinarily good test suite would detect
that their code was affected before it went live.

Like a lot of PHP's type juggling features, it is surprising in a pure
data crunching context, but extremely reasonable in PHP's original and
still very popular context of processing input from web forms. For
instance, a list of products fetched from a database will likely have
indexes which are integers, but a value from $_GET or $_POST will always
be a string. It's therefore quite helpful that $products[42] and
$products['42'] refer to the same thing, so that you can write
$products[$_GET['id']] rather than $products[(int)$_GET['id']].

Would this feature be included in a language designed with knowledge of
how PHP is used today? Possibly not, but we have no time machine to
change our minds now.

One vaguely plausible (but not easy to implement) way forward is to have
new array-like types, where keys and values can be constrained in
different ways. Then it could obey the local scalar type passing setting
("strict_types") to allow combinations like this:

declare(strict_types=0);
$stringKeyedArray = new dict<string,mixed>;
$stringKeyedArray['42'] = true; // key kept as '42'
$stringKeyedArray[42] = true; // actually sets key '42'
$intKeyedArray = new dict<int,mixed>;
$intKeyedArray[42] = true; // key kept as 42
$intKeyedArray['42'] = true; // actually sets key 42

declare(strict_types=1);
$stringKeyedArray = new dict<string,mixed>;
$stringKeyedArray['42'] = true; // key kept as '42'
$stringKeyedArray[42] = true; // TypeError
$intKeyedArray = new dict<int,mixed>;
$intKeyedArray[42] = true; // key kept as 42
$intKeyedArray['42'] = true; // TypeError

Regards,

--
Rowan Tommins
[IMSoP]

Vincent Langlet

4 years ago

> On 29/12/2021 16:58, Lynn wrote:
> > While I'd love for this inconsistency to go away, I also know that this
> is
> > most likely such a big change that it causes months of work and broken
> code
> > because it relies on this behavior. It wouldn't surprise me if this
> > singular change would cause more work than all previous BC breaks
> combined.
>
>
> I was about to say the same thing - this is one of those ingrained
> behaviours that is very hard to analyse use of without running the code,
> so only someone with an extraordinarily good test suite would detect
> that their code was affected before it went live.
>
>

I encounter this issue with an array of zipcode as key, and let's say some
delay as int:
['00001' => 2, '00002' => 5, ..., '10000' => 4, ... ];
If I want all the zipcode with a delay superior to 3, combining array_keys
and array_filter will give me an array of string and int, instead of an
array of only string ; like I would have expected.

Maybe a way to reduce the BC-break would be to say that
- The type of the key won't change anymore i.e. array_search(0, ['10' =>
0]) will be '10' and not 10.
- $array[10] will return $array[10] ?? $array['10']
- $array['10'] will return $array['10'] ?? $array[10]

This would avoid the issue with $products[$_GET['id']] VS
$products[(int)$_GET['id']] and still be a first step in the improvement of
this situation.

Stas Malyshev

4 years ago

Hi!

> I don't think this behavior should be considered as "normal" and would like
> to propose to change this for PHP 9, as it's a BC-break. To me it can and

It'd not be just a BC break, it'd be an absolutely massive BC break that
has a potential to break a ton of code and would be extremely hard to
detect. This is not something that should be changed in an advanced
version of the language.

--
Stas Malyshev
smalyshev@gmail.com

Vincent Langlet

4 years ago

> It'd not be just a BC break, it'd be an absolutely massive BC break that
> has a potential to break a ton of code and would be extremely hard to
> detect. This is not something that should be changed in an advanced
> version of the language.
>

Would you still consider this as a massive BC break if
- The type of the key don't change anymore i.e. array_search(0, ['10' =>
0]) will be '10' and not 10.
- $array[10] still return $array[10] ?? $array['10']
- $array['10'] still return $array['10'] ?? $array[10]

so it still cast the value when accessing to an array key, but when using
array_keys/array_search/array_flip/... it won't be cast anymore.

Lynn

4 years ago

On Thu, Dec 30, 2021 at 5:54 PM Vincent Langlet <
mr.vincent.langlet@gmail.com> wrote:

> Would you still consider this as a massive BC break if
> - The type of the key don't change anymore i.e. array_search(0, ['10' =>
> 0]) will be '10' and not 10.
> - $array[10] still return $array[10] ?? $array['10']
> - $array['10'] still return $array['10'] ?? $array[10]
>
> so it still cast the value when accessing to an array key, but when using
> array_keys/array_search/array_flip/... it won't be cast anymore.
>

"Would you still consider this as a massive BC break"

Yes, because `'10'` from an array flip is not `10` anymore, which means
that using any strict comparisons on those values afterwards will fail if
they assumed it was an int.

Кирилл Несмеянов

4 years ago

Christian Schneider

4 years ago

Am 31.12.2021 um 03:21 schrieb Kirill Nesmeyanov <nesk@xakep.ru>:

> I support this behavior fix because in its current form, due to a similar problem (almost?), all PSR-7 implementations contain bugs that violate RFC7230 (section 3.2:  https://datatracker.ietf.org/doc/html/rfc7230#section-3.2 ). Thus, physically, by the standard, all headers can have the name "0" (like «0: value»), but when stored inside implementations, it is converted to a string and a problem arises ($message->getHeaders() // returns array<int|string, string> instead of array<string, string>).
>
> To solve this problem without BC, it seems to me that Nikita's idea with «declares» is most suitable, like:
> ```
> <?php
> declare(strict_types=1, strict_array_keys=1);
>
> $array["0"] = 42; // array(1) { string(1) "0" => int(42) }
> ```

I probably sound like a broken record but I'd strongly advise against it as it creates language dialects which means a line like
$a["0"] = 42;
behaves differently depending on context, e.g. copying code from Stackoverflow might lead to subtle bugs and reminds me way too much of the ini-settings magic_quotes_gpc.

My take is that changing behavior is ok if it can go through a deprecation/warning phase but that is IMHO not the case here.

If people want to have different behavior I'd rather have a new type like
$a = new Dict;
$a["0"] = 42;
possibly with a short-hand for initialization.

Regards,
- Chris

Кирилл Несмеянов

4 years ago

Rowan Collins

4 years ago

On 31/12/2021 00:21, Kirill Nesmeyanov wrote:

> I support this behavior fix because in its current form, due to a similar problem (almost?), all PSR-7 implementations contain bugs that violate RFC7230 (section 3.2:https://datatracker.ietf.org/doc/html/rfc7230#section-3.2  ). Thus, physically, by the standard, all headers can have the name "0" (like «0: value»), but when stored inside implementations, it is converted to a string and a problem arises ($message->getHeaders() // returns array<int|string, string> instead of array<string, string>).

You appear to be technically correct - the RFC defines a header name
only as "token", which implies the following would all be valid HTTP
headers:

42: The Answer
!: Bang
^_^: Surprised

In practice, it would be a bad idea to use any of these.

Every single one of the field names registered with IANA [1] starts with
a letter, and proceeds with only letters, digits, and hyphen ('-'). [The
exception is "*", listed there as "reserved" to specifically prevent its
use conflicting with the wild-card value in "Vary" lists.]

I'm actually surprised this definition hasn't been updated with
interoperability advice in recent revisions of the standard. I did find
this general advice for internet message headers in RFC 3864 [2]:

> Thus, for maximum flexibility, header field names SHOULD further be
>  restricted to just letters, digits, hyphen ('-') and underscore ('_')
>  characters, with the first character being a letter or underscore.

The additional restriction on underscore ('_') in HTTP arises from CGI,
which maps headers to environment variables. For instance, Apache httpd
silently drops headers with anything other than letters, digits, and
hyphen [3] to avoid security issues caused by environment manipulation.

If I was developing a PSR-7 or similar library, I would be inclined to
drop any header composed only of digits, and issue a diagnostic warning,
so that it wouldn't escalate to a type error later. It certainly doesn't
seem reasonable to change the entire language to work around that
inconvenience.

[1] https://www.iana.org/assignments/http-fields/http-fields.xhtml
[2] https://datatracker.ietf.org/doc/html/rfc3864#section-4.1
[3] https://httpd.apache.org/docs/trunk/env.html#setting

Regards,

--
Rowan Tommins
[IMSoP]

Кирилл Несмеянов

4 years ago

Michael Morris

4 years ago

On Sat, Jan 1, 2022 at 10:47 PM Kirill Nesmeyanov <nesk@xakep.ru> wrote:

>
> >Суббота, 1 января 2022, 17:41 +03:00 от Rowan Tommins <
> rowan.collins@gmail.com>:
> >
> >On 31/12/2021 00:21, Kirill Nesmeyanov wrote:
> >> I support this behavior fix because in its current form, due to a
> similar problem (almost?), all PSR-7 implementations contain bugs that
> violate RFC7230 (section 3.2:
> https://datatracker.ietf.org/doc/html/rfc7230#section-3.2 ). Thus,
> physically, by the standard, all headers can have the name "0" (like «0:
> value»), but when stored inside implementations, it is converted to a
> string and a problem arises ($message->getHeaders() //
> returns array<int|string, string> instead of array<string, string>).
> >
> >You appear to be technically correct - the RFC defines a header name
> >only as "token", which implies the following would all be valid HTTP
> >headers:
> >
> >42: The Answer
> >!: Bang
> >^_^: Surprised
> >
> >In practice, it would be a bad idea to use any of these.
> >
> >Every single one of the field names registered with IANA [1] starts with
> >a letter, and proceeds with only letters, digits, and hyphen ('-'). [The
> >exception is "*", listed there as "reserved" to specifically prevent its
> >use conflicting with the wild-card value in "Vary" lists.]
> >
> >I'm actually surprised this definition hasn't been updated with
> >interoperability advice in recent revisions of the standard. I did find
> >this general advice for internet message headers in RFC 3864 [2]:
> >
> > > Thus, for maximum flexibility, header field names SHOULD further be
> > >  restricted to just letters, digits, hyphen ('-') and underscore ('_')
> > >  characters, with the first character being a letter or underscore.
> >
> >The additional restriction on underscore ('_') in HTTP arises from CGI,
> >which maps headers to environment variables. For instance, Apache httpd
> >silently drops headers with anything other than letters, digits, and
> >hyphen [3] to avoid security issues caused by environment manipulation.
> >
> >If I was developing a PSR-7 or similar library, I would be inclined to
> >drop any header composed only of digits, and issue a diagnostic warning,
> >so that it wouldn't escalate to a type error later. It certainly doesn't
> >seem reasonable to change the entire language to work around that
> >inconvenience.
> >
> >[1]  https://www.iana.org/assignments/http-fields/http-fields.xhtml
> >[2]  https://datatracker.ietf.org/doc/html/rfc3864#section-4.1
> >[3]  https://httpd.apache.org/docs/trunk/env.html#setting
> >
> >Regards,
> >
> >--
> >Rowan Tommins
> >[IMSoP]
> >
> >--
> >PHP Internals - PHP Runtime Development Mailing List
> >To unsubscribe, visit:  https://www.php.net/unsub.php
>
> I just gave an example of what at the moment can cause an exception in any
> application that is based on the PSR. It is enough to send the header "0:
> Farewell to the server". In some cases (for example, as is the case with
> RoadRunner) - this can cause a physical stop and restart of the server.
>
> Just in case, I will repeat my thesis: I cannot imagine that anyone is
> using this functionality consciously and that it is part of the real logic
> of the application.

You don't have a lot of experience with legacy code then. PHP, particularly
old PHP (like 4, 5.1 era) was used by a lot of idiots.

I was one of those idiots (Perhaps I still am an idiot - jury is
deliberating on that but I digress).

Snark aside though, PHP has more than its fair share of self taught
programmers (again, not trying to be insulting as I am one myself), and
they do things with the code that veterans and formally trained programmers
would never think to try, let alone implement.

I guarantee fixing how key handling is done will break something - either
in the form of code exploiting the weird behavior, or code that is guarding
against the weird behavior; not to mention any tests that might be written
- though amateurs rarely write test code (again, speaking from past
experience I've grown beyond).

> And fixing this behavior, I believe, will automatically fix many libraries
> (not necessarily PSR) that do not take this behavior into account.
>
>

And blow up who knows how many old code bases - many of which don't have
unit test suites to discover if there is a break ahead of time.  This is
the sort of BC break that would cause a cliff of users unable to migrate to
the major version that implements it.  A Python 2 vs. 3 style of break.

Even with that all said it may indeed be worth fixing - but this will
require the same sort of kid gloves approach removing register globals had
(for the newer folks, there was a time when $_REQUEST["var"] would auto
populate $var with lovely security snarls).  IIRC PHP 3 had register
globals always on, 4 created a config toggle to turn them off, and PHP 5.0
turned that toggle off by default, finally PHP 5.3 (6 without unicode more
or less) removed support for register globals entirely (My memory could be
off - it's in the changelogs for the curious).

I leave the decision making to the maintainers and contribs who do the
actual work. Hell, I personally don't even use PHP that much these days
having gotten a job where I focus on writing Cucumber tests in JavaScript
that run on node.js. I keep up with PHP and this list though cause one
never knows what the next job will entail. I just dropped out of lurk mode
to underscore along with others up thread the massive ramifications of what
is being proposed. As someone who wrote stupid code I can see this
breaking, tread lightly. And hell, I don't even know how much of that code
is still in use since I've changed employers many times since it was
written. This situation is not unique and can create huge headaches for
companies running projects on legacy code bases.

Jordan LeDoux

4 years ago

On Sat, Jan 1, 2022 at 9:20 PM Michael Morris <tendoaki@gmail.com> wrote:

> I leave the decision making to the maintainers and contribs who do the
> actual work. Hell, I personally don't even use PHP that much these days
> having gotten a job where I focus on writing Cucumber tests in JavaScript
> that run on node.js. I keep up with PHP and this list though cause one
> never knows what the next job will entail. I just dropped out of lurk mode
> to underscore along with others up thread the massive ramifications of what
> is being proposed. As someone who wrote stupid code I can see this
> breaking, tread lightly. And hell, I don't even know how much of that code
> is still in use since I've changed employers many times since it was
> written. This situation is not unique and can create huge headaches for
> companies running projects on legacy code bases.
>

I think a question worth asking is how much of that code is likely to
actually run on PHP X.X if this were done. I don't have an answer to that,
but the kind of code that you're talking about is likely to encounter other
more fundamental problems. For instance, prior to 8.0, doing `$var = $obj +
2` would result in `$var = (int)3;` In PHP 7 this also produced a notice.
In PHP 8, the same line results in a fatal error.

I don't think that the impact would be limited to code that is written
poorly however. There may be code which almost accidentally depends on this
behavior that is in fact well maintained, since it is such a nuanced
problem. To me, this seems like something worth doing, but it might be
something that should wait for a major version.

Jordan

Кирилл Несмеянов

4 years ago

  
>Воскресенье, 2 января 2022, 8:20 +03:00 от Michael Morris <tendoaki@gmail.com>:
> 
>On Sat, Jan 1, 2022 at 10:47 PM Kirill Nesmeyanov < nesk@xakep.ru > wrote:
>
>>
>> >Суббота, 1 января 2022, 17:41 +03:00 от Rowan Tommins <
>>  rowan.collins@gmail.com >:
>> >
>> >On 31/12/2021 00:21, Kirill Nesmeyanov wrote:
>> >> I support this behavior fix because in its current form, due to a
>> similar problem (almost?), all PSR-7 implementations contain bugs that
>> violate RFC7230 (section 3.2:
>>  https://datatracker.ietf.org/doc/html/rfc7230#section-3.2 ). Thus,
>> physically, by the standard, all headers can have the name "0" (like «0:
>> value»), but when stored inside implementations, it is converted to a
>> string and a problem arises ($message->getHeaders() //
>> returns array<int|string, string> instead of array<string, string>).
>> >
>> >You appear to be technically correct - the RFC defines a header name
>> >only as "token", which implies the following would all be valid HTTP
>> >headers:
>> >
>> >42: The Answer
>> >!: Bang
>> >^_^: Surprised
>> >
>> >In practice, it would be a bad idea to use any of these.
>> >
>> >Every single one of the field names registered with IANA [1] starts with
>> >a letter, and proceeds with only letters, digits, and hyphen ('-'). [The
>> >exception is "*", listed there as "reserved" to specifically prevent its
>> >use conflicting with the wild-card value in "Vary" lists.]
>> >
>> >I'm actually surprised this definition hasn't been updated with
>> >interoperability advice in recent revisions of the standard. I did find
>> >this general advice for internet message headers in RFC 3864 [2]:
>> >
>> > > Thus, for maximum flexibility, header field names SHOULD further be
>> > > restricted to just letters, digits, hyphen ('-') and underscore ('_')
>> > > characters, with the first character being a letter or underscore.
>> >
>> >The additional restriction on underscore ('_') in HTTP arises from CGI,
>> >which maps headers to environment variables. For instance, Apache httpd
>> >silently drops headers with anything other than letters, digits, and
>> >hyphen [3] to avoid security issues caused by environment manipulation.
>> >
>> >If I was developing a PSR-7 or similar library, I would be inclined to
>> >drop any header composed only of digits, and issue a diagnostic warning,
>> >so that it wouldn't escalate to a type error later. It certainly doesn't
>> >seem reasonable to change the entire language to work around that
>> >inconvenience.
>> >
>> >[1]  https://www.iana.org/assignments/http-fields/http-fields.xhtml
>> >[2]  https://datatracker.ietf.org/doc/html/rfc3864#section-4.1
>> >[3]  https://httpd.apache.org/docs/trunk/env.html#setting
>> >
>> >Regards,
>> >
>> >--
>> >Rowan Tommins
>> >[IMSoP]
>> >
>> >--
>> >PHP Internals - PHP Runtime Development Mailing List
>> >To unsubscribe, visit:  https://www.php.net/unsub.php
>>
>> I just gave an example of what at the moment can cause an exception in any
>> application that is based on the PSR. It is enough to send the header "0:
>> Farewell to the server". In some cases (for example, as is the case with
>> RoadRunner) - this can cause a physical stop and restart of the server.
>>
>> Just in case, I will repeat my thesis: I cannot imagine that anyone is
>> using this functionality consciously and that it is part of the real logic
>> of the application.
>
>
>You don't have a lot of experience with legacy code then. PHP, particularly
>old PHP (like 4, 5.1 era) was used by a lot of idiots.
>
>I was one of those idiots (Perhaps I still am an idiot - jury is
>deliberating on that but I digress).
>
>Snark aside though, PHP has more than its fair share of self taught
>programmers (again, not trying to be insulting as I am one myself), and
>they do things with the code that veterans and formally trained programmers
>would never think to try, let alone implement.
>
>I guarantee fixing how key handling is done will break something - either
>in the form of code exploiting the weird behavior, or code that is guarding
>against the weird behavior; not to mention any tests that might be written
>- though amateurs rarely write test code (again, speaking from past
>experience I've grown beyond).
>
>
>
>> And fixing this behavior, I believe, will automatically fix many libraries
>> (not necessarily PSR) that do not take this behavior into account.
>>
>>
>
>And blow up who knows how many old code bases - many of which don't have
>unit test suites to discover if there is a break ahead of time. This is
>the sort of BC break that would cause a cliff of users unable to migrate to
>the major version that implements it. A Python 2 vs. 3 style of break.
>
>Even with that all said it may indeed be worth fixing - but this will
>require the same sort of kid gloves approach removing register globals had
>(for the newer folks, there was a time when $_REQUEST["var"] would auto
>populate $var with lovely security snarls). IIRC PHP 3 had register
>globals always on, 4 created a config toggle to turn them off, and PHP 5.0
>turned that toggle off by default, finally PHP 5.3 (6 without unicode more
>or less) removed support for register globals entirely (My memory could be
>off - it's in the changelogs for the curious).
>
>I leave the decision making to the maintainers and contribs who do the
>actual work. Hell, I personally don't even use PHP that much these days
>having gotten a job where I focus on writing Cucumber tests in JavaScript
>that run on node.js. I keep up with PHP and this list though cause one
>never knows what the next job will entail. I just dropped out of lurk mode
>to underscore along with others up thread the massive ramifications of what
>is being proposed. As someone who wrote stupid code I can see this
>breaking, tread lightly. And hell, I don't even know how much of that code
>is still in use since I've changed employers many times since it was
>written. This situation is not unique and can create huge headaches for
>companies running projects on legacy code bases. 
 
 
I absolutely agree with all your arguments. Moreover, I will add one more thing to them: This fix changes the logic of the code, and it is impossible to process it correctly, causing, for example, "depressation issue", which aggravates the situation.

However, the major release (versions of PHP9) implies a breakdown of BC. And this is the very moment when problems in the language can and should be fixed.

Since the mailing list serves to discuss the problem, I can suggest a variant with the temporary introduction of the ini setting "strict_arrays=1", which will be deleted in PHP10, and the value "strict_arrays=0" will cause a deprecation notice like "Please note that you have auto-conversion of array keys is enabled, the behavior of which will be changed in PHP10" in places where such conversion is performed. What do you think about it?
 
--
Kirill Nesmeyanov
 

Andreas Hennings

4 years ago

On Sun, 2 Jan 2022 at 06:20, Michael Morris <tendoaki@gmail.com> wrote:

>
> On Sat, Jan 1, 2022 at 10:47 PM Kirill Nesmeyanov <nesk@xakep.ru> wrote:
>
> >
> > >Суббота, 1 января 2022, 17:41 +03:00 от Rowan Tommins <
> > rowan.collins@gmail.com>:
> > >
> > >On 31/12/2021 00:21, Kirill Nesmeyanov wrote:
> > >> I support this behavior fix because in its current form, due to a
> > similar problem (almost?), all PSR-7 implementations contain bugs that
> > violate RFC7230 (section 3.2:
> > https://datatracker.ietf.org/doc/html/rfc7230#section-3.2 ). Thus,
> > physically, by the standard, all headers can have the name "0" (like «0:
> > value»), but when stored inside implementations, it is converted to a
> > string and a problem arises ($message->getHeaders() //
> > returns array<int|string, string> instead of array<string, string>).

The solution is to cast the keys back to string when reading from the
array, IF the type matters.

foreach ($headers as $k => $values) {
$name = (string) $k;
}

We could introduce an alternative to array_keys() that would do this
automatically, e.g. "array_keys_str()".

> > >
> > >You appear to be technically correct - the RFC defines a header name
> > >only as "token", which implies the following would all be valid HTTP
> > >headers:
> > >
> > >42: The Answer
> > >!: Bang
> > >^_^: Surprised
> > >
> > >In practice, it would be a bad idea to use any of these.
> > >
> > >Every single one of the field names registered with IANA [1] starts with
> > >a letter, and proceeds with only letters, digits, and hyphen ('-'). [The
> > >exception is "*", listed there as "reserved" to specifically prevent its
> > >use conflicting with the wild-card value in "Vary" lists.]
> > >
> > >I'm actually surprised this definition hasn't been updated with
> > >interoperability advice in recent revisions of the standard. I did find
> > >this general advice for internet message headers in RFC 3864 [2]:
> > >
> > > > Thus, for maximum flexibility, header field names SHOULD further be
> > > >  restricted to just letters, digits, hyphen ('-') and underscore ('_')
> > > >  characters, with the first character being a letter or underscore.
> > >
> > >The additional restriction on underscore ('_') in HTTP arises from CGI,
> > >which maps headers to environment variables. For instance, Apache httpd
> > >silently drops headers with anything other than letters, digits, and
> > >hyphen [3] to avoid security issues caused by environment manipulation.
> > >
> > >If I was developing a PSR-7 or similar library, I would be inclined to
> > >drop any header composed only of digits, and issue a diagnostic warning,
> > >so that it wouldn't escalate to a type error later. It certainly doesn't
> > >seem reasonable to change the entire language to work around that
> > >inconvenience.
> > >
> > >[1]  https://www.iana.org/assignments/http-fields/http-fields.xhtml
> > >[2]  https://datatracker.ietf.org/doc/html/rfc3864#section-4.1
> > >[3]  https://httpd.apache.org/docs/trunk/env.html#setting
> > >
> > >Regards,
> > >
> > >--
> > >Rowan Tommins
> > >[IMSoP]
> > >
> > >--
> > >PHP Internals - PHP Runtime Development Mailing List
> > >To unsubscribe, visit:  https://www.php.net/unsub.php
> >
> > I just gave an example of what at the moment can cause an exception in any
> > application that is based on the PSR. It is enough to send the header "0:
> > Farewell to the server". In some cases (for example, as is the case with
> > RoadRunner) - this can cause a physical stop and restart of the server.
> >
> > Just in case, I will repeat my thesis: I cannot imagine that anyone is
> > using this functionality consciously and that it is part of the real logic
> > of the application.

It is not really relevant weather this is used _consciously_.

>
>
> You don't have a lot of experience with legacy code then. PHP, particularly
> old PHP (like 4, 5.1 era) was used by a lot of idiots.
>
> I was one of those idiots (Perhaps I still am an idiot - jury is
> deliberating on that but I digress).

We don't need to assume incompetence.

Any code that deals with arrays _must_ consider this behavior, unless
the array keys are known to be only integers, or only non-integer-like
strings.

One obvious BC break:

What would be the value of $a in the following snippet?

$a = [];
$a['5] = 's';
$a[5] = 'n';
$a[7] = 'n';
$a['7'] = 's';

Currently it would be [5 => 'n', 7 => 's'].
With the "new" behavior, we'd have to decide what happens.
Can keys '5' and 5 coexist? ['5' => 's', 5 => 'n', 7 => 'n', '7' => 's']?
Or would assignment change the key type? [5 => 'n', '7' => 's']?
Or does the initial key type remain, and only the value changes? ['5'
=> 'n', 7 => 's']?

I would argue that the current behavior might still be the best we can
get for a general-purpose structure that can act as a vector or a map
or a mix of both.
The perceived awkwardness is just a result of trying to do everything at once.

Possible solutions:
- Dedicated array-reading methods that cast all keys to string on read.
- New structures, alternative to array, that either allow separate
entries for 5 and '5', or that are restricted to one key type.


--- Andreas

Rowan Collins

4 years ago

On 2 January 2022 03:47:11 GMT, Kirill Nesmeyanov <nesk@xakep.ru> wrote:

>
>I just gave an example of what at the moment can cause an exception in any application that is based on the PSR. It is enough to send the header "0: Farewell to the server". In some cases (for example, as is the case with RoadRunner) - this can cause a physical stop and restart of the server.

Any library where a crafted HTTP request can cause a server shutdown has a bug which needs addressing right now - possibly more than one, actually, as it implies error handling is leaking across request boundaries. A change to the language applied in the next major version would fix this some time around 2025, once people start adopting it. A workaround in the library itself can be applied within weeks.

I already gave a simple solution that such libraries can apply right now, with very little chance of negative impact: sanitise headers more aggressively than the HTTP standard requires, as Apache httpd does, in this case discarding any header containing only digits. This is likely to be about three lines of code inside a loop preprocessing raw headers:

if ( ctype_digit($rawHeaderName) ) {
trigger_error("Numeric HTTP header '$rawHeaderName' has been discarded.", E_USER_WARNING);
continue;
}

If I was the maintainer of such a library, I might consider even stricter validation, considering what seems like an accidentally broad definition in the HTTP spec, and the possibility of an application receiving even more exotic characters if processing raw TCP traffic.


The idea of an array_keys variant or option that forces everything back to string seems like it might be useful (and easy to polyfill for old versions). Changing such a fundamental language behaviour in the hope that it will fix more code than it breaks is just not worth it.

Regards,

--
Rowan Tommins
[IMSoP]

Chase Peeler

4 years ago

On Sun, Jan 2, 2022 at 7:05 AM Rowan Tommins <rowan.collins@gmail.com>
wrote:

> On 2 January 2022 03:47:11 GMT, Kirill Nesmeyanov <nesk@xakep.ru> wrote:
> >
> >I just gave an example of what at the moment can cause an exception in
> any application that is based on the PSR. It is enough to send the header
> "0: Farewell to the server". In some cases (for example, as is the case
> with RoadRunner) - this can cause a physical stop and restart of the server.
>
> Any library where a crafted HTTP request can cause a server shutdown has a
> bug which needs addressing right now - possibly more than one, actually, as
> it implies error handling is leaking across request boundaries. A change to
> the language applied in the next major version would fix this some time
> around 2025, once people start adopting it. A workaround in the library
> itself can be applied within weeks.
>
> I already gave a simple solution that such libraries can apply right now,
> with very little chance of negative impact: sanitise headers more
> aggressively than the HTTP standard requires, as Apache httpd does, in this
> case discarding any header containing only digits. This is likely to be
> about three lines of code inside a loop preprocessing raw headers:
>
> if ( ctype_digit($rawHeaderName) ) {
>      trigger_error("Numeric HTTP header '$rawHeaderName' has been
> discarded.", E_USER_WARNING);
>      continue;
> }
>
> If I was the maintainer of such a library, I might consider even stricter
> validation, considering what seems like an accidentally broad definition in
> the HTTP spec, and the possibility of an application receiving even more
> exotic characters if processing raw TCP traffic.
>
>
> The idea of an array_keys variant or option that forces everything back to
> string seems like it might be useful (and easy to polyfill for old
> versions). Changing such a fundamental language behaviour in the hope that
> it will fix more code than it breaks is just not worth it.
>
>

But "001" casted to 1 will then get casted back to "1" not "001".

> Regards,
>
> --
> Rowan Tommins
> [IMSoP]
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: https://www.php.net/unsub.php
>
>

--
Chase Peeler
chasepeeler@gmail.com

Rowan Collins

4 years ago

On 3 January 2022 15:41:48 GMT, Chase Peeler <chasepeeler@gmail.com> wrote:

>But "001" casted to 1 will then get casted back to "1" not "001".

"001" would not be cast to an integer in this context: https://3v4l.org/gGFHJ

Regards,

--
Rowan Tommins
[IMSoP]

Chase Peeler

4 years ago

On Mon, Jan 3, 2022 at 10:48 AM Rowan Tommins <rowan.collins@gmail.com>
wrote:

> On 3 January 2022 15:41:48 GMT, Chase Peeler <chasepeeler@gmail.com>
> wrote:
> >But "001" casted to 1 will then get casted back to "1" not "001".
>
> "001" would not be cast to an integer in this context:
> https://3v4l.org/gGFHJ
>
>

My mistake, I thought it did get casted. Still, I think you add a lot of
complexity because you'll need to keep track of whether or not a key was a
string or not. There is no guarantee that all numeric keys were originally
a string:
[ "1" => "foo", 2 => "bar"]

And, for the record, I am not in favor of the original proposal either. I
think it's way too big of a BC break like others have said.

> Regards,
>
> --
> Rowan Tommins
> [IMSoP]
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: https://www.php.net/unsub.php
>
>

--
Chase Peeler
chasepeeler@gmail.com

Rowan Collins

4 years ago

On 3 January 2022 16:07:53 GMT, Chase Peeler <chasepeeler@gmail.com> wrote:

>My mistake, I thought it did get casted. Still, I think you add a lot of
>complexity because you'll need to keep track of whether or not a key was a
>string or not. There is no guarantee that all numeric keys were originally
>a string:
>[ "1" => "foo", 2 => "bar"]

I don't think anybody was proposing any such tracking, simply a shorthand for this:

array_map(strval(...), array_keys($array));

In other words, take the keys of any array, which might be a mixture of integers and strings, and convert them all to strings, regardless of how they were originally specified. The result being an array where the values can all be safely passed to a function expecting strings, with strict_types=1 in force.

Regards,

--
Rowan Tommins
[IMSoP]

Nick Wallace

4 years ago

I've sent a few emails to unsubscribe. Please remove me from this list. I'm
tired of these fucking emails about a dead language

On Sat, Jan 1, 2022, 8:41 AM Rowan Tommins <rowan.collins@gmail.com> wrote:

Christoph Becker

4 years ago

On 02.01.2022 at 15:12, Nick Wallace wrote:

> I've sent a few emails to unsubscribe. Please remove me from this list. I'm
> tired of these fucking emails about a dead language

Please send mail to <internals-unsubscribe@lists.php.net> to trigger the
unsubscription process.  See
<http://untroubled.org/ezmlm/ezman/ezman1.html> for details.

Regards,
Christoph

James Titcumb

4 years ago

On Wed, 29 Dec 2021, 15:42 Vincent Langlet, <misterdeviling@gmail.com>
wrote:

>
> I recently discovered that an array was automatically casting
> numeric-string keys to int if it was possible.
>

Having had to track down bugs numerous times over the years, I completely
agree this is unexpected behaviour, and would be in favour of it being
amended. It is quite a BC break indeed, and both Orklah (Psalm) and Ondrej
Mirtes (PHP Stan) agree that the issue is that it's a difficult issue to
identify with static analysis (see
https://twitter.com/OndrejMirtes/status/1478971968636567552 for some
thoughts).

As always with type juggling related discussions in PHP, I expect any RFC
to be contentious and heavily discussed ;)

I expect the main detractors would be the BC breaks, and people might not
think the BC break is worth the pain.

Just thinking out loud, being able to identify an array type might be a way
forward here (using "syntax" already in use by static analysis tools in
docblocks, for familiarity), e.g.

private array<string, mixed> $property;

This might "turn off" the automatic type juggling behaviour, for example.
If the existing strict_types=1 mode is on, this might fail:

$this->property[3] = "hi";

If strict_types=0 or not declared, the key would be cast to a string.

That said, I know there has been contentious discussion on this and the
related topic of generics multiple times, so don't really want to stir up a
hornet's nest there.

Great idea, and I'd love to see this fixed, given the countless hours I've
wasted trying to track down bugs that are a result of this. But I fear that
this would be a difficult one to push through the RFC process, sadly. I
hope I'm proven wrong though!

Thanks
James

Rowan Collins

4 years ago

On 6 January 2022 07:29:58 GMT, James Titcumb <james@asgrim.com> wrote:

>Just thinking out loud, being able to identify an array type might be a way
>forward here (using "syntax" already in use by static analysis tools in
>docblocks, for familiarity), e.g.
>
>private array<string, mixed> $property;
>
>This might "turn off" the automatic type juggling behaviour, for example.
>If the existing strict_types=1 mode is on, this might fail:
>
>$this->property[3] = "hi";
>
>If strict_types=0 or not declared, the key would be cast to a string.

Yes, something similar occurred to me earlier, and I honestly think that some new types like this are the only way this can practically be solved.

>That said, I know there has been contentious discussion on this and the
>related topic of generics multiple times, so don't really want to stir up a
>hornet's nest there.

It's not so much that generics are contentious, but that they're really, really hard to implement within the context of PHP's other features. It gets complicated really quickly when you need to verify parameter and return types, e.g. to decide if a value of type SortedList<ChildClass> meets a parameter constraint List<ParentClass>

Leaving that aside, though, an array type that has certain constraints on assignment might be workable. And a simple pair that only constrain their keys could just be individually named rather than introducing generics.

As for the original suggestion, I think some people are underestimating just how disruptive changing this could be. The really big problem is that there is nothing to deprecate, very little to detect with static analysis tools, and very confusing code if you want to support both versions.

The following would be perfectly valid PHP both before and after the behaviour change:

$a = [];
$a[42] = true;
$a['42'] = false;
$a[ (string)43 ] = true;
$a[ (int)'43' ] = false;

None of those lines is "wrong", in and of itself, so there's nothing to mark deprecated or warn about; but the result in current PHP is [42 => false, 43 => false]; and with the proposed change, the result would presumably be [42 => true, '42' => false, '43' => true, 43 => false]

I can't think of any way one could prepare for this change, other than putting a string cast in front of every array index throughout an entire code base, *and* on every use of array_keys, foreach, etc that *reads* array keys. Unless someone can present a fully working migration plan, this would get a "no" vote from me.

Regards,

--
Rowan Tommins
[IMSoP]

Vincent Langlet

4 years ago

> On 6 January 2022 07:29:58 GMT, James Titcumb <james@asgrim.com> wrote:
> >Just thinking out loud, being able to identify an array type might be a
> way
> >forward here (using "syntax" already in use by static analysis tools in
> >docblocks, for familiarity), e.g.
> >
> >private array<string, mixed> $property;
> >
> >This might "turn off" the automatic type juggling behaviour, for example.
> >If the existing strict_types=1 mode is on, this might fail:
> >
> >$this->property[3] = "hi";
> >
> >If strict_types=0 or not declared, the key would be cast to a string.
>
>
> Yes, something similar occurred to me earlier, and I honestly think that
> some new types like this are the only way this can practically be solved.
>
>

So a solution, which doesn't require generics, would be to introduce
something new like a `dict` as a way to create an `array<string, mixed>`.
`{}` could be used instead of `[]`.

$dict = {'1' => '1', 'key' => 'value'};
\is_array($dict); // true

Arvids Godjuks

4 years ago

On Wed, 29 Dec 2021 at 17:42, Vincent Langlet <misterdeviling@gmail.com>
wrote:

> Hi,
>
> I recently discovered that an array was automatically casting
> numeric-string keys to int if it was possible. For instance, the following
> array:
>
> $a = ['01' => '01', '10' => '10'];
>
> Is not an array with the key '01' and '10' but instead consider the second
> key as an int.
>
> This has some implications like the fact that
> - array_flip(array_flip($a)) !== $a
> - array_search('10', $a) is an int when array_search('01', $a) is still a
> string. Someone using strict types and passing the result to a function
> expecting a string could end with an unexpected crash.
>
> I've created an issue about this
> https://github.com/php/php-src/issues/7845
> but it was recommended to me to send a mail to this mailing list instead.
>
> I don't think this behavior should be considered as "normal" and would like
> to propose to change this for PHP 9, as it's a BC-break. To me it can and
> be considered as a follow-up of
> https://wiki.php.net/rfc/string_to_number_comparison and
> ttps://wiki.php.net/rfc/saner-numeric-strings
> <https://wiki.php.net/rfc/saner-numeric-strings>. This is still a
> "concept"
> since I never code with C and know nothing about the PHP implementation and
> if this change would be possible. Any help is welcome then.
>
> Thanks
>

Hello everyone,

This is indeed a pretty sizeable BC break that probably is going to cause
quite a few headaches for people, but it's also one of those things where I
think it's absolutely worth tackling and fixing it.
I never got hit by this personally, but when I use string keys, I expect
them to be kept as strings and not do magic conversions to an integer.
Can't vote, but you definitely have my full support to fix this one.
Probably would need a depreciation period and some warnings for a version
or two.

--

Arvīds Godjuks
+371 26 851 664
arvids.godjuks@gmail.com
Telegram: @psihius https://t.me/psihius