Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Conversation

@andyexeter
Copy link

@andyexeter andyexeter commented May 23, 2024

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 andyexeter force-pushed the definition-configurator-root-node-array-type branch 3 times, most recently from 6e4a841 to 6add490 Compare May 23, 2024 15:06
@andyexeter andyexeter force-pushed the definition-configurator-root-node-array-type branch from 6add490 to 6b19299 Compare May 23, 2024 15:18
Copy link
Author

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.

Copy link
Author

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

Copy link
Member

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?

Copy link
Author

andyexeter commented May 23, 2024
edited
Loading

Copy link
Member

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

Copy link
Author

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

Copy link
Member

  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.

Copy link
Author

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?

Copy link
Member

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.

andyexeter reacted with thumbs up emoji

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[6.1] [AbstractBundle] Call to an undefined method NodeDefinition::children()

AltStyle によって変換されたページ (->オリジナル) /