Skip to content

[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

Closed

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented May 7, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

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

image

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,
  • etc.

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:

  • by extending @param, we preserve compat with current docblock parsers
  • we also leverage the capacity to target a specific argument, which is a requirement here
  • we leverage IDEs that typically generate @param annotations for you already

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

/**
 * @param BusInterface $commandBus @bus.command A bus to dispatch commands.
 */

@linaori
Copy link
Contributor

linaori commented May 7, 2018

While this will definitely enhance DI in certain conditions (parameters primarily), I'm afraid that the @service_id will be overlooked quite a bit. Right now, if I see a type-hinted docblock with @param while the signature in the constructor is already present, I will simply remove it. This might result in over-looked bugs because "it's only a comment". Don't get me wrong, I love annotations... It's just totally not what you expect the @param to be used for.

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.

Copy link
Member

@javiereguiluz javiereguiluz left a 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;
Copy link
Member

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

@linaori
Copy link
Contributor

linaori commented May 7, 2018

@javiereguiluz I'm not that familiar with docblocks, but if I recall correctly, it should be {@see ...} for nested annotations. Correct me if I'm wrong 😄

@ogizanagi
Copy link
Contributor

ogizanagi commented May 7, 2018

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

About 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. [...]

We could debate a lot about that I guess. But it looks more like a pretext than a real argument to me. 😕
In case of the ServiceSubcriberInterface at least, the ids are really what the code expects to find in the injected service locator, so it's really part of the code. Here we're still referencing DI services directly to expect the proper service being injected, without any control.
So, in order to reverse a bit my "this proposition is writing code against DI config" predicate above, shouldn't this at least fail and throw an exception when trying to autowire a class with @param BusInterface $commandBus @bus.command and the corresponding service id does not exist?

You may have understood I'm not really convinced right now, but curious to see where this is going :)

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented May 9, 2018

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.
We'll also need to make this work with controller actions and service subscribers, which are the two other places that allow referencing services, each with a dedicated way to do so.

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented May 9, 2018

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

@Oliboy50
Copy link

I think that this would conflict with this PR PHP-CS-Fixer/PHP-CS-Fixer#3734

@ogizanagi
Copy link
Contributor

It should only remove superfluous @param when there isn't a description associated to it. So it shouldn't be an issue for this.

@Taluu
Copy link
Contributor

Taluu commented May 10, 2018

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

@Addvilz
Copy link

Addvilz commented Jul 3, 2018

@nicolas-grekas are there some specific annotation parsers that are known to fail on new annotations? For me reusing @param seems a bit... ugly? Not to mention ambiguous and potentially dangerous, as we have no idea what comments are there already @param. Plus to add support for these annotations, editors might need to do significant amount of changes, to implement, for example highlighting etc.

Maybe it would pay to actually add a new, generic param to identify services, in this case, maybe something similar to @Named from Java? This would have numerous advantages.

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 @param

@sroze
Copy link
Contributor

sroze commented Jul 30, 2018

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 @param docs for this as it has already a huge history of pros/cons and associated benefits.

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

@linaori
Copy link
Contributor

linaori commented Jul 30, 2018

Even with docblocks, I'm not sure if it stores argument docblocks at property or argument level though.

@Addvilz
Copy link

Addvilz commented Jul 30, 2018

Having a proper annotation mechanism in PHP would be even better :)

@sroze
Copy link
Contributor

sroze commented Jul 30, 2018

Even with docblocks, I'm not sure if it stores argument docblocks at property or argument level though.

Correct but you can be pretty sure that it will be easy to deal with regexes.

Having a proper annotation mechanism in PHP would be even better :)

Such a thing could drive property annotations in PHP, who knows 🤷‍♂️

@nicolas-grekas
Copy link
Member Author

Closing in favor of #28234

@nicolas-grekas nicolas-grekas deleted the autowire-params branch August 20, 2018 18:23
@nicolas-grekas nicolas-grekas removed this from the next milestone Nov 1, 2018
@nicolas-grekas nicolas-grekas added this to the 4.2 milestone Nov 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants