Everything

php

Home » Project » Newsgroups » php.internals » [Discussion] Expand deprecation notice scope for partially supported callables »

[Discussion] Expand deprecation notice scope for partially supported callables

php.internals

Unnamed Person

4 years ago

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

Larry Garfield

4 years ago

On Thu, May 12, 2022, at 11:54 AM, Juliette Reinders Folmer wrote:

> 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

Claude Pache

4 years ago

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

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

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

—Claude

Unnamed Person

4 years ago

On 12-5-2022 23:30, Claude Pache wrote:

>
>> 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:
>
> ```php
> 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

Unnamed Person

4 years ago

On 12-5-2022 18:54, Juliette Reinders Folmer wrote:

> 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

Dan Ackroyd

4 years ago

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

Unnamed Person

4 years ago

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

Dan Ackroyd

4 years ago

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.

Dan Ackroyd

4 years ago

On Sun, 29 May 2022 at 16:34, Dan Ackroyd <Danack@basereality.com> wrote:

>
> *an incorrect name*

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

cheers
Dan
Ack

Unnamed Person

4 years ago

Hi Dan,

On 29-5-2022 17:34, Dan Ackroyd wrote:

> 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

Guilliam Xavier

4 years ago

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