False positive on the "static" keyword
mnapoli opened this issue · 2 comments
Original issue: PHP-DI/PHP-DI#719
In this example:
function () {
$a = static function ($retries) {
return 750 * $retries;
};
return new Foo($a);
}
The $isUsingScope
is set to true
here:
closure/src/ReflectionClosure.php
Line 571 in 039b5cc
It seems like a false positive with a static
keyword being used like static::someMethod()
or static::$property
?
Hi, Matthieu! This should be fixed in v3.5.5, but I must warn you that the isScopeRequired
method is not used anymore by our library, so basically is deprecated.
We advice developers to use only the classes and methods which are documented and are part of the library's public API. The ReflectionClosure
class is not documented anywhere because we always have consider it to be an internal class. We didn't mark it as being internal due to the fact that in the past we have provided an Analyzer
class for the defunct SuperClosure package. Since SuperClosure is now abandoned, maybe is time to properly mark the ReflectionClosure
class as being internal.
The purpose of this library is to help developers to serialize and unserialize closures. We didn't design this library to be used as a static analysis tool like Psalm or PHPStan. Using opis/closure
outside the scope it was designed for, will potentially cause a lot of frustration for the developers.
Thank you, that's fixed upstream by requiring this new version.
Regarding the deeper thing here (using ReflectionClosure
), I understand and agree that's problematic.
Would it make sense, and be doable, to extract that reflection logic into its own package? That way opis/closure
keeps being only about serialization, and "reflection" use cases are covered by the lower-level package?
For you, as maintainer, you would get the freedom you want: making the class internal lets you break BC as you wish, without impacting opis/closure -> with a separate package, you would be able to iterate as quickly/break BC (with new major releases) without impacting opis/closure.
(closing to keep the issue tracker up to date, but feel free to continue the discussion)