- 
  Notifications
 
You must be signed in to change notification settings  - Fork 95
 
Add DefinitionConfigurator stub for rootNode ArrayNodeDefinition return type #392
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add DefinitionConfigurator stub for rootNode ArrayNodeDefinition return type #392
Conversation
6e4a841 to
 6add490  
 Compare
 
 6add490 to
 6b19299  
 Compare
 
 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.
Ah, I had to add the ArrayNodeDefinition stub. CI is passing now :)
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?
Sure. ArrayNodeDefinition is always returned by DefinitionConfigurator::rootNode because the DefinitionConfigurator TreeBuilder instance is constructed with the default array type:
But if someone passes a different $type than array this assumption does not hold.
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
- DefinitionConfigurator accepts TreeBuilder.
 DefinitionConfigurator::rootNode()calls$this->treeBuilder->getRootNode().TreeBuilder::getRootNode()returns$this->root.$this->rootis assigned like this:$this->root = $builder->node($name, $type)->setParent($this);- Therefore the type depends on how 
TreeBuilderis constructed.$typeis passed in the constructor and can be different fromarray. 
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?
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.
Fixes #278
The DefinitionConfigurator::rootNode method always returns an ArrayNodeDefinition, but without this PR when we use this code:
We get the following error: