[DI] Improve PSR4-based service discovery #23986
Conversation
| * | ||
| * @link http://jarretbyrne.com/2015/06/197/ | ||
| */ | ||
| private function getClassFromFile($path) |
There was a problem hiding this comment.
I really think this is a bad idea and we should stick to what I described in #22397 (comment)
which doesn't require this magic logic.
There was a problem hiding this comment.
Yeah, I was kind of at a loss on how to implement this otherwise... Any suggestions?
There was a problem hiding this comment.
from this:
App\Domain\*\CommandHandler\:
resource: ../../src/Domain/*/CommandHandler
tags: [command_handler]we should parse App\Domain\*\CommandHandler\:
- extract the first non variable part
App\Domain\ - load the classes with the existing logic but only the ones matching
App\Domain\*\CommandHandler\(which would have been turned into a regexp before of course, see existing logic inAddAnnotatedClassesToCachePass::patternsToRegexps().)
There was a problem hiding this comment.
Thanks for the direction - I will switch my implementation.
| $parsingNamespace = true; | ||
| } | ||
|
|
||
| if (is_array($token) && $token[0] === T_CLASS) { |
There was a problem hiding this comment.
have you testing your code on a file containing anonymous classes ?
| $class .= $token[1]; | ||
| } | ||
|
|
||
| if ($token === ';') { |
There was a problem hiding this comment.
what about code using the bracketed notation for namespaces ?
| throw new InvalidArgumentException(sprintf('Expected to find class "%s" in file "%s" while importing services from resource "%s", but it was not found! Check the namespace prefix used with the resource.', $class, $path, $pattern)); | ||
| } | ||
|
|
||
| $r = $this->container->getReflectionClass($class); |
There was a problem hiding this comment.
I think this may still fail if the parent class is missing. Can you confirm @nicolas-grekas ?
There was a problem hiding this comment.
not anymore
but anyway, this implementation is going to change completly (see my other comments)
|
I am switching the implementation away from the token matching... That being said, I really like not requiring a namespace as the service id... Any thoughts on how we can remove the namespace requirement? |
I know, you're not the only one, but that's just magic in the bad sense. That's exactly like calling composer to drop psr-0&4 and say: do get the classes yourself. Which is FRAGILE and will break anytime soon. |
|
Yeah, fair enough - I knew it was too good to be true 😄 - I was pretty excited for 5 minutes! |
|
Closing in favour of #23991. |
… implementation) (kbond) This PR was merged into the 3.4 branch. Discussion ---------- [DI] Improve psr4-based service discovery (alternative implementation) | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #22397 | License | MIT | Doc PR | symfony/symfony-docs#8310 This is an alternative to #23986. It is simpler and doesn't require a second glob in the service id. This adds a `namespace` option. It is optional and if it isn't used, then it falls back to using the service id (current behaviour). As stof mentions in #22397 (comment), it is consistent with the xml loader. With this feature, you can define your services like this: ```yaml services: command_handlers: namespace: App\Domain\ resource: ../../src/Domain/*/CommandHandler tags: [command_handler] event_subscribers: namespace: App\Domain\ resource: ../../src/Domain/*/EventSubscriber tags: [event_subscriber] ``` ping @stof, @nicolas-grekas Commits ------- 00d7f6f [DI] improve psr4-based service discovery with namespace option
This is an attempt to add the feature described here: #22397 (comment). A side effect of my implementation is the namespace is no longer required as the service id - it can be anything now... I am unsure if this is desired or could cause a BC break but all the tests pass (except one testing for an invalid namespace).
With this feature, you can now define services like this:
Let me know if you have alternative suggestions for how to implement this.
(ping @nicolas-grekas)