container-interop / service-provider

[EXPERIMENTAL] Promoting container/framework interoperability through standard service providers
72 stars 10 forks source link

A method to enforce factory methods signature? #18

Closed gmazzap closed 11 months ago

gmazzap commented 8 years ago

I already exposed in #16 my opinion about the fragility I see in current implementation, because the getServices() method is not able to enforce... anything.

It means that the definition of the standard will entirely be in the method doc block, but the getServices() method itself, that is everything the interface contains, is nothing more than a wrapper around an array.

I think that if we are expecting people to write factory methods in the service provider, we should enforce in the interface the factory methods signature.

This is more or less what I have in mind:

interface ServiceProvider
{
   /**
    * Return an associative array of provided services where:
    * each item *key* is the service name (can be used with container `get` / `has`)
    * each item *value* is the the service "type" (class, interface or PHP scalar type).
    * An invalid item value (not string or not referring any valid type) COULD throw an Exception,
    * use `null` as value to safely don't enforce any specific type
    *
    * @return string[]
    */
   public function servicesMap();

  /**
   * Given a service name create a service instance.
   * Provided container can be used to resolve dependencies.
   *
   * @param string $service Any of the service names, provided as array keys by servicesMap()
   * @param ContainerInterface $c
   * @param callable $previous If provided, can be called to obtain previous service value
   * @return mixed
   */
   public function createService($service, ContainerInterface $c, callable $previous = null);
}

Please note how I not used static methods. Refers to #5 for more info on the reason why.

I think that, even if not ideal, this is a probably acceptable trade off and reduce the "fragility" of current implementation.

Another variant could be that servicesMap, could just return a list of services names, (no information on service types). This would simplify the interface. In that case servicesMap should be renamed.

Implementation example:

namespace MyLib;

use Interop\Container\ContainerInterface;
use Interop\Container\ServiceProvider;
use Psr\Log\LoggerInterface;
use Monolog\Logger;

class LoggerServiceProvider implements ServiceProvider
{

     const MAP = ['logger' => LoggerInterface::class];

     public function servicesMap()
     {
         return static::MAP;
     }

     public function createService($service, ContainerInterface $c, callable $previous = null)
     {
         if (! array_key_exists($service, static::MAP) ) {
             throw new \Exception(sprintf("Service %s not provided by %s.", $service, __CLASS__));
         }
         $cb = [$this, $service];
         $instance = $cb($c); 
         $this->checkService($instance);

         return $instance;
      }

      private function logger(ContainerInterface $c)
      {
          return new Logger('name');
      }

      private function checkService($service, $instance)
      {
         $scalars = ['bool', 'int', 'float', 'string', 'array', 'resource'];
         $checker = in_array(static::MAP[$service], $scalars, true)
             ? 'is_'.static::MAP[$service]
             : function($instance) use($service) { return is_a($instance, static::MAP[$service]); }

         if (!is_null(static::MAP[$service) && !$checker($instance)) {
             throw new \Exception(
                sprintf(
                    "Service %s expected to be of type %s, but is of type %s instead.",
                    $service,
                    static::MAP[$service],
                    is_object($instance) ? get_class($instance) : gettype($instance)
                )
             );
         }
     }

}

Note how in a real world library containing more service providers, servicesMap,createService and checkService could be implemented in a trait or in an abstract superclass, so that each provider class could just define own MAP constant and the needed private factory methods.

It means the code necessary would be very similar on current implementation (RAD not affected) but this implementation is capable to enforce factory method signature and returned type.

Among other things, this has the huge benefit that $container->get('service') is enforced to return a predictable type (and an IDE supporting the standard could take advantage of it).

Finally, if this implementation will be take into consideration, we should probably also take into consideration to provide two exception classes, one to be thrown when createService receives a non provided service, and one to be thrown when the service type does not match the expected type. As alternative, one exception class with 2 different status codes (provided as class constants) should do.

moufmouf commented 8 years ago

Hey Giuseppe!

Thanks a lot for taking the time to put up this proposition.

Here, I'm not going to address the "static" vs "non-static" part (since it is already addressed in #5.

If I understand correctly, you are trying to add "type safety" to the service provider by making the servicesMap method return a map of "entry name" => "class name".

Then, the createService method performs 2 things:

The nice thing about your proposal is that it feels a bit less hackish than returning a string of public static method names.

However, it is also slightly slower (in the case of a compiled container, the container will need to call the createService method that will itself call the factory method instead of calling the factory method directly). @mnapoli any thoughts about this createService method? Basically, it does make it explicit that the interface has 2 sides: one about declaring the list of services available, and one about creating the services.

Regarding your argument about the fragility of the current proposal, I must say I'm not convinced at all.

First, you are adding additional code to perform type checking which will slow down the container (a lot of people are very picky when it comes to container performance). Then, this is something that should be really dealt by PHP directly.

How does it compare to our current proposal in PHP 7?

class MyServiceProvider implements ServiceProvider
{
    public static function getServices()
    {
        return [
            'my_service' => 'getMyService',
        ];
    }

