-
Notifications
You must be signed in to change notification settings - Fork 95
Support for TreeBuilder::getRootNode() #126
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
@ruudk Hi, I need a sample of analyzed code and what is this extension trying to achieve.
@ondrejmirtes I'm trying to let PHPStan understand this: https://github.com/symfony/symfony/blob/60118f14aa33165407b06ed59225a612049f715f/src/Symfony/Bundle/DebugBundle/DependencyInjection/Configuration.php#L30-L55
If you want to reproduce it:
git clone https://github.com/symfony/symfony.git
cd symfony
composer install
composer req --dev phpstan/phpstan:^0.12.71
vendor/bin/phpstan analyse --level=8 src/Symfony/Bundle/DebugBundle/DependencyInjection/Configuration.php
------ -----------------------------------------------------------------------
Line Configuration.php
------ -----------------------------------------------------------------------
33 Call to an undefined method
Symfony\Component\Config\Definition\Builder\NodeDefinition::children(
).
58 Call to an undefined method
Symfony\Component\Config\Definition\Builder\NodeDefinition::children(
).
------ -----------------------------------------------------------------------
[ERROR] Found 2 errors
6202bc8 to
5a73db0
Compare
5a73db0 to
a0c1c7b
Compare
@ondrejmirtes It's green ✅ 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should take a more "static" approach. You can also easily cause an internal error when analyzing code, for example when the constructor call has wrong arguments.
We should have some hardcoded map of what the type => root node types are. And I guess the TreeBuilderType constructor 2nd argument should be ObjectType.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the code and I feel it's now much simpler. What do you think?
I think this is how it's done everywhere.
Perfect, thank you!
After seeing #60 stuck I decided to give it a shot...
This is very naive, and it assumes the
TreeBuilderis always of typearray.If I am able to find the second
$typeparameter to thenew TreeBuilder()call then I can make the return type ofgetRootNodedynamic based on that.@ondrejmirtes Is this even possible?
The issue with NodeDefinition::end() is that it can return
nullwhen you've reached the root. The current extension removes the nullability in favor of DX. But it's also very naive. No idea if that can be done dynamically.Hope to get some advice on how to proceed.... 🙏