Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DependencyInjection] add ServiceSubscriberTrait#27077
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| </call> | ||
| <callmethod="addGlobalIgnoredName"> | ||
| <argument>service</argument> | ||
| <!-- dummy arg to register class_exists as annotation loader only when required--> |
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.
no need to make multiple dummy registration
| $returnType =$returnType->getName(); | ||
| if (!class_exists($returnType) && !interface_exists($returnType)) { |
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.
the second call could avoid triggering the autoloader, as it was already triggered by the first one
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.
Can you explain? I don't understand what should be done instead.
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 (!class_exists($returnType) && !interface_exists($returnType, false)) {
As class_exists already triggered the autoloader, there is no need to trigger it for interfaces. If the interface exists, it will already have been loaded by the class_exists check.
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.
I see, thanks!
nicolas-grekas left a comment
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.
Cool, thank you for starting this!
I think we need additional logic to deal with inheritance (i.e. call the parent method if any)
| $services =array(); | ||
| foreach ((new \ReflectionClass(static::class))->getMethods()as$method) { | ||
| if (false ===strpos($method->getDocComment(),'@service')) { |
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.
how would it look like if we were to remove this condition?
instead, we could just make all services optional ($services[$method->getName()] = '?'.$returnType; )
I feel like it could work quite nicely in practice.
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.
I tried this change and it seems to work but can't help thinking their may be side effects I can't think of...
One thing that comes to mind is developers wouldn't get immediate feedback if their services aren't wired correctly.
Thoughts?
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.
Not having to declare anything creates a great experience.
About the error, I'm not sure how it would/should behave, we should try and figure out.
| $returnType =$returnType->getName(); | ||
| if (!class_exists($returnType) && !interface_exists($returnType,false)) { |
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.
this can also be removed if the map contains only optional services :)
| continue; | ||
| } | ||
| $services[$method->getName()] =$returnType; |
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.
so:$services[$method->getName()] = '?'.$returnType;
| $this->container =$container; | ||
| } | ||
| protectedfunctionservice(string$id) |
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.
no need for this method, ppl should just have to write$this->container->get(__FUNCTION__);
(the property is private but is in the class' scope so one can access 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.
What if there is a subclass that adds its own service methods?
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.
maybe we should use$this->container->get(__METHOD__); instead?
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.
But if$container is private, subclasses can't access it in their methods.
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 don't care, do we? each such class would just have to use the trait (provided we add the logic to call the method on the parent)
nicolas-grekasApr 27, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Then we should use
$this->service(__CLASS__, __FUNCTION__);, isn't it?
OR, we could make the trait work only with public and protected methods. Any preference? Does it make sense to use a private method for these anyway? (could be, you'll tell me what you think about this :) )
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.
Why can't we just use__FUNCTION__ as I had originally?
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.
using function works when used in public/protected methods, but might break when mixed with private methods: a private method with same name but different return type can be defined in a parent class. When the parent will call its private method, it will get the service that was injected for the child definition. And this can just break...
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.
Understood.
Using$this->service(__CLASS__, __FUNCTION__); would get my vote then.
nicolas-grekasApr 28, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Cool. I updated my proposal above, it should be ok so you can borrow it. For the changelog, this should be in the 4.2 section. With a few tests, I think we might be good :)
kbond commentedMay 2, 2018
I have pushed the changes@nicolas-grekas and I have discussed and updated the PR description. If this looks good, I'll add some tests. |
nicolas-grekas commentedMay 2, 2018
Looks good to me :) |
lyrixx commentedMay 2, 2018
Even if the implementation is good, I don't like this feature. And this is going to be accepted, it will needs some tests |
nicolas-grekas commentedMay 2, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@lyrixx I didn't like autowiring first. Now I like it. Might be the same for you here. About tests, we agree, see mentions of it above :) Another way of doing things? For sure, but there is a huge benefit for DX: enabling autocompletion (as described in linked issue#23898). And this doesn't leak to the outside world: it stays a pure implementation detail. Definitely worth it IMHO. |
| usePsr\Container\ContainerInterface; | ||
| /** | ||
| * @author Kevin Bond <kevinbond@gmail.com> |
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.
could be worth some notes in the docblock
nicolas-grekas left a comment
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.
getDeclaringClass should be used instead of getProrotype, my bad
| } | ||
| try { | ||
| $method =$method->getPrototype(); |
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.
the entire try/catch should be removed actually
| } | ||
| if (($returnType =$method->getReturnType()) && !$returnType->isBuiltin()) { | ||
| $services[$method->class.'::'.$method->name] =$returnType->getName(); |
nicolas-grekasMay 6, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
$services[$method->getDeclaringClass()->name.'::'.$method->name] = '?'.$returnType->getName();
kbond commentedMay 10, 2018
I added a happy path test. |
| continue; | ||
| } | ||
| if (($returnType =$method->getReturnType()) && !$returnType->isBuiltin()) { |
nicolas-grekasMay 10, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
I feel like we should be even more strict and not wire methods from parents, as they should be the one declaring their deps.
$method->getDeclaringClass() === self::class should be added first in the "if" (then we can use self::class also on the next line.)
| publicfunctionsetContainer(ContainerInterface$container):void | ||
| { | ||
| if (\is_callable(array('parent',__FUNCTION__))) { | ||
| parent::setContainer($container); |
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.
In order to be compatible with AbstractController, we should return the return value of the parent (thus set the property before this block and remove the void on the signature.)
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 there is no parent should I return something?
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.
I don't think so.
nicolas-grekas commentedMay 10, 2018
Would be nice to have a few more test cases covering:
Otherwise, it looks great to me, thank you! |
kbond commentedMay 10, 2018
I have added the requested tests. |
nicolas-grekas left a comment
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.
I like it :)
lyrixx commentedMay 28, 2018
Just one question, insteand of writing |
nicolas-grekas commentedMay 28, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
this is a runtime call, that'd be too slow
we discussed about it in#27077 (comment) actually, I think removing the "service" helper might be nice. @kbond ok for you? |
kbond commentedMay 30, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
My only hesitation is it makes creating composable traits (which I think/hope will not be uncommon) not as intuitive. I can see people putting That being said, if composable traits aren't common, it is easier to do Seems prudent to keep it simple now and add a |
kbond commentedMay 31, 2018
I removed the |
nicolas-grekas commentedJun 4, 2018
Thank you@kbond. |
This PR was squashed before being merged into the 4.2-dev branch (closes#27077).Discussion----------[DependencyInjection] add ServiceSubscriberTrait| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#23898| License | MIT| Doc PR |symfony/symfony-docs#9809This allows you to easily configure Service Subscribers with the following convention:```phpclass MyService implements ServiceSubscriberInterface{ use ServiceSubscriberTrait; public function doSomething() { // $this->router() ... } private function router(): RouterInterface { return $this->container->get(__METHOD__); }}```This also allows you to create helper traits like `RouterAware`, `LoggerAware` etc... and compose your services with them (*not* using `__METHOD__` in traits because it doesn't behave as expected.).```phptrait LoggerAware{ private function logger(): LoggerInterface { return $this->container->get(__CLASS__.'::'.__FUNCTION__); }}``````phptrait RouterAware{ private function router(): RouterInterface { return $this->container->get(__CLASS__.'::'.__FUNCTION__); }}``````phpclass MyService implements ServiceSubscriberInterface{ use ServiceSubscriberTrait, LoggerAware, RouterAware; public function doSomething() { // $this->router() ... // $this->logger() ... }}```Commits-------238e793 [DependencyInjection] add ServiceSubscriberTrait
…ond)This PR was merged into the master branch.Discussion----------[DependencyInjection] Document ServiceSubscriberTraitDocumentation forsymfony/symfony#27077Commits-------b0ac3a4 document ServiceSubscriberTrait
Uh oh!
There was an error while loading.Please reload this page.
This allows you to easily configure Service Subscribers with the following convention:
This also allows you to create helper traits like
RouterAware,LoggerAwareetc... and compose your services with them (not using__METHOD__in traits because it doesn't behave as expected.).