opis/closure

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:

if($id_start_ci === 'self' || $id_start_ci === 'static' || $id_start_ci === 'parent'){

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)