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

Open
wants to merge 11 commits into
base: master
from

Conversation

@kejwmen
Contributor

kejwmen 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?

@kejwmen

This comment has been minimized.

Contributor

kejwmen commented Jan 7, 2018

labels are wrong, my fault.

@nicolas-grekas

Thanks for working on this, looks great :)

public function getProvidedServices(): array
{
if (empty($this->providedTypes)) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 7, 2018

Member

The list of factories can be empty. Let's do null !== here.

if (empty($this->providedTypes)) {
$this->providedTypes = $this->factories;
array_walk($this->providedTypes, function (callable &$factory) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 7, 2018

Member

A foreach would be better here.

array_walk($this->providedTypes, function (callable &$factory) {
$type = (new \ReflectionFunction($factory))->getReturnType();
$factory = $type ? $type->getName() : '?';

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 7, 2018

Member

I didn't specify, but I think we should prefix by "?" if allowsNull is true.

@@ -19,12 +19,13 @@
* @author Robin Chalas <robin.chalas@gmail.com>
* @author Nicolas Grekas <p@tchwork.com>
*/
class ServiceLocator implements PsrContainerInterface
class ServiceLocator implements PsrContainerInterface, ServiceProviderInterface

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 7, 2018

Member

ServiceProviderInterface should extend PsrContainerInterface

@nicolas-grekas nicolas-grekas changed the title from ServiceProviderInterface, implementation for ServiceLocator to [DI] ServiceProviderInterface, implementation for ServiceLocator Jan 7, 2018

@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Jan 7, 2018

@unkind

This comment has been minimized.

Contributor

unkind commented Jan 7, 2018

What's the point of the interface?

* under "logger" name
* * array('foo' => '?') means that object provides service of unknown type under 'foo' name
*
* @return array The provided service types, keyed by service names

This comment has been minimized.

@ostrolucky

ostrolucky Jan 7, 2018

Contributor

string[]

@kejwmen

This comment has been minimized.

Contributor

kejwmen commented Jan 7, 2018

@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

This comment has been minimized.

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?

@nicolas-grekas

LGTM with minor comments, thanks!

@@ -11,20 +11,21 @@
namespace Symfony\Component\DependencyInjection;
use Psr\Container\ContainerInterface as PsrContainerInterface;
use ReflectionType;

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 7, 2018

Member

should be removed (we don't put root namespaces in "use")

foreach ($this->factories as $name => $factory) {
$type = (new \ReflectionFunction($factory))->getReturnType();
$this->providedTypes[$name] = $type ? $this->determineTypeString($type) : '?';

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 7, 2018

Member

let's inline the method, this is fine:
$this->providedTypes[$name] = $type ? ($type->allowsNull() ? '?' : '').$type->getName() : '?';

namespace Symfony\Component\DependencyInjection;
use Psr\Container\ContainerInterface as PsrContainerInterface;

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 7, 2018

Member

no need for "as"

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jan 7, 2018

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

/**
* A ServiceProviderInterface exposes provided service types via the {@link getProvidedServices} method.
*/

This comment has been minimized.

@chalasr

chalasr Jan 7, 2018

Member

Not only types but identifiers, I would change it to something like Exposes the services provided by a container. to be consistent with PSR-11. @author annotations are missing also

This comment has been minimized.

@kejwmen

kejwmen Jan 7, 2018

Contributor

does it sound better now?

This comment has been minimized.

@chalasr

chalasr Jan 7, 2018

Member

I would drop the via the {@link getProvidedServices} method. part (no need for describing the api, the code speaks for itself), but good enough :)

* A ServiceProviderInterface exposes identifiers and types of services provided by a container
* via the {@link getProvidedServices} method.
*
* @author Mateusz Sip <mateusz.sip@gmail.com>

This comment has been minimized.

@chalasr

chalasr Jan 7, 2018

Member

I would put @nicolas-grekas as co-author here (no obligation of course)

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jan 8, 2018

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

@moufmouf

This comment has been minimized.

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

This comment has been minimized.

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

* * array('foo' => '?') means that object provides service of unknown type under 'foo' name
* * array('bar' => '?Bar\Baz') means that object provides service implementing Bar\Baz or null
*
* @return array|string[] The provided service types, keyed by service names

This comment has been minimized.

@stof

stof Jan 9, 2018

Member

string[] is enough

This comment has been minimized.

@kejwmen

kejwmen Jan 9, 2018

Contributor

ok, changed

* * 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
* * array('bar' => '?Bar\Baz') means that object provides service implementing Bar\Baz or null

This comment has been minimized.

@stof

stof Jan 9, 2018

Member

missing under "bar" name for consistency

This comment has been minimized.

@kejwmen

kejwmen Jan 9, 2018

Contributor

thanks, fixed.

*
* * 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

This comment has been minimized.

@stof

stof Jan 9, 2018

Member

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 ?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 9, 2018

Member

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 removed the Ready label Jan 23, 2018

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jan 23, 2018

Looks good to me, let's put this on hold, waiting for the FIG.
@moufmouf @mnapoli @Crell needing you at this stage :)

@moufmouf

This comment has been minimized.

moufmouf commented Mar 1, 2018

Hey!

As promised, I just wrote a lengthy article presenting your solution and comparing it to container-interop/service-provider.

I'm trying to highlight the strength of both solutions. I have a feeling there must be a way to gather the best of both worlds.

I opened the discussion on the PHP-FIG mailing list here: https://groups.google.com/forum/#!topic/php-fig/Up0JATOb0-w

Feel free to join the discussion!

@mnapoli

This comment has been minimized.

Contributor

mnapoli commented Mar 4, 2018

Hey! Nice summary @moufmouf! Lately I've been completely swamped and I'm afraid I cannot do much to help except sending +1's on trying out that idea :/ It does sound good, especially if it fits Symfony's need (we were originally worried about performances and compilation).

@nicolas-grekas

This comment has been minimized.

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?

@nicolas-grekas nicolas-grekas moved this from TODO to WIP in Contracts Sep 5, 2018

@stof

This comment has been minimized.

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

This comment has been minimized.

Member

nicolas-grekas commented Sep 5, 2018

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

@kejwmen

This comment has been minimized.

Contributor

kejwmen commented Sep 6, 2018

@nicolas-grekas sure :)

@kejwmen kejwmen force-pushed the kejwmen:feature/service-provider-interface branch from 92061e8 to 35362af Sep 7, 2018

@kejwmen

This comment has been minimized.

Contributor

kejwmen commented Sep 7, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment