-
Notifications
You must be signed in to change notification settings - Fork 96
Narrow type for Extension::getConfiguration if class exists #384
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
577564f
to
93f1df9
Compare
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.
You could do Configuration::class
here.
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.
Will do. I think I based it on some other test fixture, which also used a string. But if ::class
works and is preferred I will obviously change it 👍🏻
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.
Please solve this todo, or remove it.
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.
If you can provide some pointers on how to determine this I will try to implement it :) I haven't been able to find some example on how to determine / inspect such information.
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.
You can get ClassReflection from ReflectionProvider and ask about constructor from there.
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.
That was the easy part which I did find out already :) Anyway, I'm now iterating over the variants => parameters => checking isOptional
on every one of them. Not sure if there is an easier way to do this though.
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 can solve this for union types too. Don't return early if it's not 1, iterate over the class names and create a union.
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.
True. But I don't think it's common. I believe that ->getConfiguration()
is only called on $this
(/by the extension itself), or by the Symfony framework. So my reasoning was that handling it for other cases (/unions) makes the code more complex while not being of any benefit.
But I'll update the code so it handles that as well 👍🏻
The base extension class automatically creates a Configuration instance when a Configuration class exists in the namespace of the extension. But PHPStan obviously doesn't understand this behaviour and always assumes that `getConfiguration()` returns `ConfigurationInterface|null` meaning that the default pattern to get and parse the configuration reports an error. I.e.: ```php namespace Foo; class SomeExtension extends Extension { public function load(array $configs, ContainerBuilder $container): void { $configuration = $this->getConfiguration($configs, $container); $config = $this->processConfiguration($configuration, $configs); } } ``` results in an error because `processConfiguration()` doesn't accept `ConfigurationInterface|null`. But when a `Configuration` class exists in the same namespace as the `Extension` class (so `Foo\Extension`) an instance of it is returned. This `DynamicReturnTypeExtension` overrides the return type of `Extension::getConfiguration()` so it automatically narrows the return type in case `getConfiguration()` is not overriden and a `Configuration` class exists. So that in the given example `getConfiguration()` doesn't return `ConfigurationInterface|null` anymore but `Foo\Configuration` and there is no error on calling `processConfiguration()`.
93f1df9
to
5d1f883
Compare
Thank you!
The base extension class automatically creates a Configuration instance when a Configuration class exists in the namespace of the extension. But PHPStan obviously doesn't understand this behaviour and always assumes that
getConfiguration()
returnsConfigurationInterface|null
meaning that the default pattern to get and parse the configuration reports an error.I.e.:
results in an error because
processConfiguration()
doesn't acceptConfigurationInterface|null
. But when aConfiguration
class exists in the same namespace as theExtension
class (soFoo\Extension
) an instance of it is returned.This
DynamicReturnTypeExtension
overrides the return type ofExtension::getConfiguration()
so it automatically narrows the return type in casegetConfiguration()
is not overriden and aConfiguration
class exists. So that in the given examplegetConfiguration()
doesn't returnConfigurationInterface|null
anymore butFoo\Configuration
and there is no error on callingprocessConfiguration()
.