    public static function getMyService() : MyService
    {
        return new MyService();
    }
}

Using this implementation:

gmazzap commented 8 years ago

My implementation was just an example.

See this one:

class MyServiceProvider implements ServiceProvider
{
    public function servicesMap()
    {
      return ['logger' => LoggerInterface::class];
    }

  public function createService($service, ContainerInterface $c, callable $previous = null)
  {
    if ($service === 'logger')
      return new Logger('name');
  }
}

I think that there's no performance concern here.

Finally, in my experience, talking about performance without any real world profiling makes no sense: excluding "stupid" code, most of the times performance bottlenecks are where you don't expect them and assumption are not something that worth talking about.

And, surely, if we had only PHP 7+ as target, my serviceMap could just be something like serviceNames() and returning just services names, instead of a map of names to type. A solution that could be considered in any case, as mentioned in OP.

My point is not only type sanity, but the fact that current implementation is just a "fake" interface.

A "real" interface is a behavioural contract that object instances have to respect. The current implementation (besides not considering any object instance because of statics) does not contain any "contract" in code: getServices can return literally anything and the implementation code that will call it has no way to be sure what it will return. In my approach, the code that will call servicesMap, at least, knows that the method createService is there, and accept a specific signature.

Still it isn't the perfect design, but at least I find it less hackish and more intention revealing.

Anyone looking your example immediately undersand what that service provider does... but that is an implementation: if you have to look at implementation to understand what an object does, than the abstraction is wrong, because the most important thing that abstraction should do is to make clear what implementations are expected to do.

Please imagine a developer (even experienced) who never heard about container interop and see this code. They could use all the experience and all the imagination they has, but they will never ever could be capable to guess what you expect getServices does.

And even the naming does not help... getServices makes one guess it returns.... "sevices", instead it returns "an array of static method names that once called on the class who contain the method return a service". If this is not a fragile abstraction, I don't imagine what a fragile abstraction could be.

mnapoli commented 8 years ago

I also hope that "people [that] are very picky when it comes to container performance" are not the same who use "autowiring" via reflections. ;)

Could you be more constructive please? This is just wrong information, and derailing the topic.

moufmouf commented 8 years ago

I also hope that "people [that] are very picky when it comes to container performance" are not the same who use "autowiring" via reflections. ;)

Actually, I was referring to Symfony. They are very much counting the number of function calls performed when it comes to compiling their container and trying to minimize the impact of the container on global performance as much as possible.

gmazzap commented 8 years ago

@mnapoli removed that sentence.

And I don't think performance is the main and not even a minor topic here. Performance will entirely depends on implementations, and we are talking about interfaces.

moufmouf commented 8 years ago

Hey,

Giuseppe, if you look at your proposed implementation, it is actually not that far from an "extended" ContainerInterface:

