Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:117824
MIME-Version: 1.0
References: <627D3BCF.7020903@adviesenzo.nl> <628F685B.8090909@adviesenzo.nl>
 <CA+kxMuQb3OXV-zSQBBiVXHgSt+u06MBOZgw-xqgMEJ05_i+ebw@mail.gmail.com> <6291DBBC.8020403@adviesenzo.nl>
In-Reply-To: <6291DBBC.8020403@adviesenzo.nl>
Date: Sun, 29 May 2022 16:34:49 +0100
Message-ID: <CA+kxMuT0XWyL6NjEZKfUH62SmEZiiau7R7Sa4jZYuJm9iP-Twg@mail.gmail.com>
To: Juliette Reinders Folmer <php-internals_nospam@adviesenzo.nl>
Cc: internals@lists.php.net
Content-Type: text/plain; charset="UTF-8"
Subject: Re: [PHP-DEV] [Discussion] Expand deprecation notice scope for
 partially supported callables
From: Danack@basereality.com (Dan Ackroyd)

Hi Julie,

On Sat, 28 May 2022 at 09:22, Juliette Reinders Folmer
<php-internals_nospam@adviesenzo.nl> wrote:
>
> 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.

Cool.

Actually, I've just realised there is an error in the code in the RFC,
which might be based on a misconception caused by how terrible
callables are. In the code:

if (is_callable('static::methodName')) {
    // For valid callbacks, this call will be executed in PHP 8.x,
    // but will no longer execute in PHP 9.x.
    static::methodName();
}

The string based callable 'static::methodName' is not equivalent to
the syntax* based callable static::methodName().

Using the string version consistently, the equivalent code would be:

if (is_callable('static::methodName')) {
    call_user_func('static::methodName', []);
}

which for 8.2 gives the message 'Deprecated: Use of "static" in
callables is deprecated in %s on line %d'. btw trying to call
('static::methodName')(); gives the error message 'Uncaught Error:
Class "static" not found in %s:%d' which is part of the consistency
cleanup done by the previous RFC.

Using the syntax version, the equivalent code that would compatible
with PHP < 8.1

if (is_callable(static::class . '::methodName')) {
    static::methodName();
}

Or if support for less than PHP 8.1 can be dropped, using the first
class callable syntax
https://wiki.php.net/rfc/first_class_callable_syntax :

if (is_callable(static::methodName(...))) {
    static::methodName();
}

or

$fn = static::methodName(...);
if (is_callable($fn)) {
    $fn();
}

Passing the callable round by getting the closure from
static::methodName(...) is probably the safest way of referencing this
type of callable.

None of the syntax based ways of referring to the callable are
deprecated or going to be removed in the foreseeable future.


> for the practical implications of fixing the deprecations,
> there is no difference between the two.

People don't have to fix their code. Deprecations can sit there for as
long as the user likes. If a company decides that paying RedHat for
long term PHP 8.2 support, then they may choose to never fix these
deprecation warning and just silence them instead.

Which leads to a difference of, the deprecation notice when checking
with is_callable and using the callable can be suppressed reasonably
easily:

@is_callable('static::methodName')
@call_user_func('static::methodName', []);

And that's a reasonably sane** thing to do. But the deprecation notice
when passing callables around could happen across many pieces of code,
and there's not a good way of suppressing them, unless you just turn
off deprecation notices entirely.

With the deprecation notice where a user is checking, and a
deprecation notice where it is used, I don't see any value in extra
deprecation notices where the callable is being passed around.

> do you still feel that splitting the vote in two is the best way to go ?

Yes, due to the deprecation notices on type-checks when calling
functions have a higher pain-to-utility that in the is_callable check.

Just guessing, I think the previous RFC thought a deprecation notice
on is_callable isn't needed, as there will be a deprecation notice
when the callable is used. But that probably didn't account for people
being able to mix'n'match callable syntaxes, where is is_callable
check, and the actual use of the callable, are not the same callable.

I also like the deprecation notice on is_callable, as that notice will
be 'closer' to where the bad callable is coming from, so would be
easier to reason about. There's also a rare edge-cases where someone
has a callable that is only called in emergencies (like a disk running
out of space) and so might not have that happen for months. Having the
deprecation on is_callable would help those edge-cases a little.

cheers
Dan
Ack

* Is "syntax based callable" the right name? Better suggestions welcome.

** compared to some stuff I've seen/written.