Don't silence chr function arguments error

  105489
April 28, 2019 23:05 carusogabriel34@gmail.com (Gabriel Caruso)
Hello Internals.

I'd like to discuss a small change, but that demands a discussion, for a
function in PHP's Core: https://php.net/chr <http://php.net/chr>.

Currently, if you pass an argument that is not an integer, it will simply
return an empty string: https://3v4l.org/FF2nA. In case you pass more than
1 argument, it will complain about "Wrong number of arguments":
https://3v4l.org/Yrr43.

The proposal of https://github.com/php/php-src/pull/4080 is to relly these
checks to ZPP, making the first argument mandatory to an integer, and make
the error message for extra arguments consistent with the rest of the core
functions, e.g. "Warning: chr() expects parameter 1 to be int, string
given".

The idea is to merge in PHP-7.4, but if there's a consensus that this
should go into PHP 8 only, so it is :)

Best,

-- Gabriel Caruso
  105490
April 29, 2019 01:43 pollita@php.net (Sara Golemon)
On Sun, Apr 28, 2019 at 6:05 PM Gabriel Caruso <carusogabriel34@gmail.com>
wrote:

> Currently, if you pass an argument that is not an integer, it will simply > return an empty string: https://3v4l.org/FF2nA.
Not entirely accurate. It outputs a one-character string (the null byte).
> In case you pass more than > 1 argument, it will complain about "Wrong number of arguments": > https://3v4l.org/Yrr43. > >
> The proposal of https://github.com/php/php-src/pull/4080 is to relly these > checks to ZPP, making the first argument mandatory to an integer, and make > the error message for extra arguments consistent with the rest of the core > functions, e.g. "Warning: chr() expects parameter 1 to be int, string > given". > > I'm actually perfectly in favor of raising a proper warning here. I noticed the odd behavior when I was porting this function to HHVM and IIRC
I raised a discussion about it and the bottom line was "Let's not do this because X", and I couldn't possibly tell you what X was. :( The thread is out there somewhere though.
> The idea is to merge in PHP-7.4, but if there's a consensus that this > should go into PHP 8 only, so it is :) > > Technically a BC break, so 8.0 would be the preferred branch IMO.
-Sara
  105499
April 29, 2019 07:37 nikita.ppv@gmail.com (Nikita Popov)
On Mon, Apr 29, 2019 at 3:44 AM Sara Golemon <pollita@php.net> wrote:

> On Sun, Apr 28, 2019 at 6:05 PM Gabriel Caruso <carusogabriel34@gmail.com> > wrote: > > > Currently, if you pass an argument that is not an integer, it will simply > > return an empty string: https://3v4l.org/FF2nA. > > > Not entirely accurate. It outputs a one-character string (the null byte). > > > > In case you pass more than > > 1 argument, it will complain about "Wrong number of arguments": > > https://3v4l.org/Yrr43. > > > > > > > > The proposal of https://github.com/php/php-src/pull/4080 is to relly > these > > checks to ZPP, making the first argument mandatory to an integer, and > make > > the error message for extra arguments consistent with the rest of the > core > > functions, e.g. "Warning: chr() expects parameter 1 to be int, string > > given". > > > > I'm actually perfectly in favor of raising a proper warning here. I > noticed the odd behavior when I was porting this function to HHVM and IIRC > I raised a discussion about it and the bottom line was "Let's not do this > because X", and I couldn't possibly tell you what X was. :( > The thread is out there somewhere though. > > > > The idea is to merge in PHP-7.4, but if there's a consensus that this > > should go into PHP 8 only, so it is :) > > > > Technically a BC break, so 8.0 would be the preferred branch IMO. >
As this would be a TypeError on 8.0, I think it might make sense to introduce it in 7.4 where this is still a warning (and should keep the current "\0" return value in that case -- just make it throw a warning). Nikita
  105511
April 29, 2019 16:01 pollita@php.net (Sara Golemon)
On Mon, Apr 29, 2019 at 2:37 AM Nikita Popov ppv@gmail.com> wrote:

> > The idea is to merge in PHP-7.4, but if there's a consensus that this >> > should go into PHP 8 only, so it is :) >> > >> > Technically a BC break, so 8.0 would be the preferred branch IMO. >> > > As this would be a TypeError on 8.0, I think it might make sense to > introduce it in 7.4 where this is still a warning (and should keep the > current "\0" return value in that case -- just make it throw a warning). > > Sounds good to me.
  105648
May 8, 2019 19:33 carusogabriel34@gmail.com (Gabriel Caruso)
Em seg, 29 de abr de 2019 às 13:01, Sara Golemon <pollita@php.net> escreveu:

> On Mon, Apr 29, 2019 at 2:37 AM Nikita Popov ppv@gmail.com> wrote: > >> > The idea is to merge in PHP-7.4, but if there's a consensus that this >>> > should go into PHP 8 only, so it is :) >>> > >>> > Technically a BC break, so 8.0 would be the preferred branch IMO. >>> >> >> As this would be a TypeError on 8.0, I think it might make sense to >> introduce it in 7.4 where this is still a warning (and should keep the >> current "\0" return value in that case -- just make it throw a warning). >> >> Sounds good to me. >
If there's no objection, I'll be merging this change during the weekend :)