Roave / BetterReflection

:crystal_ball: Better Reflection is a reflection API that aims to improve and provide more features than PHP's built-in reflection API.
MIT License
1.19k stars 131 forks source link

Getting parameter from method with new initializer throws #1311

Closed DanielBadura closed 1 year ago

DanielBadura commented 1 year ago

Again, coming originally from BackwardsCompatibilityCheck. Running it results right now to some Skipped checks due to an error in BetterReflection. Here the output from BC-Check:

Error: Unable to compile initializer in method Patchlevel\EventSourcing\Projection\Projectionist::boot() in file /src/Projection/Projectionist.php (line 11)
Error: Unable to compile initializer in method Patchlevel\EventSourcing\Projection\Projectionist::run() in file /src/Projection/Projectionist.php (line 16)
Error: Unable to compile initializer in method Patchlevel\EventSourcing\Projection\Projectionist::teardown() in file /src/Projection/Projectionist.php (line 18)
Error: Unable to compile initializer in method Patchlevel\EventSourcing\Projection\Projectionist::remove() in file /src/Projection/Projectionist.php (line 20)
Error: Could not locate constant Patchlevel\EventSourcing\Projection\ProjectorStatus::New while trying to evaluate constant expression in method Patchlevel\EventSourcing\Projection\ProjectorStore\ProjectorState::__construct() in file /src/Projection/ProjectorStore/ProjectorState.php (line 14)
Error: Unable to compile initializer in method Patchlevel\EventSourcing\Projection\MetadataProjectorResolver::__construct() in file /src/Projection/MetadataProjectorResolver.php (line 17)
Error: Unable to compile initializer in method Patchlevel\EventSourcing\Projection\DefaultProjectionist::__construct() in file /src/Projection/DefaultProjectionist.php (line 23)
Error: Unable to compile initializer in method Patchlevel\EventSourcing\Projection\DefaultProjectionist::boot() in file /src/Projection/DefaultProjectionist.php (line 28)
Error: Unable to compile initializer in method Patchlevel\EventSourcing\Projection\DefaultProjectionist::run() in file /src/Projection/DefaultProjectionist.php (line 75)
Error: Unable to compile initializer in method Patchlevel\EventSourcing\Projection\DefaultProjectionist::teardown() in file /src/Projection/DefaultProjectionist.php (line 117)
Error: Unable to compile initializer in method Patchlevel\EventSourcing\Projection\DefaultProjectionist::remove() in file /src/Projection/DefaultProjectionist.php (line 148)
Error: Unable to compile initializer in method Patchlevel\EventSourcing\Projection\SyncProjectorListener::__construct() in file /src/Projection/SyncProjectorListener.php (line 14)

So I tried to get an reproducer for this and came up with this TestCase:

    public function testClassWithNewInitializer(): void
    {
        $php = <<<'PHP'
            <?php
            class Foo {}
            class Bar {
                public function __construct(Foo $foo = new Foo()) {}
            }
        PHP;

        $reflector       = new DefaultReflector(new StringSourceLocator($php, $this->astLocator));
        $classReflection = $reflector->reflectClass('Bar');
        foreach ($classReflection->getMethod('__construct')->getParameters() as $parameter) {
            $parameter->getDefaultValue();
        }
    }

Which leads to this error:

vendor/bin/phpunit --filter testClassWithNewInitializer
PHPUnit 9.5.25 #StandWithUkraine

Random Seed:   1669196177

E                                                                   1 / 1 (100%)

Time: 00:00.190, Memory: 56.00 MB

There was 1 error:

1) Roave\BetterReflectionTest\Reflection\ReflectionClassTest::testClassWithNewInitializer
Roave\BetterReflection\NodeCompiler\Exception\UnableToCompileNode: Unable to compile initializer in method Bar::__construct() in file "" (line 4)

/app/src/NodeCompiler/Exception/UnableToCompileNode.php:107
/app/src/NodeCompiler/CompileNodeToValue.php:62
/app/vendor/nikic/php-parser/lib/PhpParser/ConstExprEvaluator.php:146
/app/vendor/nikic/php-parser/lib/PhpParser/ConstExprEvaluator.php:101
/app/src/NodeCompiler/CompileNodeToValue.php:136
/app/src/Reflection/ReflectionParameter.php:282
/app/src/Reflection/ReflectionParameter.php:376
/app/test/unit/Reflection/ReflectionClassTest.php:2830

ERRORS!
Tests: 1, Assertions: 0, Errors: 1.

This is due to https://github.com/Roave/BetterReflection/blob/6.5.x/src/NodeCompiler/CompileNodeToValue.php#L61-L63.

I will try to provide a patch for it.

kukulich commented 1 year ago

This is valid and expected in BetterReflection.

Ocramius commented 1 year ago

Yup, we don't allow getting values for initializer expressions, since they can trigger autoloading, and avoiding autoloading is the primary purpose of this library.

DanielBadura commented 1 year ago

Should I open another issue then for BC-Check?

DanielBadura commented 1 year ago

Also the one error in between the new initialize errors is about a default enum value. This should be possible to solve, right?

Ocramius commented 1 year ago

Yeah, I think the @throws in roave/backward-compatibility-check is not handled.

I suggest raising a PR directly, perhaps.

Ocramius commented 1 year ago

Also the one error in between the new initialize errors is about a default enum value. This should be possible to solve, right?

Fetching an enum still causes it to autoload, heh

DanielBadura commented 1 year ago

Yeah, I think the @throws in roave/backward-compatibility-check is not handled.

I suggest raising a PR directly, perhaps.

If i would know exactly what to do, sure. Already looking into it..

Fetching an enum still causes it to autoload, heh

True...

kukulich commented 1 year ago

Fetching an enum still causes it to autoload, heh

https://github.com/patchlevel/event-sourcing/blob/2.1.x/src/Projection/ProjectorStore/ProjectorState.php#L14

No, I think this should be resolved without autoload.

@DanielBadura Can you try different enum case? There may be problem with the name New.

DanielBadura commented 1 year ago

Tried it already here https://github.com/patchlevel/event-sourcing/pull/331/commits/8384f5b5667b364941a97834d3ee850020cb570b

But there the "base" was still having the enum with New so could also possibly that why it was still failing.

kukulich commented 1 year ago

My bad, @Ocramius is right. Enum::SOME_CASE returns instance, so it cannot be resolved. It looks like a constant access, but it's not.

DanielBadura commented 1 year ago

Yeah, getting the string value might be possible but this would not be the real value passed to the method. So i guess this is not possible without autoloading and could also be handled somehow in roave/backward-compatibility-check so that it will not produce errors there.