[DI] Improve psr4-based service discovery (alternative implementation) #23991
Conversation
|
We should throw an exception when namespace is used and resource isn't. |
|
nicolas-grekas
left a comment
nicolas-grekas
left a comment
There was a problem hiding this comment.
Definitely 👍 with this implem (vs #23986)
If you can rebase+retarget on 3.4, please do. We can do it while merging yes, but if there are merge conflicts, we'll have to ask you to rebase :)
|
Ok, rebased/retargeted to 3.4 and doc PR opened. |
| if ($throw = $this->isLoadingInstanceof) { | ||
| $keywords = self::$instanceofKeywords; | ||
| } elseif ($throw = isset($definition['resource'])) { | ||
| } elseif (isset($definition['resource']) || isset($definition['namespace'])) { |
There was a problem hiding this comment.
I wouldn't add the isset checl for the namespace here, it adds confusion to me (one would then wonder how a resource can be null while namespace isn't, which contradicts the above check
There was a problem hiding this comment.
Ok, then the check for namespace without resource would have to come before this conditional... Is that acceptable?
There was a problem hiding this comment.
OK, got it, the diff mislead me. Fine as is!
There was a problem hiding this comment.
but $throw still needs to be set
There was a problem hiding this comment.
Oops, yeah in master it isn't required anymore (#22750) but in 3.4 it is.
|
Thank you @kbond. |
… 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
…luz) This PR was merged into the 3.4 branch. Discussion ---------- [DI] add docs for new namespace option | Q | A | ------------- | --- | Doc fix? | no | New docs? | yes | Applies to? | 3.4 | Fixed tickets? | symfony/symfony#23991 Commits ------- f395dad Reworded and turned the sidebar into a section 16e0cc7 [DI] add docs for new namespace option
6 participants
This is an alternative to #23986. It is simpler and doesn't require a second glob in the service id. This adds a
namespaceoption. 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:
ping @stof, @nicolas-grekas