Static Methods on Service Container Too Restrictive

[dragoonis]

<?php
use Interop\Container\ServiceProvider\ServiceProvider;

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

    public static function createMyService(ContainerInterface $container, $previous = null)
    {
        $dependency = $container->get('my_other_service');

        return new MyService($dependency);
    }
}

As per the above example, you're exposing static methods to a provider of multiple services. This means you will need 1 public method per service.

Problem 1) What if I have 100 services, I will need 100 methods, this will be slow. Problem 2) Some service names will be dynamic and thus you can't have a dynamic function name as it must be statically declared.

A better way would be to have:

<?php
class MyServiceProvider implements ServiceProvider
{

    protected $serviceList = [];

    public static function getServices()
    {
        return array_keys($this->buildServices());
    }

    protected function buildServices()
    {
        if(!empty($this->serviceList)) {
            return $this->serviceList;
        }

        $ret = [];
        $ret['my_service'] = MyServiceFactory::class;
        $cacheType = CACHE_TYPE;
        $cacheTypeClass = CACHE_TYPE_CLASS;
        $ret['my_service_cache_' . $cacheType] = $cacheTypeClass::class;

        return $this->serviceList = $ret;
    }
}

[mnapoli]

Keep in mind these service providers are only meant for reusable modules. Users of a framework are free to use a more practical configuration format for their container.

Problem 1) What if I have 100 services, I will need 100 methods, this will be slow.

This might not be a problem in modules. In an application or in the internals of a framework you may have a lot of services to define, but in a module it's usually way less.

Here is an example of a very simple module: GlideServiceProvider. The goal would be to get rid of all those adapters for each framework: https://github.com/thephpleague?utf8=%E2%9C%93&query=glide

Regarding the "this will be slow" part, I don't see why. On the contrary with your solution each service provider has to re-register all services on every request. With the current solution the container only has to do an array_merge of all getServices() calls. It seems faster to me.

Problem 2) Some service names will be dynamic and thus you can't have a dynamic function name as it must be statically declared.

Honestly I have very rarely seen this, but this is of course just my experience. But again, since this standard is only for modules (which are often simpler than whole applications), I don't think it will be a problem. Do you have a real use case that we can discuss?

[dragoonis]

@mnapoli please clarify the distinction between Modules and Framework Bundles.

[mnapoli]

Those are the same thing. Also consider that bundles/modules can be used to share code/integrate libraries as open source packages (e.g. TwigBundle, GlideBundle, etc.) and they can also be used by end-users to organize their application.

