phpstan / phpstan-symfony

Symfony extension for PHPStan
MIT License
698 stars 89 forks source link

Add DefinitionConfigurator stub for rootNode ArrayNodeDefinition return type #392

Closed andyexeter closed 3 months ago

andyexeter commented 3 months ago

Fixes #278

The DefinitionConfigurator::rootNode method always returns an ArrayNodeDefinition, but without this PR when we use this code:

use Symfony\Component\Config\Definition\Configurator\DefinitionConfigurator;

class ExampleBundle extends AbstractBundle
{
    public function configure(DefinitionConfigurator $definition): void
    {
        $definition->rootNode()
            ->children()
            ->end()
        ;
    }

We get the following error:

Call to an undefined method Symfony\Component\Config\Definition\Builder\NodeDefinition::children().
andyexeter commented 3 months ago

I'm not sure why the build is failing

Seems to be an issue with result cache. Running make phpstan with an empty result cache locally causes the same error, but running it with a filled result cache returns no errors.

andyexeter commented 3 months ago

Ah, I had to add the ArrayNodeDefinition stub. CI is passing now :)

ondrejmirtes commented 3 months ago

According to comment in code (https://github.com/symfony/symfony/blob/1f90bc30c32b375ed2cdc3234f56d26b8b99cb53/src/Symfony/Component/Config/Definition/Builder/TreeBuilder.php#L33) this isn't always ArrayNodeDefinition. Can you elaborate on that?

andyexeter commented 3 months ago

Sure. ArrayNodeDefinition is always returned by DefinitionConfigurator::rootNode because the DefinitionConfigurator TreeBuilder instance is constructed with the default array type:

https://github.com/symfony/symfony/blob/1f90bc30c32b375ed2cdc3234f56d26b8b99cb53/src/Symfony/Component/Config/Definition/Configuration.php#L36

https://github.com/symfony/symfony/blob/1f90bc30c32b375ed2cdc3234f56d26b8b99cb53/src/Symfony/Component/Config/Definition/Builder/TreeBuilder.php#L26

ondrejmirtes commented 3 months ago

But if someone passes a different $type than array this assumption does not hold.

andyexeter commented 3 months ago

The TreeBuilder is constructed within Symfony’s config component so the $type value cannot be changed.

The rootNode method of DefinitionConfigurator will always return an ArrayNodeDefinition because that’s how its TreeBuilder is constructed. End developers cannot change the construction of the TreeBuilder

ondrejmirtes commented 3 months ago

1) DefinitionConfigurator accepts TreeBuilder. 2) DefinitionConfigurator::rootNode() calls $this->treeBuilder->getRootNode(). 3) TreeBuilder::getRootNode() returns $this->root. 4) $this->root is assigned like this: $this->root = $builder->node($name, $type)->setParent($this); 5) Therefore the type depends on how TreeBuilder is constructed. $type is passed in the constructor and can be different from array.

andyexeter commented 3 months ago

But theDefinitionConfigurator is only ever constructed in Configuration::getTreeBuilder, which doesn't pass a type.

I see your point though, it could be constructed elsewhere. Would a dynamic return type extension work here? Similar to TreeBuilderGetRootNodeDynamicReturnTypeExtension?

ondrejmirtes commented 3 months ago

Hey, if you're really confident the stub should look like this, it should be easy to convince Symfony to update their return type.

But I don't think your assumption is correc,t and that's why I'm closing this PR. In my opinion the current behaviour is correct and you shouldn't rely on ArrayNodeDefinition being always returned by the DefinitionConfigurator::rootNode() method.