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

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

Merged
ondrejmirtes merged 1 commit into phpstan:1.3.x from RobertMe:configuration-type
Mar 5, 2024

Conversation

Copy link
Contributor

@RobertMe RobertMe commented Feb 29, 2024

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.:

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().

@RobertMe RobertMe force-pushed the configuration-type branch 2 times, most recently from 577564f to 93f1df9 Compare February 29, 2024 14:46
public function load(array $configs, ContainerBuilder $container): void
{
\PHPStan\Testing\assertType(
'PHPStan\Type\Symfony\Extension\WithConfiguration\Configuration',
Copy link
Member

@ondrejmirtes ondrejmirtes Mar 5, 2024

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.

Copy link
Contributor Author

@RobertMe RobertMe Mar 5, 2024

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 👍🏻

return new NullType();
}

// TODO: return NullType if configuration class has constructor with required arguments
Copy link
Member

@ondrejmirtes ondrejmirtes Mar 5, 2024

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.

Copy link
Contributor Author

@RobertMe RobertMe Mar 5, 2024

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.

Copy link
Member

@ondrejmirtes ondrejmirtes Mar 5, 2024

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.

Copy link
Contributor Author

@RobertMe RobertMe Mar 5, 2024

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.

): ?Type
{
$extensionType = $scope->getType($methodCall->var);
$classes = $extensionType->getObjectClassNames();
Copy link
Member

@ondrejmirtes ondrejmirtes Mar 5, 2024

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.

Copy link
Contributor Author

@RobertMe RobertMe Mar 5, 2024

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()`.
@ondrejmirtes ondrejmirtes merged commit d8a0bc0 into phpstan:1.3.x Mar 5, 2024
Copy link
Member

Thank you!

RobertMe reacted with thumbs up emoji

@RobertMe RobertMe deleted the configuration-type branch April 8, 2024 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ondrejmirtes ondrejmirtes Awaiting requested review from ondrejmirtes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

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