Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:117814
To: internals@lists.php.net
References: <627D3BCF.7020903@adviesenzo.nl> <628F685B.8090909@adviesenzo.nl>
 <CA+kxMuQb3OXV-zSQBBiVXHgSt+u06MBOZgw-xqgMEJ05_i+ebw@mail.gmail.com>
Cc: Dan Ackroyd <Danack@basereality.com>
Message-ID: <6291DBBC.8020403@adviesenzo.nl>
Date: Sat, 28 May 2022 10:22:20 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:38.0) Gecko/20100101
 Thunderbird/38.7.0
MIME-Version: 1.0
In-Reply-To: <CA+kxMuQb3OXV-zSQBBiVXHgSt+u06MBOZgw-xqgMEJ05_i+ebw@mail.gmail.com>
Content-Type: multipart/alternative;
 boundary="------------090805030501010409050206"
Subject: Re: [PHP-DEV] [Discussion] Expand deprecation notice scope for
 partially supported callables
From: php-internals_nospam@adviesenzo.nl (Juliette Reinders Folmer)

--------------090805030501010409050206
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit

On 26-5-2022 20:23, Dan Ackroyd wrote:
> Hey Julie,
>
> On Thu, 26 May 2022 at 12:45, Juliette Reinders Folmer
> <php-internals_nospam@adviesenzo.nl> wrote:
>> I propose to open the vote on this RFC tomorrow.
> Before you open the vote, please could you split the voting into two,
> one for the is_callable, and one for the callable type check?
>
> Rowan wrote in https://news-web.php.net/php.internals/117670:
>> is that passing "99 red balloons" to an "int"
>> parameter raised an E_NOTICE in PHP 7.x, so a "callable" parameter
>> raising an E_DEPRECATED should be fine.
> There's one issue.
>
> When "99 red balloons" is coerced to an int, that is done once, and
> then any further int type check will pass. For callables, it's pretty
> common to pass them down a stack of code, e.g. similar to:
>
> function foo(callable $fn, $data)
> {
>      $fn($data);
> }
>
> function bar(callable $fn, $data)
> {
>      return foo($fn);
> }
>
> function quux(callable $fn, $data)
> {
>      return bar($fn, $data);
> }
>
>
> function main(array $data)
> {
>      $fn = get_callable_from_input();
>      if (is_callable($fn) === false) {
> // give some error.
>          return;
>      }
>
>      quux($data);
> }
>
> As far as I understand it, this code would give a deprecation notice
> for each call level, which seems quite undesirable. Particularly if
> the callable is being used in a loop.
>
> Also, without a patch it's hard to guess what performance impact this
> would have. I doubt it would be huge, but it probably wouldn't be zero
> either. Performance wouldn't matter for is_callable, as that is
> typically only done once per callable, but callable type checks are
> done continuously.
>
> cheers
> Dan
> Ack

Hiya Dan,

I admit, I puzzled over this for a little and wanted to take the time to 
respond properly before opening the vote, so I'm delaying the start of 
the vote until beginning of the upcoming week.

In my first draft of the RFC I actually had two votes, but once it 
shaped up this was brought down to one vote.

Reason being, that for the practical implications of fixing the 
deprecations, there is no difference between the two.

In your example, you suggest a variable being passed between functions 
all using the `callable` type. The same can be encountered with 
functions without the `callable` type, but using manual defensive coding 
via `is_callable()` - typical example: a collection of callbacks being 
checked before being registered to a callback stack and being checked 
again before actually being called as the stack may have been 
manipulated from outside.

Yes, having each of those instances  throw a deprecation notice will be 
more noisy, especially if the same deprecated callback is used in a loop 
and yes, this will have a (small) performance impact while these 
callbacks aren't fixed yet, but the same *one* fix - at the point where 
the problematic callback is defined - will fix them all in one go, so 
the amount of fixes to be made does not change, but the chances of 
_identifying_ all the places were a fix is _needed_ is greatly improved.

In that sense, it is no different from the "99 red balloons" case when 
those issues are solved at the _source_ of the problem.
Patching the "99 red balloons" by applying `(int)` casts at the point 
the deprecation is thrown, will lead to a codebase full of type casts, 
while the underlying problem - the origin of the text string - is not 
addressed (and in the case of the callbacks, the origin is the only 
place they _can_ realistically be solved).

Considering the above, do you still feel that splitting the vote in two 
is the best way to go ?

Smile,
Juliette


--------------090805030501010409050206--