// The interface extends ContainerInterface (a service provider can be called with get/has)
// The interface extends Traversable: when traversed, the values yielded are the name of the services
interface ServiceProviderInterface implements ContainerInterface, Traversable
{
    // The get method takes an additional parameter: the previous declaration of a service.
    public function get($identifier, $previous = null);
}

This is not really a "service provider" anymore, but it fills the same purpose.

What I would however is maybe to put a pause on this discussion for now.

There are a lot of things to be discussed regarding the final look of the ServiceProviderInterface, but it is really useless to discuss those if the majority of the PHP-FIG members think that we should focus on something else than service providers (see https://groups.google.com/forum/#!topic/php-fig/xsY8bRG5K0M)

So could we postpone any discussion regarding the ServiceProviderInterface design from one week or two, just the time to see if most PHP-FIG members think we are headed the right direction?

gmazzap commented 8 years ago

David,

please, don't look at implementation.

Current interface is:

interface ServiceProvider {
    public static function getServices();
}

My proposal is:

interface ServiceProvider {
  public static function servicesMap();

  public static function createService($service, ContainerInterface $c, callable $previous = null);
}

I used statics methods in my proposal here as well for a more direct comparison.

Note how the current implementation already expects that service provider classes contain factory methods that have the exact same signature of createService I proposed.

It means that the class design (in term of class responsibility) is identical.

The difference is that my implementation makes things explicit, while current implementation does not.

So people looking at the current interface will need to read documentation or some meta document on the web to understand how to implement factory methods, looking at my interface they will just need to look at code.

This "explicitness" I think is the main benefit of my proposal and capability to enforce the signature of factory methods is a, non trivial, additional benefit.

In summary, with the interface I propose you loose nothing and you just gain something.

mnapoli commented 8 years ago

What you loose is that all the factory code is crammed into one method though (if you want to avoid sub-method calls).

gmazzap commented 8 years ago

Well, assuming a service provider would provide just sever providers, the code could be:

public static function createService($service, ContainerInterface $c, callable $previous) {
    switch($service) {
          case 'a':
              return new A();
          case 'b':
              return new B();
          case 'c':
              return new C();
    }
}

or maybe:

public static function createService($service, ContainerInterface $c, callable $previous) {
    if (method_exists(__CLASS__, $service)) {
       return call_user_func([__CLASS__, $service], $c, $previous);
    }
}

Implementation is left to implementers.

Just to say some numbers: in current implementation the number of factory methods is always, at least n+1 where n is the number of services the provider provides.

Using the implementation I proposed, number of methods could be minimum 2 or maximum n+2, so comparing the best scenario of current implementation and the worst scenario for solution I proposed, the latter has just 1 method more than current implementation. However, in its best scenario, solution I proposed could have several methods less than current implementation.

My point is:

mnapoli commented 8 years ago

I don't understand your numbers.

Current solution Your 1st suggestion Your 2nd suggestion
Number of calls to get 1 service 1 1 2
Code creating the service is in its own method yes no yes

Yes your suggestion has the benefit of being able to enforce the signature of the factory method but having to put all the service factories inside one method is a big flaw IMO.

Not being able to enforce the signature of the factory isn't such a big deal, that's how it works today with Pimple (Silex, Slim…), Laravel, PHP-DI, etc. Zend ServiceManager also allows PHP callbacks (so no interface in that case). It doesn't stop those projects from working.

gmazzap commented 8 years ago

Just to explain the numbers.

First of all there's no "my 1st solution" and "my 2nd" solution: those "1st" and "2nd" are implementation examples.

You can't stop people on do specific things (or crazy things) on implementantion, so you should not care about implementation at all.

There's only one proposal of mine, and it is the interface.

Using my proposal, that regards the interface, people are free to have all factory methods inside the createSevice or they can use createService as a gateway (a facade, if you prefer) and then actual factory methods can be each on their own method.

Let's admit that put all the service factories inside one method is a big flaw: this is a implementation detail, not something that is enforced by the interface, you can't blame the interface for an implemementation choice on which the interface has no power.

Are you really convinced that current interface is good enough? Fine.

I'm not. Really not. I tried to convince you, without success.

I think that until me, you and @moufmouf are the only three people involved in the discussion, there's no much more to say.

The point is that the real number of methods depends on implementation. And since you can't know how people will implement the interface, makes absolutely no sense to talk about the implementation details such as the number of methods.

Considering that the final target of the standard of is an interface, we should only talk about which is the best interface not the best implementation.


(Update 12/04/2016 everything below here is something stupid I wrote without thinking too much)


Hoping that my point is clear, let's go back to numbers.

Assuming that a service provider provides 3 services.

Current implementation needs:

In total: 4 methods call. That is n+1 where n is the number of services. So, to "resolve" a service provider current implementation always needs, at least, n+1 methods call. I said "at least" because you can never know what people do on implementation and what happen inside factory methods. It means that n+1 is the best case scenario for current implementation.

A service provider that implements the interface I proposed, to be fully resolved, may require:

So in best scenario the interface I proposed uses just 2 method calls vs 4 of the current implementation. Which may became 2 VS 11 for a service provider that provides 10 services.

In worst scenario, my implementation requires 5 methods (for 3 services) VS 4 of current implementation (assuming best scenario for current implementation). Which became 12 VS 11 for a service provider that provides 10 services.

In short, in worst scenario requires just a method more, but in best scenario can save much more method calls.

mnapoli commented 8 years ago

First of all there's no "my 1st solution" and "my 2nd" solution: those "1st" and "2nd" are implementation examples.

We need to consider how interfaces will be implemented. It doesn't make sense to consider the interface alone ore we'll only be talking about smoke. If it's not possible to write an implementation that is clear and efficient then the interface needs to be improved.

Let's admit that put all the service factories inside one method is a big flaw: this is a implementation detail, not something that is enforced by the interface

Exactly:

On the other hand the current solution doesn't have this problem because it can enforce it.

1 call to getServices() which returns 3 methods names

We don't need to consider this call because it will most probably not be done at runtime… (for obvious reasons)

Which may became 2 VS 11 for a service provider that provides 10 services.

I don't understand. createService() needs to be called 10 times, because one call to this method will only return 1 service, not 10. So it's the same amount of calls at minimum.

The point is that the real number of methods depends on implementation. And since you can't know how people will implement the interface

Maybe with your solution we can't know but with the current one we know.

Considering that the final target of the standard of is an interface, we should only talk about which is the best interface not the best implementation.

This is wrong on many levels. Caring about a standards but not about implementations is the best way to fail. We have to take real implementation constraints into account.

Have you tried implementing your suggestion into your DI container or your framework (or any existing container or framework)? The PHP-FIG members don't want to waste time on theoretical ideas, that's why we spend time actually implementing and testing what we design. We spend considerable amount of time writing adapters and implementations to have real experience on what we suggest. Drafting something on a paper on a GitHub issue is 5% of the job. That's why we are discussing details.

moufmouf commented 8 years ago

Hey guys,

I understand very well both points of view.

On @Giuseppe-Mazzapica 's side, it's very clear this is all about having an interface that is "readable" and "explicit". Looking at the method signature of the interface proposed by Giuseppe, it is immediately obvious what the interface does. Looking at the current proposed interface, the method signature alone is not enough. You have to read the PHP-Doc to understand how it works. So regarding expressiveness of the interface, it is very clear that @Giuseppe-Mazzapica 's proposal is best.

Now, I've worked a lot on different implementations (both with runtime and with compiled containers). I also know how some folks are completely adamant about the performance of the container.

If you "compile" the container, it means that you want your container to have literally 0 impact on performance.

Using Giuseppe's proposal, we would certainly have something like this most of the time:

public static function createService($service, ContainerInterface $c, callable $previous) {
    return call_user_func([__CLASS__, $service], $c, $previous);
}

Now, for each service, we get:

  1. a call to createService
  2. a call to call_user_func. call_user_func is known to be very slow

Let's compare this to the current interface proposal.

For runtime containers, the call to call_user_func needs to be done anyway, so we only save one function call to createService.

For compiled containers however, the call to call_user_func is replaced by a much more efficient MyClass::myMethod() call. So we do save some performance.

Always regarding performance, with the current interface proposal, we can add some performance improvements for compiled container. I've been playing lately with a service provider to definition-interop container. This is completely alpha and a bit crazy, but it works.

Basically, I'm reading the service provider and trying to understand the "meaning" of the code. If I find some code like this:

function bar(Container $container) {
    return $container->get('foo');
}

then I can tell that the 'bar' factory is an alias to 'foo'. This is interesting because compiled container can resolve aliases at compile time rather than at runtime, hence saving some more function calls.

You get the idea. The current interface offers a great deal of optimization strategies. If we add a createService method in the middle, we get some flexibility, but we loose in performance.

So that's it basically. We are facing a dilemma. We must make trade-off between expressiveness and performance. There is no "right" choice, just a trade-off.

Very honestly, I like both solutions, and if any one of those solutions makes it as a PSR, I'll be happy. As the project matures and more people get interested we will probably get more feedback regarding this issue. In the meantime, shall we keep this issue open? Maybe we can open a dedicated thread on the PHP-FIG mailing list in a few weeks (when it is clear that we follow the service-provider route?)

moufmouf commented 8 years ago

Hi guys,

I was at Symfony Live in Paris last Friday, and I had a chance to speak with @stof about service-providers vs definition-interop. I was explaining what I'm trying to do right now - static analysis of PHP code to "guess" the definitions from the factory code (which is a bit hackish), when he came up with a great idea (IMHO) -.

Here goes his idea:

Rather than having a getServices() method that returns a mapping between service names and public static method names, we should have a mapping between service names and callables.

class MyServiceProvider implements ServiceProvider
{
    public (maybe static) function getServices() {
        return [
            "my_alias" => function(ContainerInterface $c, callable $previous = null) {
                return $c->get('foo');
            }
        ]
    }
}

or

class MyServiceProvider implements ServiceProvider
    public (maybe static) function getServices() {
        return [
            "my_alias" => [self::class, 'alias']
        ]
    }

    public static function alias(ContainerInterface $c, callable $previous = null) {
        return $c->get('foo');
    }

}

Why is it a good idea? Because it allows the creation of invokable classes. Like this:

interface ServiceProviderFactory {
    public function __invoke(ContainerInterface $container, callable $previous = null);
}

class Alias implements ServiceProviderFactory, ReferenceDefinitionInterface /* from definition-interop! */ {
    private $target;

    public function __construct($target) {
        $this->target = $target;
    }

    public function getTarget() {
        return $this->target;
    }

    public function __invoke(ContainerInterface $container, callable $previous = null) {
        return $container->get($this->target);
    }
}

Now, the service provider can look like this:

class MyServiceProvider implements ServiceProvider
    public (maybe static) function getServices() {
        return [
            "my_alias" => new Alias('alias');
        ]
    }
}

This is very interesting because this enables us to build a "bridge" between service providers and definition-interop. Objects can implement both the __invoke method and (optionnally) some interface from definition-interop. Runtime containers will simply ignore the definition-interop part. Compiled container can tap into it to make additional optimizations if needed (resolving aliases or parameters, maybe doing some inlining, etc...)

So on the positive side, we have:

On the downside:

So that's it. I'm pretty fond of this @stof idea. I think it is way cleaner than what I'm doing with static code analysis (and way more robust!) I'd definitely like to give it a go. What do you think?

mnapoli commented 8 years ago

