Skip to content

[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

Merged

Conversation

mateuszsip
Copy link
Contributor

@mateuszsip mateuszsip commented Jan 7, 2018

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?

@mateuszsip
Copy link
Contributor Author

labels are wrong, my fault.

Copy link
Member

@nicolas-grekas nicolas-grekas left a 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 :)

@nicolas-grekas nicolas-grekas changed the title ServiceProviderInterface, implementation for ServiceLocator [DI] ServiceProviderInterface, implementation for ServiceLocator Jan 7, 2018
@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Jan 7, 2018
@unkind
Copy link
Contributor

unkind commented Jan 7, 2018

What's the point of the interface?

@mateuszsip
Copy link
Contributor Author

@unkind let you verify if ServiceLocator can provide services for class implementing ServiceSubscriberInterface.

And can be used for debugging.

probably @nicolas-grekas will bring more specific examples.

Thank for review, missed that it's supplementary for PsrContainerInterface.

@unkind
Copy link
Contributor

unkind commented Jan 7, 2018

let you verify if ServiceLocator can provide services for class implementing ServiceSubscriberInterface.

I mean not method by itself, but separated interface. I see the only implementation — ServiceLocator. Do you mind to provide example when type hinting against ServiceLocator is not possible/enough if you want to execute getProvidedServices()? Any other implementations?

Copy link
Member

@nicolas-grekas nicolas-grekas left a 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!

@nicolas-grekas
Copy link
Member

@unkind what I described in th linked PR: to provide reflection on the inner of the locator.
This concern is legitimate for any PSR-11 container - ServiceLocator, but could also be implemented on our Container, etc.
It could help e.g generating useful message, allow iterating over services, etc. (any use case that reflection provides really.)

@nicolas-grekas
Copy link
Member

@mnapoli @moufmouf @Crell FYI here is an implementation of what we talked about recently in Paris :)

@moufmouf
Copy link
Contributor

moufmouf commented Jan 9, 2018

@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 getProvidedServices() return types?

For instance: "string" maps to a string, "LoggerInterface[]" maps to an array of LoggerInterface...

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jan 9, 2018

"string" maps to a string

that's already the case here

"LoggerInterface[]" maps to an array of LoggerInterface

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

stof
stof previously requested changes Jan 9, 2018
*
* * 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
Copy link
Member

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 ?

Copy link
Member

@nicolas-grekas nicolas-grekas Jan 9, 2018

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.

@nicolas-grekas nicolas-grekas added this to the next milestone Apr 20, 2018
@nicolas-grekas nicolas-grekas removed the RFC RFC = Request For Comments (proposals about features that you want to be discussed) label Aug 16, 2018
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Sep 5, 2018

@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 Symfony\Contracts\Service namespace. WDYT?

@stof
Copy link
Member

stof commented Sep 5, 2018

@nicolas-grekas https://github.com/symfony/symfony/tree/master/src/Symfony/Contracts#design-principles is saying:

all contracts must have a proven implementation to enter this repository;

Doesn't this means that this cannot go directly in contracts ?

@nicolas-grekas
Copy link
Member

@stof correct, but we can still get closer to the point where this might be merged :)

@mateuszsip
Copy link
Contributor Author

@nicolas-grekas sure :)

@mateuszsip mateuszsip force-pushed the feature/service-provider-interface branch from 92061e8 to 35362af Compare September 7, 2018 20:49
@mateuszsip
Copy link
Contributor Author

@nicolas-grekas done.
What should I do about this failing test? Should I submit a separate PR with contracts or can we just ignore it?

@nicolas-grekas nicolas-grekas force-pushed the feature/service-provider-interface branch 2 times, most recently from 77e29d6 to 6ff44c4 Compare March 9, 2019 23:14
Copy link
Member

@nicolas-grekas nicolas-grekas left a 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.

@nicolas-grekas nicolas-grekas force-pushed the feature/service-provider-interface branch 2 times, most recently from a97202e to 4d8bb18 Compare March 10, 2019 00:04
@nicolas-grekas nicolas-grekas force-pushed the feature/service-provider-interface branch from 4d8bb18 to a91a570 Compare March 10, 2019 00:05
@nicolas-grekas nicolas-grekas force-pushed the feature/service-provider-interface branch from a91a570 to ba1260f Compare March 20, 2019 15:10
@nicolas-grekas
Copy link
Member

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

@nicolas-grekas nicolas-grekas force-pushed the feature/service-provider-interface branch from ba1260f to aaf5422 Compare March 28, 2019 11:22
@nicolas-grekas
Copy link
Member

Thank you @kejwmen.

@nicolas-grekas nicolas-grekas merged commit aaf5422 into symfony:master Mar 28, 2019
nicolas-grekas added a commit that referenced this pull request Mar 28, 2019
…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
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019
@fabpot fabpot mentioned this pull request May 9, 2019
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.