On 26-5-2022 20:23, Dan Ackroyd wrote:
Hey Julie,
On Thu, 26 May 2022 at 12:45, Juliette Reinders Folmer
<php-internals_nos...@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