Hi @moufmouf maybe we should start another thread about your suggestion? Anyway this was a very interesting read and very interesting idea with a lot of potential.

However I'm a bit cautious because in order for your solution to work everyone (i.e. every module) needs to be using the objects instead of the callables. So writing modules has gone back to being complicated like in definition-interop, we loose a lot of the simplicity of service-provider.

Yes modules are not forced to use objects, because we only require them to be callables, however if some containers require definitions to be objects then we have 2 worlds and the standard is not a standard anymore.

And while your example works, it's simple. Aliasing an entry is quite rare so not really a performance issue. I believe the biggest performance issue might be with decorating an object repeatedly (e.g. event dispatcher to which each module registers listeners) or decorating an array repeatedly (e.g. "twig.extension" array to which modules add items). Both of those are not as simple to represent using object definitions.

A solution I was thinking about but I'm not sure if it could ever be accepted could be:

But obviously annotations… Anyway the idea behind that would be to have 2 standards: one basic that works everywhere, and another one that comes on top of the first that adds more specific behaviors.

I don't think we should make too many sacrifices because of compiled containers (mainly Symfony after all). I'm fine with finding a middle ground, but as @mindplay-dk said in https://github.com/container-interop/definition-interop/issues/26 we shouldn't penalize everyone because of one container.

