-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[DI] Extend semantic of @param
to describe intent and use it when autowiring services and parameters
#27172
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
While this will definitely enhance DI in certain conditions (parameters primarily), I'm afraid that the I like the idea because it's an out-of-the-box solution, but it still comes down to DI by annotation, which has been rejected in the past. The only difference is that it's not a custom annotation, but alters behavior of an existing annotation, which is not a good idea imo. |
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.
At first I wasn't convinced by this proposal, but now I like it. Defining this info in the @param
attribute looks wrong to me ... but now I see it's the best place to do this. It's the only "natural" place to do that. Otherwise we'd need to create a new annotation for services (rejected in the past for lots of reasons) or do some weird hack.
Super minor comment about an edge case: if the PHPdoc description of a param starts with @
but it's not a service, what will happen? Example:
@param BusInterface $commandBus @see App\Bus\Commands
} | ||
|
||
foreach ($reflectionClass->getMethods() as $reflectionMethod) { | ||
$r = $reflectionMethod; |
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, let's not use 1-letter var names 🙏 Thanks!
$r
-> $method
or $reflectedMethod
$m
-> $methodName
$p
-> $parameter
or $argument
or $methodParam
@javiereguiluz I'm not that familiar with docblocks, but if I recall correctly, it should be |
While bindings are writing DI config against code, to me this proposition is writing "code" against DI config (but I guess it depends the POV).
We could debate a lot about that I guess. But it looks more like a pretext than a real argument to me. 😕 You may have understood I'm not really convinced right now, but curious to see where this is going :) |
018ad42
to
ac8bc39
Compare
…utowiring services and parameters
ac8bc39
to
2d1cd7f
Compare
Status: needs work I'm putting this PR on-hold so that instead of targetting ids/parameters, we could leverage "semantics", as defined in #27207. |
For the record and to answer @iltar's question, my idea is to allow referencing only "semantics" from these annotations. This would make it clear that we're not targetting a specific service, but only those that convey a generic and meaningful name. That would also allow to enforce the type of the target at compile time, and allow building nice error messages (eg "you cannot target service foo because it's not listed for FooInterface, here are the ones that are possible here: ..."). |
I think that this would conflict with this PR PHP-CS-Fixer/PHP-CS-Fixer#3734 |
It should only remove superfluous |
I know it's been mentionned, but I'm kinda against that, as @ogizanagi said : it looks like you're coding against the DI. If you want to use the object outside, you have docblocs which are... superfluous and doesn't bring anything... Adding this in docblock support is the same as having the proper di config, except that your code stays unaware of DI. Which should satisfy some principes of SOLID (here, it kinda violates IMO the dependency inversion...) |
@nicolas-grekas are there some specific annotation parsers that are known to fail on new annotations? For me reusing Maybe it would pay to actually add a new, generic param to identify services, in this case, maybe something similar to Think something like this: /**
* @param Client $myFooBarClient
* @param string $param
* @param OtherService $bar totally autowired, no @named tags
* @named Client @mycompany.connectors.foo.main_connector
* @named string %some.param.maybe%
*/
public function __construct(Client $myFooBarClient, string $param, OtherService $bar)
{
} Yes, a bit verbose. But for me, much better than mangling with |
I really like the idea of adding such a concept and "semantics" is a name I could buy in. Though, I'm really not fond of the idea of using I'd prefer something that simulates Java-like annotations on method properties like the following: public function __construct(
/* @Semantics("event_bus") */ MessageBusInterface $eventBus,
/* @Semantics("video_filesystem") */ FlysystemInterface $videoFilesystem
) {
// ...
} Disclaimer: I didn't dig in the faisibility of this but AFAIK PHP's reflection suite does not allow to get such comments (while it does for the method docblock). |
Even with docblocks, I'm not sure if it stores argument docblocks at property or argument level though. |
Having a proper annotation mechanism in PHP would be even better :) |
Correct but you can be pretty sure that it will be easy to deal with regexes.
Such a thing could drive property annotations in PHP, who knows 🤷♂️ |
Closing in favor of #28234 |
This PR may come as a surprise to many. Please take time to read and understand the present description before engaging in the discussion. It's been written on the way to/back from Grand Canyon. What an inspiration :)
As we're gaining experience with autowiring, we know that it works well when one single service implements some interface. But it's also pretty common for several interfaces to have many different services implementing them. Comes to my mind:
CacheItemPoolInterface
, for different cache pools,BusInterface
, for different messenger buses,FilesystemInterface
from Flysystem, for different storages,ContainerInterface
, for local registries of dedicated services,In similar situations, users are required to do configuration. The best solution we currently have for doing so is by using bindings. While there is nothing wrong with doing configuration, I've met several ppl that trick autowiring in order to workaround doing configuration. We almost merged a PR doing so. The trick is clever, but it's a dead end, as it puts one's code out of SOLID principle.
IMHO, it is our responsibilty to provide a solution that encourages ppl to stay on the safe side of maintainability.
It turns out the core issue is that by type-hinting e.g.
BusInterface
somewhere, we still don't give enough hint to autowiring to allow it to select which service should be passed. The issue can be fixed at the semantic level by hinting we're seeking for a command bus.This PR proposes to add this hint using a service identifier in an extended
@param
annotation.This provides several benefits:
@param
, we preserve compat with current docblock parsers@param
annotations for you alreadyAbout using service identifiers: I used to consider them as pure ref-targets with no inherent semantic; i.e. random strings to target objects in the container. That's what they are for many ids. But I recently realized that service ids also convey a generic semantic that is broader than the container itself. A canonical example for this is the "logger" service. This id is more than a random string you can reference. It's also a promise you're asking for: getting an object that belongs to the "logger" domain. Seen this way, service ids can be considered as string conveying generic semantic, thus they can fit an annotation without leading to any sort of coupling (e.g. none to Symfony's DI).
Note that defining services using DI is not what this PR is about: autowired services already exist. They "just" need to be made to work. Autowiring's job is to fill the holes (the arguments) that are missing a value. #23758, #21103, #836 were all PRs that belong to this group and that we rightfully rejected IMHO.
There has been several tentative issues and PRs that share some bits with the approach presented here: #21376, #22467, #20738. The exact syntax used in this PR has even been proposed in a comment previously.
Using annotations has a desired property: locality. The class consuming a dependency is the right place to express the way the dep is going to be used ("as a command bus", "as a storage for user pictures", etc.)
The added semantic is useful to hint the reader doing manual configuration. And it is useful to hint the autoriwing logic when doing hybrid configuration. That's how this PR leverages it.
Here is an example: