Don't silence chr function arguments error

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

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

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

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.

Em seg, 29 de abr de 2019 às 13:01, Sara Golemon pollita@php.net escreveu:

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