ircmaxell summarizes that very well in http://blog.ircmaxell.com/2014/10/an-open-letter-to-php-fig.html (which was partly about how PSR-6 could have been much simpler yet still powerful).

moufmouf commented 8 years ago

Hey Matthieu,

I completely agree that we should split this into 2 standards. And actually, right now, we really need to focus on only one standard: service-providers.

Now, I've been playing with the return type of "getServices" this afternoon. I've been switching the return type to an array of callables. After spending some time tinkering with it with both compiled and runtime container, I can say this:

We get some direct benefits from this:

One of this freedom is about having the possibility to use objects implementing the __invoke method.

You are talking about decorating an array repeatedly (twig.extensions). Today, code would look like this:

public static function getServices() {
     return [
        'twig.extensions' => 'addExtension',
        'myExtension' => 'myExtension',
    ];
}

public static function addExtension() {
    return new MyExtension();
}

public static function addExtension(Container $c, $previous = null) {
    if ($previous = null) {
        $extensions = [];
    } else {
        $extensions = $previous();
    }
    $extensions[] = $c->get('myExtension');
}

Tomorrow, it could look like this:

public static function getServices() {
     return [
        'twig.extensions' => new AddServiceToArray('myExtension'),
        'myExtension' => function() {
            return new MyExtension();
        }
    ];
}

And maybe AddServiceToArray does not implement any special interface and is not even readable by a compiled container. The second piece of code is way more readable than the first (in my opinion).

So even take the definition-interop argument out of the equation, I still think that returning callables instead of function names is a better idea.

So my proposal would be to switch to "array of callables" instead of "array of method names" for the return type of "getServices" and do not touch anything in the PSR. Especially, I'm ok to not add any reference to definition-interop and keep it dead simple. Would you agree with that?

gmazzap commented 8 years ago

Replying to @mnapoli

I don't understand. createService() needs to be called 10 times, because one call to this method will only return 1 service, not 10. So it's the same amount of calls at minimum.

You are perfectly right. I even updated my original comment to say that part is completely BS.


We don't need to consider this call because it will most probably not be done at runtime… (for obvious reasons)

(talking about getServices())

Can you say which these "obvious reasons" are? I, honestly, don't get it.

Me and a few of other millions of people use Pimple as container. If I have to think at a service provider that implemements this standard, I can only think at something as similar as possible to Pimple service provider... which would call getServices on runtime.

In your "simplex", getServices is called on runtime for each service provider.

Or I am missing something?


Exactly:

  • it's a design flaw
  • it can't be enforced by your interface suggestion

(talking about a single method used to factory all services)

If it is a design flaw, it is a very minor one. Most of real service providers will have very few services. A method that factory more than one service is not ideal, agree, but assuming it is used to create 3, 4 or even 5 servises, it will stay efficient, readable and maintenable.

