Fix GH-8932: Wrong closure scope class reported for static methods

[cmb69]

ReflectionFunction::getClosureScopeClass() needs to report the
called_scope, if there is one.

Unfortunately, it is not yet possible to retrieve that value from the
closure object, so we need to introduce a getter.

---

Frankly, I don't know wether the called_scope can ever be NULL. If not, we won't need the fallback to closure_func->common.scope. And I'm not sure whether it might be preferable to catch the called_scope during ReflectionFunction::__construct(), so we won't need the getter.

[mvorisek]

can this target PHP 8.0?

[mvorisek]

ReflectionMethod returns null

https://3v4l.org/hXBYu

[cmb69]

I think this is to be expected; since there is no closure, there is no closure scope class. But in any way, ReflectionFunctionAbstract::getClosureScopeClass() needs proper documentation.

[arnaud-lb]

Zend/tests/closure_042.phpt is failing, but I think that the new output is the expected one

Frankly, I don't know wether the called_scope can ever be NULL. If not, we won't need the fallback to closure_func->common.scope. And I'm not sure whether it might be preferable to catch the called_scope during ReflectionFunction::__construct(), so we won't need the getter.

We could avoid the getter and the common.scope fallback by using the object's get_closure handler. Since this is what the engine uses to determine the called scope, this will ensure that ReflectionFunction::getClosureScopeClass() remains consistent with the engine.

[cmb69]

Zend/tests/closure_042.phpt is failing, but I think that the new output is the expected one

That is what confuses me. That test explicitly assumes that the dummy sope[sic] is Closure:

php-src/Zend/tests/closure_042.phpt

Line 10 in 63912b5

var_dump($rm->getClosureScopeClass()->name); //dummy sope is Closure

Wrong test expectation? Maybe, but changing that might break code which relies on that behavior.

[nicolas-grekas]

Wrong test expectation?

I think so yes.

might break code which relies on that behavior

Every bugfix has this potential :) The current behavior isn't useful.

[nicolas-grekas]

can this target PHP 8.0?

I second that, this is a bugfix to me.

[cmb69]

We could avoid the getter and the common.scope fallback by using the object's get_closure handler.

Ah, right. Thank you! I've did this now.

I'm still somewhat reluctant to target PHP-8.0, because of that closure_042.phpt.

[arnaud-lb]

Looking more closely, I'm wondering whether getClosureScopeClass() is supposed to return the called_scope or the scope.

In my understanding, the static keyword and $this refer to called_scope, visibility checks during execution of the function are made against scope, and the self keyword refers to scope.

If getClosureScopeClass() is supposed to return the scope, both closure_042.phpt and the behavior observed in #8932 was right:

• The scope in closure_042.phpt is Closure after the call to bindTo()
• In Wrong closure scope class reported for static methods #8932 the scope of B::b (which really is A::b) is A

Also, the only other place where we use the word scope in the API is in the newScope parameter of Closure::bindTo() and Closure::bind(). It affects the scope of the closure, while the newThis parameter affects the called_scope.

I think that we should introduce a new function to expose the called_scope.

[nicolas-grekas]

I can testify that yes, for named closures, the current behavior has introduced very subtle and unexpected bugs, impossible to anticipate without intimate understanding of the engine, and more importantly impossible to fix.

See eg twigphp/Twig#3722

[arnaud-lb]

We can think of of scope as self, and called_scope as static.

getClosureScopeClass() returns the self of a Closure: https://3v4l.org/ktqXB

I agree that we need a way to expose called_scope / static. E.g. a new method getClosureCalledScopeClass() that would return Foo for Closure::createFromCallable("Foo::bar")).

Maybe we could add this to 8.0 and 8.1 as well.

[nicolas-grekas]

The thing is: the "self" for a named closure serves no purpose. This is never a useful info since nothing can be built out of it. And reciprocally for anonymous closure, there is no "static" scope. For this reason, the current fix looks the correct one to me.

[bwoebi]

Given class A { public static function F() { return "A"; } }

The difference is https://3v4l.org/SuAuh:

(function() { var_dump(static::F()); })->bindTo(new A, NULL)();
vs
(function() { var_dump(self::F()); })->bindTo(new A, NULL)();
vs
(function() { var_dump(self::F()); })->bindTo(new A, "A")();

Basically: "self" scope of closure = second argument to bindTo, static scope of closure = scope of $this, if $this is given, otherwise static = self.

The only way to get the self scope of the closure for anonymous closures is (new \ReflectionFunction($c))->getClosureScopeClass(). As such, changing the behaviour of closure_042.phpt is wrong.

Now, this static scope of closure = scope of $this rule is only valid for anonymous closures. For named closures, the static scope of the closure may be independent, and is currently not accessible. As such I support @arnaud-lb idea to add a new function for this case in PHP-8.0+.

@nicolas-grekas Why is "self" of a named closure not useful information? How would you then get the defining class of a named closure (of a static method)?

[nicolas-grekas]

In all cases where I've needed this, I first have a branch based on getClosuseThis. When I enter this branch, I'd use ReflectionMethod($this, 'method')->getDeclaringClass(), so it's very natural to do the same for static methods.

If we're up to adding a new method, may I suggest toCallable()? This would turn back named closures into either a string or an array, and this would return anonymous closures as is. That would be super helpful (I've already written logic for this a couple of times in several libs.)

[bwoebi]

@nicolas-grekas yes, if you have a $this (i.e. a non-static method), then this is viable. Hence I was explicitly saying (of a static method). Maybe you haven't encountered an use case for Closures of static methods.

I would simply add a new function getting the ReflectionClass of the static scope. Writing your own toCallable logic based on that would then be easy.

[nicolas-grekas]

My point is: I don't see a use case where getting the declaring class of a static method is useful, in the context of named closures.
Actually, I think there are bugs lying out there that don't know about this behavior, because the only moment where it matters is when constructing a named closure out of a static method on a class that has __callStatic. This is a super niche edge case, yet this is the one Laravel relies on, and how I spotted this issue (see twigphp/Twig#3722)

[cmb69]

Thanks for the explanation, @arnaud-lb and @bwoebi. LSB is such a nice feature. ;) Anyhow, I'm closing this PR, as it is obviously wrong.

I agree that a new method makes sense, but it seems to me that adding a public method to ReflectionFunctionAbstract() constitutes a BC break.

[nicolas-grekas]

I'm sorry but I don't understand the rationale. What's the "scope" of a closure if it's not what static::class returns? The current behavior is just buggy...

[bwoebi]

@nicolas-grekas It's self::class. I think there is a bit a naming problem... Internally we have scope and called_scope. The former being self, the latter static. I think the naming has been applied 1:1 to the reflection method name here and confused you probably.

[cmb69]

The super verbose documentation of ReflectionFunctionAbstract::getClosureScopeClass() may add to the confusion. ;)

[nicolas-grekas]

Got it thanks. I submitted #9299 as a follow up.