Closed Wirone closed 2 months ago
Hey, this is a tough one. We can't simply change it because there might be people out today relying on concrete implementations being returned when asked for an interface. So it'd be a BC break.
We usually pride ourselves of precise type inference. This one is done by https://github.com/phpstan/phpstan-symfony/blob/1.4.x/src/Type/Symfony/ServiceDynamicReturnTypeExtension.php.
It's great that PHPStan can know the exact service types and use them in code. It's also great that it forces "only useful" @var
annotations - meaning it's going to tell you the annotation is not narrowing down the type. I stand firmly behind both of these features.
On one hand, I understand what you ask for. On the other hand, I don't see an easy solution for you.
I'm not even sure if Symfony has some special handling for class names or not. With a config like this:
services:
# Interface
Foo\ClientFactory: '@Foo\ApplicationClientFactory'
# Implementation
Foo\ApplicationClientFactory: ~
Would Symfony throw an exception if Foo\ApplicationClientFactory
did not implement Foo\ClientFactory
interface? Or are all of these just arbitrary names for services that happen to be same as class/interface names?
Also please note you're getting this error:
PHPDoc tag
@var
with type Foo\ClientFactory is not subtype of type Foo\ApplicationClientFactory.
Only because you have enabled reportWrongPhpDocTypeInVarTag
: https://phpstan.org/config-reference#reportwrongphpdoctypeinvartag
Without this setting you'd only get this error against native types, like: https://phpstan.org/r/1c76b925-aecf-4b2c-95c7-84cfd1317fb1
Thank you for the quick and descriptive response.
Hey, this is a tough one. We can't simply change it because there might be people out today relying on concrete implementations being returned when asked for an interface. So it'd be a BC break.
Is it possible to achieve this with some configuration flag / feature toggle?
Would Symfony throw an exception if
Foo\ApplicationClientFactory
did not implementFoo\ClientFactory
interface? Or are all of these just arbitrary names for services that happen to be same as class/interface names?
I was pretty sure Symfony's cache:clear
command would fail on it, but it does not (at least not in 6.4 we use) 😩. I've changed the aliased service to random class that does not implement the interface, and cache was compiled successfully. @nicolas-grekas is it something that should be addressed on Symfony's side or maybe was improved in 7.x?
The role of the DI component is not to validate your config. The PHP engines does that. You might want to use the lint:container command for that.
One more point I want to make here - you can completely avoid this problem if you stop using DI container as a service locator (which is an anti-pattern). If you really want to have clean code, you should ask for ClientFactory
in the constructor instead. PHPStan is not going to complain about that, and it's going to keep ClientFactory
as the type.
Thanks @nicolas-grekas for fast response. Yeah, the lint:container
command reports such mismatch:
$ bin/console lint:container
[ERROR] Invalid definition for service "Foo\Bar": argument 2 of "Foo\Bar::__construct()" accepts "Foo\ClientFactory", "Foo\Baz" passed.
Thankfully we have this in our CI setup so we won't let such invalid DI definitions sneak into our codebase 🙂.
One more point I want to make here - you can completely avoid this problem if you stop using DI container as a service locator (which is an anti-pattern). If you really want to have clean code, you should ask for
ClientFactory
in the constructor instead. PHPStan is not going to complain about that, and it's going to keepClientFactory
as the type.
@ondrejmirtes I completely agree and am aware of it, but as I stated in the issue - this is legacy part of our huge app and it's impossible to get rid of direct container usage just like that. We work on it step by step, but there's too much of a work around that.
Invalid definition for service
That doesn't actually sound like the error I'd expect.
What does lint:container
say about these situations?
services:
# Interface
Foo\NonexistentClass: '@Foo\ApplicationClientFactory'
And:
services:
# Interface
Foo\InterfaceThatExistsButApplicationClientFactoryDoesNotImplementIt: '@Foo\ApplicationClientFactory'
There is not any error for both, as service name can be anything, FQNs are just used for convenience (if I remember correctly service name is used as a fallback when class
is not provided explicitly). The error would be encountered if Foo\NonexistentClass
or Foo\InterfaceThatExistsButApplicationClientFactoryDoesNotImplementIt
would have been used as arguments for other service definition and Foo\ApplicationClientFactory
service would not match that service's signature. Exactly what happened for Foo\Bar
above, which expected Foo\ClientFactory
but service alias was resolved to the service based on class that does not implement such interface. IMHO "Invalid definition for service" is correct here, even though the root cause lays in the invalid alias for one of the dependencies of that service.
So PHPStan can't assume anything about names or class hierarchy here. The DIC is simply not designed for that.
You can name your services anything. It'd be wrong to infer service fetched by "ClientFactory" name to implement "ClientFactory" interface and not go more specific with the service type.
You can override what phpstan-symfony does here with this extension: https://apiref.phpstan.org/1.11.x/PHPStan.Type.ExpressionTypeResolverExtension.html
But instead what I'd recommend is some kind of wrapper class for your container where you can write your own getByClass
method with a simple generic signature that will make PHPStan infer the type you want. From class-string<T>
in a parameter to T
in the return type.
So PHPStan can't assume anything about names or class hierarchy here. The DIC is simply not designed for that.
Hmmmm, I am not sure about that. Container's approach aside, PHPStan's extension gets service's name and resolves the type using the cached container. I believe PHPStan could verify if service's name is a FQN of an interface, and if the resolved type implements this interface, then interface could be returned instead of more specific type of an implementation. Are there any technical difficulties to achieve this?
PHPStan could technically give a meaning to something that doesn't have said meaning, but why would it though? It'd just cause issues being reported by people that expect the opposite to happen.
I gave you some ideas for solutions on your side already. But this isn't in phpstan-symfony's interest to change.
Anyway, this is likely my last response on this issue, I already spent a lot of effort and mental energy on it so don't expect any more from me. Thanks for understanding.
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
We encountered issue today (PHPStan 1.11.6 with bleeding edge + Symfony extension 1.4.5) that I consider violating the Liskov Substitution Principle. Consider simplified code:
then, when we do something like:
we get an error:
which does not make sense. We fetch
ClientFactory
from DI container, it's aliased toApplicationClientFactory
implementation that satisfies the interface, so of course interface is not a subtype, it's super type.I don't want more specific type here, as the whole idea of interface is interchangeability. In the code, when I fetch
ClientFactory
(interface) I don't care what implementation comes from DI container, I only want this particular contract, because I don't want to encounter problems when DI alias is changed to other implementation. In our caseApplicationClientFactory
has more methods that implementedClientFactory
interface, I don't want developers to be able to use these methods because PHPStan allows it (as it knows the aliased service), rather the other way around - I would expect that static analysis reports usage of methods not defined in the interface.I believe it's a bug and PHPStan should not narrow the type here. Similar here, I believe I should be allowed to restrict the contract to interface instead of relying on extended implementation.
FYI: we don't use
@var
when we don't need, in this case it's only for IDE which does not have autocompletion forClientFactory
fetched from the container (sinceget()
's return type is?object
). We use dependency injection mostly, but unfortunately in legacy part of the code there is a lot of direct interaction with DI container.Originally posted by @Wirone in https://github.com/phpstan/phpstan-symfony/issues/350#issuecomment-2208459313