Again: surely interface I proposed is not perfect, far from it, but I think current implementation is worse on many levels. I'm advocating what imo is the lesser evil.


Maybe with your solution we can't know but with the current one we know.

(talking about number of methods called)

Sorry? How do you know how many method will be called. This make no sense.

class SomeProvider implements ServiceProvider extends SomeBaseProvider {

    public static function getServices() {
       return ['aService'];
    }

    public static function aService() {
         $factory = ServiceProviderFactory::factoryFor(__CLASS__);
         $instance = $factory->createInstance(); 
         return $instance->setup()->createrService('a'); // inherited from parent class
    }

    public function __construct() {
        $this->init(); // inherited from parent class
    }

    private function setup() {
          $this->anotherMethodForFun(); // inherited from parent class
          return $this;
    }
}

In above implementation there are 6 method calls to instantiate a service.

People do crazy things. Note: not might do, just do. Assuming how people will implement an interface (or the number of method called) is impossible and it also is wrong.

An interface is a contract to be implemented polymorphically by objects. And polymorphically means "in different ways". The only way to know how people will implement an interface is that there's only one possible way. And if there's only one possible way, then you don't need an interface at all.


This is wrong on many levels. Caring about a standards but not about implementations is the best way to fail. We have to take real implementation constraints into account.

You can think at possible implementations. And you can do experiments, and tests.You have to. The point is that you can't make assumption on the "quality" of an interface based on the "quality" of an implementation.

The current implementation is very "open" in the sense it is capable to enforce literally nothing, so it can be implemented in so many ways that it is not even possible to imagine. So you can't say "this interface is good because it can be implemented this way" in the same way you can't say "this interface is very bad because it can be implemented that way".

You can be sure that shitty implementations will come whichever interface you'll ship in the wild.

But as long as you have proven and tested that good implementation may exists this is not a concern.

What is very wrong imo, is to make interface decisions just based on the "possible" implementation that will not part of the standard, but left to people who can and will implement in the way they like.


Have you tried implementing your suggestion into your DI container or your framework (or any existing container or framework)?

ServiceProvider are not implemented in Container, or I am missing something? They are implemented in applications (because not all PHP application uses frameworks), in frameworks and in libraries. Implementation in a library would not be different (pretty much identical, actually) to what I have written here multiple times. I can write a simple application example, I'll do as soon as I find some time, and I'll share here.


Replying to @moufmouf

Using Giuseppe's proposal, we would certainly have something like this most of the time [...] call_user_func is known to be very slow...

Sorry but is the opposite :) Using static methods the only way to call method is call_user_func.

With implementation I proposed:

In both cases, no call_user_func call.

Rather than having a getServices() method that returns a mapping between service names and public static method names, we should have a mapping between service names and callables.

This is a very good idea IMO. Much better than current implementation. That was also my first idea when I looked at this repo. Then I saw that it was explicitly refused as idea in FAQ so I don't even try to propose it...

If I have to be honest, a (dynamic) method that returns an array of callables would probably the best implementation I can think of. Expecially if that method is named serviceFactories() and not getServices(), but naming is probably matter for another issue...

cziegenberg commented 8 years ago

Using static methods the only way to call method is call_user_func.

You don't have to use call_user_func, you can also call a static method as follows: $callable = $className . '::' . $methodName; $callable($argument);

// Or this way (not tested, but should work too): $className::$methodName($argument);

This should work from PHP 5.3.

Just a note... and now following the discussion again. :)

gmazzap commented 8 years ago

@ziege Yes, you right, I simplified there, because just few minutes before writing this comment I wrote this other one and there I wrote that actually it is possible to use save callback in a variable for static methods (which, in any case is also possible with dynamic methods and inside an implementation of my interface proposal).

cziegenberg commented 8 years ago

@Giuseppe-Mazzapica Then I could at least present two additional variants. :)

