Everything

php

Don't silence chr function arguments error

php.internals

Gabriel Caruso

7 years ago

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

Sara Golemon

7 years ago

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

Nikita Popov

7 years ago

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

Sara Golemon

7 years ago

On Mon, Apr 29, 2019 at 2:37 AM Nikita Popov <nikita.ppv@gmail.com> wrote:

Gabriel Caruso

7 years ago

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 <nikita.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 :)