This standard only applies to open source modules. The goal is to avoid having to write a framework integration bundle/module for every framework (as shown in the example: https://github.com/thephpleague?utf8=%E2%9C%93&query=glide). On the other hand users (i.e. those that write the applications) can exploit 100% of the features of their framework/container of choice, they don't have to use the standard.

[moufmouf]

Hey Mathieu,

Actually, I was asking myself similar questions to what Paul asked.

Especially: why did you decide to go static? and also why an array of public function names passed in return of getServices rather than an array of callables?

Regarding the fact that the methods are static, I perfectly understand that the goal is to provide default services and that 99% of the time, those services will not change. Also, having static methods makes the system much easier to integrate with Puli (if we ever want to auto-discover service providers). Still, I'm wondering whether we are not loosing some flexibility. If you look at Symfony Bundles, 99% of the bundles take no arguments in the constructor, but yet, you have the possibility to do it.

Also, out of curiosity, I was wondering why you chose this particular signature for the getServices method? Currently, it returns an array of public method names. Why did you chose this over an array of callables? Is it to make it easier to integrate with compiled containers?

[mnapoli]

Hi!

All the methods are static because it comes from the assumption that container configuration is stateless, i.e. it doesn't depend on anything.

If container configuration is not stateless, that means problems with compiled containers (e.g. Symfony) or containers that cache some stuff (e.g. PHP-DI). Static guarantees everything to be stateless, which cannot be guarantee otherwise.

Again, frameworks and end users are free to build their application however they want. This is only a restriction for modules.

Also static methods are easy to call, no need to instantiate any object which, when building a container in every request, is a bit of a performance advantage compared to having to create new objects for no reason. (but I agree this is a minor reason)

why an array of public function names passed in return of getServices rather than an array of callables?

An array of callables means anonymous functions could be returned, which is incompatible with compiled or cached containers.

Even if we explicitly forbid closures (which would be weird in the first place), what about invokable objects? Or object methods? Those are also incompatible with compiled/cached containers.

So the only callables that would really be allowed would be:

• function name
• static method (['Class', 'method'])

I've never seen functions be used for that. So we end up with static methods. If we allow static methods to be called on other classes, then why not but what's the point? It could as well be on the service provider, which could forward to anything. And factories are usually not static, so people would be complaining that we can't call non-static methods. And others would ask for the ability to customize the parameters passed to the factory because their factory is specific.

So all in all I'm afraid it will open a can of worms for absolutely not advantage, since static methods are in the end the only thing we can handle.

But maybe I missed some use cases?

[moufmouf]

Ok, got it!

This explanation you gave definitely belongs to a META document (or similar).

I'll try to think about a use case we are missing, but so far, I'm rather convinced.

@dragoonis : any idea of a (pratical) use case we could not do? I'm not sure I understood your code sample.

[mnapoli]

@moufmouf Agreed about the META document. Also it might be worth including in a FAQ in the README?

[moufmouf]

FAQ proposal here: #7

[gmazzap]

All the methods are static because it comes from the assumption that container configuration is stateless, i.e. it doesn't depend on anything.

We already discussed in #16 how static methods gives absolutely no warranty that providers are stateless. Moreover, using a static method seems to me an incentive to use use static properties to store state, and I think we all agree that's worse than instance state.

Also static methods are easy to call...

This makes no sense to me. Any framework will need to somehow receive an array (or a collection) of service providers. If we go with statics, that list will be an array of class names. If we go with dynamic methods, that list will contain provider instances.

And there's no difference in to call a dynamic method on a received instance vs to call a static method on a received class name.

Is true that object instances will require some more memory, but in real world, the memory gain will be negligible since there will never be thousands (or even hundreds) of service providers, but in the worst case a few dozen.

However, using real objects we open the possibility to make use of type hint (or return type for PHP 7+) for providers, which would be a huge help in the design of containers and frameworks.

For exampe, let's imagine a framework that bootstrap itself from a series of providers:

class AwesomeFramework {

   public function bootstrap(ContainerInterface $container, array $providers) : ContainerInterface {
      return array_reduce(
           $providers,
           function(ContainerInterface $container, ServiceProvider $provider) : ContainerInterface {
               // use the provider here to register servivese in the container
               return $container;
           },
           $container
       );
   }
}

Note how even if providers are passed as array, type hint is possible using any of the array_* function so effectively enforcing type content of array items.

In the same way, a specific implementation of container, could have a method that receives a provider and do the registration:

class AwesomeContainer implements ContainerInterface  {

  public function register(ServiceProvider $provider) : AwesomeContainer  {
      // use the provider here to register servivese in the container
      return $this;  
   }
}

In short, I really think that static methods comes with no real pros compared to dynamics, while is true the other way around.

[mnapoli]

The standard will say that service providers should be stateless (because that required for example for compiled containers like Symfony). But if we ask service providers to be stateless, why would we allow service providers be instances? I don't find this coherent.

As for the type-hint, I agree it's a plus. But I haven't found that significant when writing integration in PHP-DI and another framework. Have you tried it? If so, was it a big issue?

Maybe we should do a pros and con list?

[gmazzap]

why would we allow service providers be instances... But I haven't found that significant when writing integration in PHP-DI and another framework

This is about coding style, I guess. I prefer a defensive programming where before to call a method on class I want to be sure that the class supports that method (and if it is a class at all).

If the factory methods are statics and so providers are class names, I would need to use is_subclass_of to ensure that. That I found much worse of type hint for different reasons, just to name some: no need for custom exception in case of failure, better liability to static analysis tools, and so on.

Maybe we should do a pros and con list?

We could. But since the fact that service providers should be stateless in both cases, I really have hard time to find any pros on the static method approach, while "type-hint not possible" is a surely a cons. Moreover, IMO another cons of statics is the fact that they favor usage of static state.

Maybe the "less memory required" could be a pros for static approach. But I am sure that if we do some profiling using real world scale of magnitude, (and stateless providers) the memory gain will be so trivial that can't be compared as a benefit to have type hint.

In short, my pros / cons comparison would be:

	PROS	CONS
Static	use less memory^	type hint not possible return type hint (PHP 7+) not possible favor static state favor inheritance, no composition possible
Dynamic	type hint possible return type hint (PHP 7+) possible better liability to static analysis tools allow composition allow usage of anonymous classes	use more memory^

^ to be tested

[mindplay-dk]

All the methods are static because it comes from the assumption that container configuration is stateless, i.e. it doesn't depend on anything.

But that isn't always true.

For example, in our system, we have some modules that depend on other modules - since this dependency/coupling happens at run-time in the container, there is only one way to express that dependency in a real, meaningful way... we do it like this:

class UserProvider {
    public function __construct(CryptoProvider $crypto) {
        $this->crypto = $crypto;
    }

    public function register(Container $c) {
        $this->crypto->register($c);

        // ...
    }
}

In this example, CryptoProvider is an interface being implemented by the provider, and there may be multiple different implementations registering different implementations of cryptography-services.

We could of course just allow you to register the UserProvider and select a CryptoProvider on your own, but the constructor signature forces you to do so, since we know that UserProvider will not work without selecting a CryptoProvider.

Another example is providers that require some configuration - perhaps an endpoint URL passed to the constructor, or a configuration object of some sort, or some other dependency that can't be created or defined by the provider itself, which you must provide, so it can register it under the correct name for you. Since these are hard dependencies without which the provider can't create complete registrations that actually work, we like to enforce the registration of these by using constructor arguments. It's called dependency injection :-D ... it's a virtue for provider classes just like it is for every other class.

Also, what about specialization? Maybe I have a base class for a general type of provider, with some factory methods I reuse across different providers.

You're also blocking me from using anonymous classes, which could be useful to quickly override a provider method, for example when setting up a container for a test-suite. Of course you could let the provider register it's dependencies and then overwrite some registrations afterwards with test doubles, but still...

I guess I'd say that classes with only static methods aren't really classes - they're actually a kind of namespace for functions. I think that's part of the reason static methods in interfaces aren't even a thing in most other languages - if there's no object, there's no abstraction, and so an interfaces doesn't really do what an interface is supposed to do; at best, it kind of acts like a "tag", an indication to a consumer as to what the intended use of the class is.

IMO, you should not be enforcing statelessness. There's nothing wrong with having state, such as configuration for the provider, in the provider. Where else? :-)

Having mutable state is of course a different matter - and of course, there's no way to enforce immutability of variables in PHP, except for a "hands off" doc-block, but I find that several of my providers have state, although they never have state that changes...

Of course, I could register these state variables in the container, or require the consumer to do so, but again, see my comment above about creating complete registrations that actually work... I like self-contained providers that "plug and play", e.g. when you've created and registered the provider, you don't have to go and read a manual or something, to learn what else you must register or inject in order to actually make it work... Our DI containers and providers wire the dependencies they register, but the providers themselves are also dependencies while you're bootstrapping your project...

IMO, statics are never a good idea, unless the class really is just a pseudo-namespace for a bunch of functions - and even then.....

[dragoonis]

After reading the replies - I still don't see how forcing a service provider class to use static methods solves problems that non-static have.

[moufmouf]

Hey Rasmus,

Thanks for the great feedback.

Most of your comments make a lot of sense.

There is just one comment I disagree with. You say:

Another example is providers that require some configuration - perhaps an endpoint URL passed to the constructor, or a configuration object of some sort, or some other dependency that can't be created or defined by the provider itself, which you must provide, so it can register it under the correct name for you. Since these are hard dependencies without which the provider can't create complete registrations that actually work, we like to enforce the registration of these by using constructor arguments. It's called dependency injection :-D ... it's a virtue for provider classes just like it is for every other class.

What we would precisely like to do is avoid storing configuration in the service provider constructor. Instead, we would like to advocate that configuration should be part of the container.

This is what we do in the DBAL service provider: https://github.com/thecodingmachine/dbal-universal-module/blob/master/src/DbalServiceProvider.php

For instance, this module expects the container to contain a dbal.host entry (and provides a default one if the entry does not exists).

That being said, I do agree with everything else you said. For the record, I'm currently doing some tests to see how easy it can be to remove the static keyword and what are the impacts for containers (especially compiled/cached containers). Please keep in mind that with the static keyword, we don't have to instantiate all the service providers on each request.

So I'm still working on the issue and this static/no static debate is not over yet (otherwise, this issue would be closed :) )

[gmazzap]

I'm advocating most of the issues have been addressed by others since a while now, glad to see someone agree with me.

I want to thank @mindplay-dk for something I never said:

You're also blocking me from using anonymous classes

IMO is a big pros in favor of the dynamic approach.

Another small point.

If methods are statics, in PHP version < 7, there are 2 ways to call them: via call_user_func or assign them to a variable, which is not very clean:

// option 1
$services = call_user_func(UserProvider::class, 'getServices');

// option 2
$method = [UserProvider::class, 'getServices'];
$services = $method();

If methods are dynamic, implementers will have an instance of the provider, which will allow to call method dinamically:

$services = $userProvider->$method();

With a syntax that is just PHP 5.4+ (see https://3v4l.org/4Naug).

Using static methods, a similar syntax approach (but still worse IMO) is possible only with PHP 7+ (https://3v4l.org/fGuZK)

Beside the syntax sugar, it's interesting how comparing "performace" tab on 3v4l, "Memory" and "User time" numbers are better for the dynamic approach, even if an object is created. This is surely all but reliable, but I think is an interesting cue for a better profiling.

[mindplay-dk]

we would like to advocate that configuration should be part of the container.

It's fine to advocate that - but it's not okay to mandate it ;-)

This is what we do in the DBAL service provider...

Here's what I would advocate - see the comment at the bottom for the "why", since it's kind of off-topic for this discussion.

Note that this example uses a fictional non-static pattern, since it wouldn't be possible otherwise.

This is what most of my providers look like today...

[mnapoli]

@mindplay-dk

All the methods are static because it comes from the assumption that container configuration is stateless, i.e. it doesn't depend on anything.

But that isn't always true.

It must be in order to be compatible with serialized, compiled and cached containers.

I'm sorry if I don't go further than that but I'm feeling like I'm parroting that sentence every day now. Like all of you I'm also interested to improve the current solution, but we need to solve that.

@dragoonis What don't you understand exactly?

[mindplay-dk]

It must be in order to be compatible with serialized, compiled and cached containers.

Must be stateless or must be immutable?

I mean, you can serialize state - it just doesn't work if that state can change and the container has no way to detect the change if it's loading compiled/cached data, right?

Assuming the constraint is that you can't have mutable state - we have no way to enforce immutability in php, of course, but even so... static classes are no more a guarantee of immutable state or statelessness than regular classes - you can still have state in static properties, static variables, globals, superglobals... I'm not trying to be pedantic about this - yes, static classes are less likely to have state, probably, but it's still just a convention, and something you can only encourage, not strictly enforce.

For another thing, it kind of seems like, again, this constraint favors the heavier containers - the smaller ones don't really need to compile or cache for some marginal/theoretical performance gains. Again, not trying to be pedantic, but if we want all container implementations to be able to participate on equal playing ground, we have to consider both ends of the spectrum.

Would there be some class or interface, or some mechanism or pattern that would provide a means of enforcing immutable configuration objects?

Like maybe this pattern:

$container->bootstrap(function () use ($container) {
    $container->add(new XProvider());
    $container->add(new YProvider());
    // ...
});

The bootstrap() function would get the modification timestamp from the file in which the bootstrap function closure was defined, e.g. filemtime((new ReflectionFunction($bootstrap_func))->getFilename()) and compare with the cache timestamp.

Assuming anything that's going to change is in your bootstrap file, that would work, wouldn't it?

Just an idea :-)

[mnapoli]

I understand your point. And I guess you understand mine:

yes, static classes are less likely to have state, probably, but it's still just a convention, and something you can only encourage, not strictly enforce.

However I take your comments as a good example to what I'm trying to get at:

Another example is providers that require some configuration […] It's called dependency injection :-D ... it's a virtue for provider classes just like it is for every other class.

The fact that those dependencies (configuration or other service providers) can change means definitions are mutable (indeed mutable might be a better term), hence a problem with some containers. And about that:

if we want all container implementations to be able to participate on equal playing ground, we have to consider both ends of the spectrum.

Implementing support for "simple" containers is very easy. That's not really an issue for those containers. So I agree with you and I think there's more a challenge for compiled/cached/serialized containers.

I guess I'd say that classes with only static methods aren't really classes - they're actually a kind of namespace for functions.

Agreed, which is why I suggested that if we go the callable way (see #18) then I'd rather have service providers be an array of callables rather than a class. But in the meantime static methods have a few limitations that are in sync with the limitations that we are dealing with.

So yes I do see a problem with configuration injected in SP: if it's not mutable then it can go in the container (defined as container entries). The only usefulness to SP configuration I can see is if you want to change the definitions at runtime, but then we have a problem (mutability).

---

Anyway the discussion has evolved a bit since it started and more people have voiced their opinion (which I value a lot). If I'm the only one thinking static is helpful and I can't convince others, then I'm 👍 to merge a PR that removes static. After all the worst we can get (if my concerns are correct) are poor service provider implementations, which can be fixed.

[mindplay-dk]

I'd rather have service providers be an array of callables rather than a class

And by callables you mean strictly callables that do not reference an object, and are not closures? Because neither of those would serialize, right? So basically ['class','method'] which works only for static methods, but... aren't we right back where we started then? :-)

[mnapoli]

Yep I meant "if we go the "callable" way". See the equally long discussion in #18 ;)

[moufmouf]

@mnapoli The issue for both of us (the cached/compiled container writers) is to try to avoid instantiating the service providers or calling the getServices method if this is not needed.

I was working on a registry class to store ServiceProviders and instantiate them lazily when needed. I realized it could be useful for you too. I put it in a separate package if you ever find this useful:

https://github.com/thecodingmachine/service-provider-registry

This should help us lower the performance impact of having non-static service providers.

[moufmouf]

Anyone interested in writing this PR? (I'm a bit busy testing this in Yaco and Symfony right now... :) )

[mnapoli]

This issue can be closed as there are no static methods anymore.