[Discussion] Expand deprecation notice scope for partially supported callables

1 year ago by Juliette Reinders Folmer — view source

unread

After the prior discussion about the same topic:
https://externals.io/message/117342, I have created an RFC to expand the
scope of the deprecation notices being thrown for the deprecated
partially supported callables to include is_callable() and the callable
type in PHP 8.2.

With this email I'm opening the two week discussion period for this RFC.
All points raised in the prior discussion are already included in the RFC.

https://wiki.php.net/rfc/partially-supported-callables-expand-deprecation-notices

I look forward to your feedback.

Smile,
Juliette

1 year ago by Larry Garfield — view source

unread

After the prior discussion about the same topic:
https://externals.io/message/117342, I have created an RFC to expand the
scope of the deprecation notices being thrown for the deprecated
partially supported callables to include is_callable() and the callable
type in PHP 8.2.

With this email I'm opening the two week discussion period for this RFC.
All points raised in the prior discussion are already included in the RFC.

https://wiki.php.net/rfc/partially-supported-callables-expand-deprecation-notices

I look forward to your feedback.

Smile,
Juliette

I didn't follow the earlier discussion in much detail, but the is_callable() deprecation seems fine to me.

For the callable type declaration, I'm not opposed but is it redundant with the existing deprecation? When would you pass a callable to something and not end up calling it anyway, which would trigger the existing deprecation? (Meaning in practice you'd always get 2 deprecations, which are not necessarily better than one.)

--Larry Garfield

1 year ago by Claude Pache — view source

unread

Le 12 mai 2022 à 23:11, Larry Garfield larry@garfieldtech.com a écrit :

For the callable type declaration, I'm not opposed but is it redundant with the existing deprecation? When would you pass a callable to something and not end up calling it anyway, which would trigger the existing deprecation? (Meaning in practice you'd always get 2 deprecations, which are not necessarily better than one.)

Although such a callable is probably intended to be called at some point, it is not necessarily called immediately, and it may be easier to track the source of it when you trigger the deprecation earlier. Example:

public function registerErrorHandler(callable $onerror) {
    $this->onError[] = $onerror;
}

(Of course, this argument also holds for is_callable().)

—Claude

1 year ago by Juliette Reinders Folmer — view source

unread

Le 12 mai 2022 à 23:11, Larry Garfield larry@garfieldtech.com a écrit :

For the callable type declaration, I'm not opposed but is it redundant with the existing deprecation? When would you pass a callable to something and not end up calling it anyway, which would trigger the existing deprecation? (Meaning in practice you'd always get 2 deprecations, which are not necessarily better than one.)
Although such a callable is probably intended to be called at some point, it is not necessarily called immediately, and it may be easier to track the source of it when you trigger the deprecation earlier. Example:
public function registerErrorHandler(callable $onerror) {
     $this->onError[] = $onerror;
}
(Of course, this argument also holds for is_callable().)

—Claude

Exactly as Claude says and to quote the RFC:

While it will be common to use the partially supported callable in a
callback function call within the function with the type declaration,
this may not always be the case, especially in frameworks where
callbacks can be registered to be executed later in a limited set of
circumstances and those circumstances may not be met by default.

Without a deprecation notice, those “/limited circumstances
callbacks/” may not be discovered as needing updating in time for PHP 9.0.

Ref:
https://wiki.php.net/rfc/partially-supported-callables-expand-deprecation-notices#adding_a_deprecation_notice_for_callable

Smile,
Juliette

1 year ago by Juliette Reinders Folmer — view source

unread

After the prior discussion about the same topic:
https://externals.io/message/117342, I have created an RFC to expand
the scope of the deprecation notices being thrown for the deprecated
partially supported callables to include is_callable() and the
callable type in PHP 8.2.

With this email I'm opening the two week discussion period for this
RFC. All points raised in the prior discussion are already included in
the RFC.

https://wiki.php.net/rfc/partially-supported-callables-expand-deprecation-notices

I look forward to your feedback.

Seeing how there isn't any new discussions on this RFC and presuming
that means that the previous discussion touched all concerns, I propose
to open the vote on this RFC tomorrow.

RFC:
https://wiki.php.net/rfc/partially-supported-callables-expand-deprecation-notices

If anyone still wants to know a little more about the background, you
may want to have a listen to Derick's podcast about this RFC:
https://phpinternals.news/101

Smile,
Juliette

1 year ago by Dan Ackroyd — view source

unread

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

1 year ago by Juliette Reinders Folmer — view source

unread

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

1 year ago by Dan Ackroyd — view source

unread

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();
}

$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.

1 year ago by Dan Ackroyd — view source

unread

an incorrect name

Apologies for writing your name incorrectly. That should of course
have been addressed to Juliette.

cheers
Dan
Ack

1 year ago by Juliette Reinders Folmer — view source

unread

Hi Dan,

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().

...

This is actually not an error in the RFC, but an anonymized code sample
based on real-life code patterns found in popular packages.

While the syntaxes are not technically equivalent, functionally, they
yield the same result, which is why this code pattern is not uncommon.

It is exactly this type of code pattern - and the associated
misconception leading to this code existing in real life code bases -,
which started the initial discussion about the lack of deprecation
notices for is_callable() and led to this RFC.

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.

IMO that's a false argument as deprecation notices are not intended for
people who don't intend to upgrade their PHP version.

If there is no intention to upgrade to PHP 9.0, the much simpler
solution would be to set error_reporting to E_ALL & ~E_DEPRECATED.
Alternatively, a custom error handling could be registered which filters
out all, or only a selection of, deprecation notices.

Deprecation notices are for those people who do want to upgrade to PHP
9.0 once it comes out and want to prepare their code base to be ready.

And for those people, the callable type not throwing a deprecation
notices means that for the "registered, but rarely called" callables,
they will go from nothing to a Fatal Error once PHP 9.0 comes round,
which is exactly what this RFC tries to address.

Either way, I do agree that this potential objection should be heard, so
I have added an extra section to the "Discussion" section of the RFC in
which I have summarized our discussion (so far) about this:
https://wiki.php.net/rfc/partially-supported-callables-expand-deprecation-notices#these_additional_deprecation_notices_will_be_very_noisy

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.
Fair enough, I will split the vote and have updated the RFC to show this.

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.
Just pointing out, the same thing can happen with the callable type -
a callable being registered for an emergency (like a disk running out of
space), but not being called for months. This example edge case is not
limited to is_callable().

I hope the RFC update addresses your concerns sufficiently. If there are
no further objections, I will open the vote.

Smile,
Juliette

1 year ago by Guilliam Xavier — view source

unread

Hi Dan, just an error in this part:

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();
}

This throws an Error on static::methodName(...) [if not callable],
before even passing it to is_callable(): https://3v4l.org/m29BK

And yes, code should probably better use a same variable for both the
check and the call, but that could also degrade DX (IDE
autocompletion, static analysis) especially if calling with
arguments...

(the other parts of your message have been answered by Juliette)

Regards,

--
Guilliam Xavier