-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[DI] ServiceProviderInterface, implementation for ServiceLocator #25707
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
[DI] ServiceProviderInterface, implementation for ServiceLocator #25707
Conversation
labels are wrong, my fault. |
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.
Thanks for working on this, looks great :)
What's the point of the interface? |
src/Symfony/Component/DependencyInjection/ServiceProviderInterface.php
Outdated
Show resolved
Hide resolved
@unkind let you verify if And can be used for debugging. probably @nicolas-grekas will bring more specific examples. Thank for review, missed that it's supplementary for |
I mean not method by itself, but separated interface. I see the only implementation — |
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.
LGTM with minor comments, thanks!
src/Symfony/Component/DependencyInjection/ServiceProviderInterface.php
Outdated
Show resolved
Hide resolved
@unkind what I described in th linked PR: to provide reflection on the inner of the locator. |
src/Symfony/Component/DependencyInjection/ServiceProviderInterface.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/DependencyInjection/ServiceProviderInterface.php
Outdated
Show resolved
Hide resolved
@nicolas-grekas Cool! I know I promised to write an article on this idea and I haven't had time to do it yet... this is still on my TODO list though! I know the Symfony container is mostly used to store objects (which is not the case of all PSR-11 containers). Yet, some factories might return arrays of objects, or scalars. Do you think it might be worthwhile to add those cases to For instance: "string" maps to a string, "LoggerInterface[]" maps to an array of LoggerInterface... |
that's already the case here
that'd need more information than we rely on in this implementation, but otherwise, yes (the current implementation is already compatible with it, it just won't ever return that :).) |
src/Symfony/Component/DependencyInjection/ServiceProviderInterface.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/DependencyInjection/ServiceProviderInterface.php
Outdated
Show resolved
Hide resolved
* | ||
* * array('logger' => 'Psr\Log\LoggerInterface') means the object provides service implementing Psr\Log\LoggerInterface | ||
* under "logger" name | ||
* * array('foo' => '?') means that object provides service of unknown type under 'foo' name |
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'm not sure we should use a single ?
as it can be confusing with the notation of nullable types. Is there any other proposal for that ?
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 benefit of using ?
for unknown types is that if one want to know if such a type can be null, then the logic is invariably '?' === $type[0]
. Any other convention would require more code.
@kejwmen would you be up to resume this PR? I'd suggest to rebase on top of master, and move the new interface in the |
@nicolas-grekas https://github.com/symfony/symfony/tree/master/src/Symfony/Contracts#design-principles is saying:
Doesn't this means that this cannot go directly in contracts ? |
@stof correct, but we can still get closer to the point where this might be merged :) |
@nicolas-grekas sure :) |
92061e8
to
35362af
Compare
@nicolas-grekas done. |
77e29d6
to
6ff44c4
Compare
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 just rebased this PR and implemented some missing bits in ResolveServiceSubscribersPass
.
I think it would be a nice addition.
The next step could be to provide a pass that uses ServiceProviderInterface
to wire an external locator into the main one. This could require a new tag.
a97202e
to
4d8bb18
Compare
4d8bb18
to
a91a570
Compare
a91a570
to
ba1260f
Compare
@symfony/deciders I'm going to merge this PR soon: the feature it provides (reflecting what a container provides) is going to play nice with #30348. |
ba1260f
to
aaf5422
Compare
Thank you @kejwmen. |
…iceLocator (kejwmen) This PR was merged into the 4.3-dev branch. Discussion ---------- [DI] ServiceProviderInterface, implementation for ServiceLocator | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #25686 | License | MIT | Doc PR | applicable here? Implements #25686 Not sure if it needs any additional documentation, @nicolas-grekas? Commits ------- aaf5422 [DI][Contracts] add and implement ServiceProviderInterface
Implements #25686
Not sure if it needs any additional documentation, @nicolas-grekas?