In my current ContainerInterface implementation with ServiceProvider support I use the string variant 'class::method' to efficently save the callables:

$services = [ 'id' => 'class::method' ];

Instead of: $services = [ 'id' => ['class', 'method'] ];

This requires about 70% less memory (in PHP 7.0.5).

mnapoli commented 8 years ago

@webmozart an opinion? You mentioned you were wary too about the removal of static.

gmazzap commented 8 years ago

As promised, I wrote a simple application example to test usage of service providers.

You can find it here https://github.com/Giuseppe-Mazzapica/container-interop-experiments

There you can find 3 (at the moment) "experiments" each of them contains a Container implementation, a service provider interface and ncessary concrete implementations of it.

The repo readme contains more info.

stof commented 8 years ago

@Giuseppe-Mazzapica each experiment should be implemented in 2 versions:

Otherwise, your experiments will be biased towards the possibilities of a runtime container

stof commented 8 years ago

btw, regarding the proposal we discussed during the SymfonyLive, the great thing about it is that the service provider interface only needs to be specified to return an array of callables. The fact that this callable can be an object with additional methods can be considered as an implementation detail at first. Then, frameworks wanting to dump callables in an optimized way will be able to build a package providing such special object (like the Alias one) and provide special dumping for them. Any unsupported object would just be treated like any other callable.

@moufmouf have you started writing code about this experiment ?

@Giuseppe-Mazzapica regarding your experiment, the code you wrote makes me think that you misunderstood the concept of the $previous callable, as you use it in the opposite way than the expected behavior.

moufmouf commented 8 years ago

Hey @stof!

@moufmouf have you started writing code about this experiment ?

Yes, I worked most of the day on it. It is more complex than working with static methods, but it's definitely possible. Since a few minutes ago, I have a working prototype on Yaco (a PSR-11 compatible container compiler). I'm planning on migrating the service-provider Symfony bundle next week.

You can see examples of such a service provider here (the static keyword needs to be removed from the getServices method)

mnapoli commented 8 years ago

@stof @moufmouf but in order for that solution (callables) to be compatible with Symfony and similar containers that means modules have to use the correct objects (e.g. the Alias object). If they don't they might work but it will never be optimized, so in the end they might be "not recommended" (e.g. by Symfony) or not even used. I'm afraid it would split the standard.

moufmouf commented 8 years ago

@mnapoli That's true. But so far, there are very few compiled/cached containers out there (Symfony, Yaco, PHP-DI and Neon I think). So there is a good chance we can converge on something common.

Furthermore, even if we do not converge on a de-facto implementation, we are not that far away from a standard with container-interop/definition-interop. So even if we do not have that right away, we have the possibility to add that in a few years.

Also, I've proposed a PR for non static getServices method that returns callable. Here: #20 Comments are welcome!

gmazzap commented 8 years ago

@stof

regarding your experiment, the code you wrote makes me think that you misunderstood the concept of the $previous callable, as you use it in the opposite way than the expected behavior.

Might surely be. My understanding is that $previous is a callback that once called inside a factory method (if not null) returns an instance of the same service that the factory method is called to build.

It means that the factory method can either:

In my experiments I tried to implement all those 4 behaviours. What I get wrong?

each experiment should be implemented in 2 versions:

I'm not going to write something involving a compiled container. It's like asking a vampire to grow garlic :) But that repo is open to pull requests and I am also open to add whoever ask for it as repository collaborator with write access.

moufmouf commented 8 years ago

Quick status update: I've updated the Symfony service-provider bridge bundle to work with non-static callables (provided in #20).

I've managed to apply a big deal of optimizations:

So this should ensure that service providers have a very minimal impact on the compiled container. At least, if no service is queried from the service-providers, performance impact is close to 0.

mindplay-dk commented 11 months ago

The proposal looks very different today, and I'm fairly certain these concerns were addressed? For one, we have extensions now.

If you feel the current PSR does not address your needs, please open a new thread in Discussions. 🙂