Unbinding $this of closure is deprecated

[kylekatarnls]

• Laravel Version: 5.8 / 6.0
• PHP Version: 7.4.0 / 8.0.0

Description:

PHP deprecate closure unbinding, so the following line:
https://github.com/laravel/framework/blob/master/src/Illuminate/Support/Traits/Macroable.php#L110

throws:
Unbinding $this of closure is deprecated

since PHP 7.4.0

As having a similar macro system in Carbon, I'm working on possible alternatives.

Do you plan to support PHP 7.4 and 8? And to still support mixin/macro in those new versions?

[derekmd]

Relevant links:

• php/php-src@28cf080#diff-833c0daf9450be2a512d6cecf4351b6c
• https://wiki.php.net/rfc/deprecations_php_7_4#unbinding_this_from_non-static_closures
• https://news-web.php.net/php.internals/106158 (bottom paragraph)

[driesvints]

Hmm. We should try to get this fixed before v6.0 is out since PHP 7.4 drops in fall this year.

[kylekatarnls]

@driesvints The option I took for next Carbon release is muting the errors with @. Of course it just postpones the problem to a later version. But it would give a few more time.

[driesvints]

@kylekatarnls yeah I'd opt to actually provide a fix already if we can.

[kylekatarnls]

Here is for the quick fix: #29418

But some testing utils throws other deprecation notices with PHP 7.4 in Travis-CI. I will try to fix them.

No fix for Laravel 5.8?

[yaayes]

Using error control symbol @ is a bad practice, it can prevent other errors from being shown and/or handled, and you'll have a more time just to know what the heck is happening.
From PHP docs:

Warning: Currently the "@" error-control operator prefix will even disable error reporting for critical errors that will terminate script execution. Among other things, this means that if you use "@" to suppress errors from a certain function and either it isn't available or has been mistyped, the script will die right there with no indication as to why.

IMO, and from PR #29418 you shouldn't use @ before a call_user_func_array, because we'll never know if the closure or the function typed by the developer is working and does not have any typos or any kind of errors!

[driesvints]

@kylekatarnls I've closed your temp fix pr. Let's try to fix the actual problem before we release 6.0

[driesvints]

No fix for Laravel 5.8?

No, we can't make breaking changes here.

[driesvints]

Okay so here's the gist: everything will still work as is except for static macro calls. When you perform a static macro call which is a closure you'll need to make sure it's also marked the closure as static. Only the unbinding of $this for non-static closures is deprecated. The error message above is actually a bit misleading.

So basically the following won't work anymore:

FooClass::macro('tryStatic', function () {
    return 'foo';
});

FooClass::tryStatic();

Because the closure is non-static you can't unbind $this anymore. But if you do the following, everything will work as expected:

FooClass::macro('tryStatic', static function () {
    return 'foo';
});

FooClass::tryStatic();

Notice the static keyword. The closure is marked as static now and $this cannot be called from within anymore.

I've sent in a PR here: #29482

[faliure]

In Macroable.php:110 we're inside __call, I don't see how we'd get that error there: return call_user_func_array($macro->bindTo($this, static::class), $parameters);. It's binding to $this, in a non-static context, $this cannot be null.

Maybe the error comes from line 84? That'd make more sense unless I'm missing something big.

The way I see it, there are a few options:

1. Remove the special handling (the conditional) for closures altogether in __callStatic.
   Cons: can't use static:: within those macros.
   I found only one instance where a macro uses static and it's in a test: tests/Support/SupportMacroableTest.php:45. It's still a breaking change, though.

2. Make sure static macros are declared as such (static function X(){ ...}).
   Pros: it's the right way to do it (Symphony did this as well)
   Cons: it's a breaking change

The great solution would be to be able to change the context (for static::) without touching the $newthis. However, it seems like they didn't think of that when deprecating unbinding non-static Closures...