factory callable signature actually hiding two signatures?

Looking at the signature of factory callables, it immediately occurred to me, the prescribed signature may actually be hiding two distinct signatures.

function(ContainerInterface $container, callable $getPrevious = null)

The optional callable actually seems like a work-around covering two distinct use-cases:

function(ContainerInterface $container, callable $getPrevious)
function(ContainerInterface $container)

These are deceitfully similar, but I think there are two distinct use-cases:

you need the previous value because you intend to modify it somehow
you intend to define or replace any the previous value (you don't care)

I'm not convinced that there is meaningful third use-case, which seems to be what the optional argument is suggesting: you want to conditionally modify an existing value, if present, or otherwise define a new value. It sounds fragile. It calls for if-statements and encourages radically different behavior depending on, for example, the order in which providers are bootstrapped, which seems brittle.

It's an issue of control - we're letting each factory closure assume control and introduce logic. Isn't it safer to say that providers simply provide, unconditionally, so that control stays in the hand of the person bootstrapping the container with providers?

For one, using two signatures for factory callables enables the consumer to reflect on the number of arguments and error out early - if a callable requires a previous value, but none has been registered.

To illustrate my point, take a look at "Module B" of your entry extension example:

    public static function getLogger(ContainerInterface $container, callable $getPrevious = null)
    {
        // Get the previous entry
        $previous = $getPrevious();

        // Register a new log handler
        $previous->addHandler(new SyslogHandler());

        // Return the object that we modified
        return $previous;
    }

Simply calling $getPrevious, which is defined as optional defaulting to null, isn't safe.

Worse, in this example, there is no alternative - what this provider intends to plug-in, is a logger, it isn't ready to bootstrap an entire logging facility implementation; the facility is a dependency, it's only ready to provide a logger for that facility.

It could counter that by only conditionally bootstrapping the logger, which might be okay in the case of a non-critical component like a logger, but in other cases, that sort of thing could lead to a lot of wtf when some provider, apparently bootstrapped, simply stops working as a side-effect of commenting-out some other provider.

I don't know, maybe I'm overthinking this.

I thought I should bring it up though :-)

Hey Rasmus,

Thanks for the feedback. First of all, I'll link to the 2 other tickets that are addressing this issue: #9 and #21. This issue has been discussed in #21, but mostly from a performance point of view.

I completely agree there is some work to be done there and the code example you are referring too is proof of that.

Another solution that was never discussed

First of all, I want to bring a solution that was never discussed before. Maybe we could make the $getPrevious argument compulsory. e.g.:

function(ContainerInterface $container, callable $getPrevious)

If there is no previous value, the $getPrevious callable MUST throw an exception when called. This way, properly throwing an understandable exception becomes the responsibility of the container (instead of being the responsibility of the service provider developer):

    public static function getLogger(ContainerInterface $container, callable $getPrevious)
    {
        // If the previous entry does not exist, an exception with a nice error message is thrown.
        $previous = $getPrevious();

        $previous->addHandler(new SyslogHandler());
        return $previous;
    }

Please note I'm not pushing this forward, just stating we never talked about this possibility. I think it is an improvement on what we currently have, but I'm sure we can find better alternatives.

About your proposed solution

If I understand correctly, what you are advocating here is to use "Alternative 1" declared in #21, i.e. use 2 methods instead of one:

class MyServiceProvider implements ServiceProvider
{
    public function declareServices()
    {
        return [
            'my_service' => function(ContainerInterface $container) {
                return new MyService($container->get('dependency'))
            }
        ];
    }

    public function extendServices()
    {
        return [
            'event_dispatcher' => function(ContainerInterface $container, EventDispatcher $previous)
            {
                $previous->registerListener(new MyListener());
                return $previous;
            }
        ];
    }    
}

Most feedback we received from #21 (from @mnapoli , @Giuseppe-Mazzapica and from @webmozart) was that they would rather stick with only one method. I have honestly no strong opinion on this.

What do you think about the alternative signature (going back to $previous signature)?

class MyServiceProvider implements ServiceProvider
{
    public function getServices()
    {
        return [
            'event_dispatcher' => function(ContainerInterface $container, EventDispatcher $previous)
            {
                $previous->registerListener(new MyListener());
                return $previous;
            }
        ];
    }  
}

If a container tries to call the factory without passing the required second parameter, the user will be notified directly that something is wrong. Also, we can type hint on the $previous.

Also, you say:

I'm not convinced that there is meaningful third use-case, which seems to be what the optional argument is suggesting: you want to conditionally modify an existing value, if present, or otherwise define a new value.

I've been writing quite a few service providers and it's true this use case is rare. I still think it is possible, in the case of arrays of values. For instance, if you want to create an array of Twig extensions, you might want to do something like:

class MyServiceProvider implements ServiceProvider
{
    public function getServices()
    {
        return [
            'twig_extensions' => function(ContainerInterface $container, callable $getPrevious = null)
            {
                if ($getPrevious === null) {
                   $previous = [];
                } else {
                   $previous = $getPrevious();
                }
                $previous[] = new MyExtenstion();
                return $previous;
            }
        ];
    }  
}

Finally

I have really no strong opinion on this.

Let's consider the 2 signatures:

function(ContainerInterface $container, callable $getPrevious);
function(ContainerInterface $container);

The first signature is meant to extend or replace. Therefore, it is capable of doing all the things that the second signature is doing (and more). I understand the temptation to merge everything into one signature that is sufficient. I also understand your desire to split into 2 more meaningful methods.

Really, I have no strong opinion on this (except that the current proposal is suboptimal and needs to be worked upon).

What do you guys think?

container-interop / service-provider