Standardize on setting values

[Anahkiasen]

So, as I mentioned in #55 it is crucial for me for the ContainerInterface to describe how values can be bound to the container, in order to later on have the possibility for a PSR11 ServiceProviderInterface which would allow for truly agnostic interoperable packages.

Current behavior

There is currently two schools of containers:

• Those where you decide when setting a value if it'll be a singleton
• Those where you decide when retrieving a value if it'll be a singleton

Here are the signatures currently found in the wild, in the most downloaded containers on Packagist:

Container	Bind an instance	Bind a singleton
league/container	add(string $alias, mixed $concrete, bool $shared)	share(string $alias, mixed $concrete)
illuminate/container	bind(string $alias, mixed $concrete, bool $shared)	singleton(string $alias, mixed $concrete)
joomla/di	set(string $alias, mixed $concrete, bool $shared, bool $protected)	share(string $alias, mixed $concrete, bool $protected)
pimple/pimple	offsetSet(string $alias, mixed $concrete), $concrete wrapped in factory(mixed $concrete)	offsetSet(string $alias, mixed $concrete)
php-di/php-di	set(string $alias, mixed $concrete)	set and retrieve with make(string $alias, array $parameters)
aura/di	set(string $alias, Closure $concrete)	set(string $alias, mixed $concrete)

As we can observe, by far the most common pattern is to have two separate methods. As we already have a get it would make sense to me for the setter to be set, and it is also the most common name.

For binding singletons, share seems to be the most used at the moment.

The type of $concrete

As seen in the table, most of the time $concrete is not callable but mixed, this is to allow people to bind values to the containers, like this:

$container->set('foo', 'some-value');
$container->set('bar', ['some', 'array']);

Binding these kind of values will just, bind them. However, binding callables:

$container->set('foobar', function () {
  return new Foobar();
});

Will result in the callable being executed on retrieval – ie. if you do ->get('foobar') you won't get a closure, but an instance of Foobar. I think this is a flexible system and that's the behavior I have most observed in the containers I tested.

The case of serializable containers and read-only containers

Some containers are meant to be serializable, like Symfony's. Others are just read only in general. For this purpose, I suggest to make this a separate interface entirely, that could be facultatively implemented by the containers, like for the delegate lookup.

Proposed interface

With all this in mind, this is the interface I propose:

interface WritableContainer extends ContainerInterface
{
    /**
     * Set a value on the container.
     * If the value is a callable, it will be called
     * on retrieval.
     * The value bound will be fetched at every
     * retrieval, if it is a callable, it will be
     * called every time as well
     *
     * @param string $alias
     * @param mixed  $concrete
     *
     * @return mixed
     */
    public function set(string $alias, mixed $concrete);

    /**
     * Set a value on the container
     * The value will always be the same upon retrieval
     * If it is a callable, it will only be called once
     *
     * @param string $alias
     * @param mixed  $concrete
     *
     * @return mixed
     */
    public function share(string $alias, mixed $concrete);
}

Name could probably be something way better, input welcome.

---

Note: this is probably imperfect in a lot of ways, I'm fully aware of that, it's just to get the discussion going somewhere. So throw your input at me, let's get the discussion started.

Not all containers are the same, and as discussed in the other issue the main reason the original ContainerInterface didn't standardize on setting values is because all the containers did it differently.

This is to me exactly the reason why this should be in PSR11, so that we can take all this mess of various behaviors and unify to something the user can rely on. This does not mean all containers should act the same, any containers can then have extra features (protected instances etc), this does not go against that. It's just to ensure that we at least have a baseline of what can be expected from a PSR11 container.

[mnapoli]

@Anahkiasen are you in ForumPHP in Paris today? We just had a discussion about that topic, maybe it's worth meeting.

[Anahkiasen]

Unfortunately no, I'm near Rennes I'm afraid

[mnapoli]

Please have a look at https://github.com/container-interop/definition-interop and https://github.com/container-interop/definition-interop/pull/2 which should sum up a little the discussions we had. Keep in mind though that this is just an idea that we want to try, we'll see if it leads somewhere.

[Anahkiasen]

That's an interesting approach; so packages would define "definitions" of what should be present in a given interop container and those would be passed to the container and bound following whatever logic feels best to said container?

[moufmouf]

Yes, that's the point. Definitions can be converted to instances easily by any container, and we keep maximum compatibility with all containers.

Also, definitions can be "compiled" (i.e. we can dynamically write the PHP code of a container that contains the code generating all the entries defined). And if this compiled container is implementing PSR-11, we can plug it to other PSR-11 compatible containers. So this is what we are testing right now. Very experimental though, we started yesterday :)

[Anahkiasen]

And so this would also replace service providers am I correct?

[mnapoli]

@Anahkiasen that's one of the ideas, see also: https://github.com/container-interop/definition-interop/pull/1#issuecomment-159703816

[mindplay-dk]

There is currently two schools of containers:

Those where you decide when setting a value if it'll be a singleton Those where you decide when retrieving a value if it'll be a singleton

I'd like to point out that there is in fact a third school of containers, in which everything is a singleton.

This is my strong personal preference, and one of my firm commitments for unbox, which will never have the option to create anything that is not a singleton, and here's why, in a nutshell:

The moment your container creates more than one instance of anything, it's no longer a container.

Components that create more than one instance of something are factories, not containers - I can't regard this as a container feature, at all.

I regard it is a violation of the single responsibility principle - a container is supposed to be a registry for specific instances of components; the ability to also act as a factory, in my opinion, is something you've added for convenience, e.g. so you don't need factories, not because it really has anything to do with providing a registry for components.

But that's actually secondary and an opinion based on a design principle.

The real problem is that you've got the same API (the PSR-11 get() method) apparently doing the same thing on the surface, e.g. providing you with a component, but the problem is, you now have hidden, internal state governing how that method works: whether it returns the same instance or a different instance on subsequent calls.

Unintentionally treating a non-singleton as a singleton can lead to very serious, hard-to-debug issues, etc. - and worse, treating a non-singleton as a singleton can lead to accidentally sharing state, which leads to side-effects, which leads to near-impossible-do-debug scenarios.

But on top of this, actually, the best argument for not having this feature at all, is you don't really need it at all. Writing a factory-class is usually trivial, and (in my experience with a team of 4 developers working for about a year now with unbox, having used two other containers with that feature prior) usually leads to a much clearer dependency graph, e.g. every hard dependency clearly visible in a constructor signature.

I'd urge you to try it in your applications: Factor away any uses of your DI container as a factory, by introducing real factories. See how you like it.

I'd never personally go back.

As a footnote, unbox does have a factory-facet, but this method always returns a new instance - but it has nothing to do with the registry; it's there for utility, e.g. allowing easy implementation of a controller-factory, middleware-factory, etc... in other words, create() consistently returns new objects, and get() consistently returns singletons, so there's no question about which you're getting, and no potential side-effects from overloading the get() method with factory-behavior.

[garrettw]

Logically, your idea makes sense – but in practice, to accomplish IoC with auto-wiring, I need both a singleton container and factory functionality in the same library.

[mindplay-dk]

@garrettw yeah, the library can have factory functionality - as noted, my library does, the difference is, I have a dedicated method for that, I don't overload the get() method with factory-functionality.

$cache = $container->get(Cache::class);

Am I getting a new instance, or the same instance?

If you were expecting a new instance, that's not obvious or visible at all, which makes this a very risky transaction - you basically have no idea if you're getting a shared cache instance or two instances backed by different storage, so now you can't be sure if you need to deal with issues like key-collisions, you can't be sure about the scope or impact of clearing the cache, and so on.

If you were expecting a new instance, the following is much clearer:

$cache = $container->create(Cache::class);

Or if Cache is an interface (which is more likely) then you can correctly declare your dependency on a CacheFactory rather than depending on the container:

$cache_factory = $container->get(CacheFactory::class);

$cache = $cache_factory->createCache();

Again, I'd urge you to actually try it out - introduce real factories, factor away any use of the container as a factory, I think you'll find it makes the whole thing much simpler and more transparent :-)

[garrettw]

The problem that introduces is that you can't have a single entry point where the IoC container creates the whole object graph, making new instances or using singletons where needed (according to configuration).

[mindplay-dk]

The problem that introduces is that you can't have a single entry point where the IoC container creates the whole object graph, making new instances or using singletons where needed (according to configuration)

You can make that happen, by introducing factories and registering those in your DI container - you just can't have the DI container do it for you, correct.

In my opinion, making the container also act as a factory creates potentially far more serious problems than having to write a few factory classes - which is not really a problem in the first place, it's just more convenient than actually doing the work.

I wouldn't touch auto-wiring either, by the way - it makes things "work" by just assuming you want a single instance of everything. Let's hope that's true, or else, weird bugs, side-effects.

I just prefer to keep things simple and explicit.

Again, all I can do is urge you to try it. You might like it :-)

[garrettw]

Ok, registering factories in the DIC doesn't sound so bad. But as far as auto-wiring, what is your feeling on a container that defaults to new instances (rather than singletons as you suggested) and that can inject singletons but just has to be configured to do so?

[mindplay-dk]

But as far as auto-wiring, what is your feeling on a container that defaults to new instances (rather than singletons as you suggested) and that can inject singletons but just has to be configured to do so?

Whether your default is to create a singleton or multiple instances, it makes no difference - in some cases, your assumption is going to be wrong, and this will most likely lead to silent bugs, e.g. faulty behavior rather than an error/exception.

If you were expecting state-accumulation in a single object (such as a registry) and you get new instances, something will break.

If you were expecting new instances but were expecting to collect state, something will break.

So it's bad either way.

My position is I don't want convenience if it comes at the cost of safety - I'll take the simple solution, do that tiny bit of work up front, and avoid the extra complexity and potential bugs down the line.

[mindplay-dk]

If you must have auto-wiring, the safest default is likely a single instance, as this will make sense for things like a registry, cache, stateless helpers and services etc. - which are some of the most common dependencies.

You will always need to live with a degree of risk though, since the container will blindly create a single instance of DateTime, ArrayObject, SplStack or any other class with optional or empty constructor arguments.

Try getting two different objects randomly bootstrapped with an empty shared ArrayObject or SplStack with different types of content, and watch as one breaks from side-effects when you start putting one or the other type of item in there.

Fun times 😄

[garrettw]

And it would be relatively easy to throw in a single instance of a factory for the cases where you need new instances every time.

@TomBZombie As I respect your opinion a lot, what say you to this? Any thoughts?

[garrettw]

@mindplay-dk Upon further thought, I think I'm going to stick with my original plan for my DIC, which is auto-wiring fresh instances unless configured otherwise. Why?

• I want auto-wiring, because the alternative, as you present it, is registering every class that should be a singleton and then -- what if I need a non-singleton object injected deeply in the object graph? I don't see a way to do that with unbox.
• I could make the container's objects singletons by default, but then, like you say, you run into unexpected behavior with classes that you wouldn't normally want singletons of, like data structures. So I'd prefer to make a dev specify that a class should be shared, even if it might commonly be shared (like PDO), rather than introduce unexpected behavior.

Your feedback is welcomed.

[mindplay-dk]

what if I need a non-singleton object injected deeply in the object graph?

Use a factory class - unbox deliberately omits non-singletons, because these don't get registered in the container; you're not using the container as a container, but as an easy way to avoid writing a factory, which is what you really need.

I'd prefer to make a dev specify that a class should be shared, even if it might commonly be shared (like PDO), rather than introduce unexpected behavior.

As explained, you will get unexpected behavior no matter what you choose.

Good luck :-)

[garrettw]

So I guess there'd be no way around it: I'd have to have a glut of register() calls or a glut of factories. Seems tedious.

[TRPB]

So, as I mentioned in #55 it is crucial for me for the ContainerInterface to describe how values can be bound to the container

"How values are bound to the container" is an implementation detail, and it's not really "how the values are bound to the container" it's "how the container locates dependencies". Whether they were bound externally or constructed internally is an implementation detail and should not be part of the interface.

That way, it's up to the individual container on how to handle any ->get calls If you call $container->get('Foo'); it's down to the container's configuration on what happens next: retrieve an existing or new instance... return an instance that's been bound or construct a new one. The problem here is that the calling code should not care about how ->get works. The fact that get() may or may not work depending on whether the relevant register()/share()/whatever method has been called first highlights action-at-a-distance: A get() call may or may not work depending on whether the right register() call has been made somewhere else in the application.

The problem is the underlying $container->register($instance) call. The container is no longer in control of creating instances or its own state. Let the container do its job and create the instance. If you have code like this:

$foo = new Foo(new Bar));

$container->register('Foo', $foo);

$foo = $container->get('Foo');

you don't need a container, you need an array. The above code should just be:

$foo = $container->get('Foo');

By keeping this logic inside the container, the interface no longer needs to be concerned with implementation details (where the container gets its instances from and how it's configured).

edit: The above is also why has should really not be in the original implementation. If the container is unable to provide an adequate value for a get() call it should throw a relevant exception, try-catch is better than if ($container->has())

[mindplay-dk]

Seems tedious.

"Seems" being most likely the keyword.

I think you'll find with experience that, most of the time, auto-wiring isn't actually saving you much work, nor is the container helping much by acting as a factory for dependencies that don't actually get registered in it.

Another issue with auto-wiring is things that shouldn't be auto-wired at all, can be - I mean types that are internal dependencies of a package, implementation details. This can be a real problem on a large team with many contributors, where it's not necessarily obvious which classes are intended to be used by client-code and which ones aren't.

One benefit of explicitly bootstrapping dependencies is I can actually read your bootstrapping code and see how things are bootstrapped, rather than having figure out the source of any undeclared dependencies and how they connect to the rest of your architecture.

We started out with PHP-DI and auto-wiring, and it was one of the first things the team agreed to globally switch off, because it did cause problems. So we do bootstrap everything manually in on our projects, and the whole team is very happy with that.

I'm fairly certain your mind is exaggerating the extra amount of work you'd have to do. If you'd actually try it, I think you'd realize it's ho hassle at all.

Anyways, each to his own and all - but having tried both, having seen auto-wiring fail in serious ways, and having been successful with manual bootstrapping for about a year now, I wouldn't go back.

A lot of things seem tedious because as programmers we're lazy by nature, and a lot of the time, that's a good thing - personally, I always try to weigh complexity against real benefits, and I think this is a perfect example of how we tend to favorize "easy" in the short run, over "simple" in the long run, and end up building things are more complex than they needed to be.

If you haven't watched this, watch it - if you have, watch it again, then watch it some more. It's the single most important thing you can learn and practice when it comes to programming. :-)

[TRPB]

Seems tedious.

Regardless of which container you use, you need some method of informing the container which instances are singletons and which are new instances. Ideally we want to make that as easy as possible but that information does need to be provided in some manner.

[TRPB]

Another issue with auto-wiring is things that shouldn't be auto-wired at all, can be - I mean types that are internal dependencies of a package, implementation details. This can be a real problem on a large team with many contributors, where it's not necessarily obvious which classes are intended to be used by client-code and which ones aren't.

Unfortunately this is a symptom of PHP not supporting private/protected classes. Sometimes you do want to prevent arbitrary pieces of code asking to be injected with some specific instance and perhaps containers should implement that- however it's not an issue with auto-wiring per se.

[garrettw]

@TomBZombie just to clarify, what I was referring to as tedious was not merely the need to specify non-default behavior, but rather the need to create simple factory classes every time I just need fresh instances injected - and then register all those factories along with all classes that should be singletons. In short, I'm saying that unbox's way is much more work/code than Dice's. I'm still trying to decide if unbox's way would have any measurable benefit for me.

[TRPB]

Absolutely, doing so with less code is less annoying, which is why I designed Dice the way I did :) But presumably you could write something to simplify the task in other containers, couldn't you write a generic factory with relevant constructor args to replace all your factories?

[mindplay-dk]

it's not an issue with auto-wiring per se.

It is in PHP today - we don't have private/protected classes.

[mindplay-dk]

couldn't you write a generic factory with relevant constructor args to replace all your factories?

I have an implementation actually, on an experimental branch of unbox:

https://github.com/mindplay-dk/unbox/blob/psr-11-refactor/src/Resolver.php

This was designed to work with any ContainerInterface implementation.

(Note that this will make the DI container itself a dependency, which I wouldn't personally want to do in a scenario that calls for a factory.)

[TRPB]

It is in PHP today - we don't have private/protected classes.

But if this is a problem, there's nothing to stop you writing a container in such a way to mimic the feature. (edit: and in fact, this is already possible with Dice)

(Note that this will make the DI container itself a dependency, which I wouldn't personally want to do in a scenario that calls for a factory.)

At which point you may as well just have that logic inside the container ;)

As for autowiring vs manually configuring dependencies. Personally I can't stand manual configurations, it makes the entire application cumbersome and needlessly inflexible. Want to add a new dependency to a class? You have to make two changes: The class definition and the container. With Dice, for example, you change public function __construct() {} to public function __construct(\PDO $pdo) and it just works or to add a class to the application I can add the class and ask for it as a dependency anywhere else and just use it without having to consider exactly where in the system it needs to be passed in.

If you're using factories without autowiring the problem is even worse: You now have to get the dependency into the factory so that the factory can pass it into other objects.

[mindplay-dk]

With Dice, for example, you change public function __construct() {} to public function __construct(\PDO $pdo) and it just works

The same is true for Unbox - the only difference is I have to do register(MyClass::class) to explicitly indicate I want this class treated as a singleton, and it "just works".

Despite what you might think (not having used it) it's very friendly to refactoring - in many cases, all you have to do is indicate you want a managed singleton and 95% of time it "just works"; the other 5% of the time is when auto-wiring would have lead to bugs, so I'm more than comfortable having to manually indicate what can be safely injected.

As said, we used auto-wiring early on, and we found that it did not in fact save us any time, made things less transparent, and increase the chance of hard-to-debug issues. Indicating that a new dependency can be safely managed by the container is practically zero work, so auto-wiring would save me about 5 seconds of work in a typical day. Who cares, really. I opt for safety and transparency.

If you won't try it, you won't know the difference :-)

[TRPB]

Perhaps we have different definitions of autowiring then. Using Pimple as an example, you have to explicitly add a closure for each class you want to create via the container and specify exactly what dependencies it needs. A change to a constructor means a change to pimple's configuration.

If you can call $container->get('ArbitraryClass'); and it works without any prior configuration for ArbitraryClass and works out what dependencies are needed, that's autowiring isn't it?

[mindplay-dk]

If you can call $container->get('ArbitraryClass'); and it works without any prior configuration for ArbitraryClass and works out what dependencies are needed, that's autowiring isn't it?

Yes, but that isn't what I just described.

If you have a class X with a constructor dependency on PDO, you have to register PDO as well as X - difference between Unbox and e.g. Pimple is I don't (in most cases) have to do anything more than simply indicate that X can be safely treated as a dependency by other classes, e.g. register(X::class) with no arguments.

Similarly, if I add a class Y that depends X, all I have to do is register(Y::class), since I have already indicated that X should be managed by the container.

Auto-resolving constructor-dependencies on classes that have been registered, is not auto-wiring - if I add a class Z that depends on X and Y, I have to minimally register(Z::class) as well, whereas with auto-wiring, the container would automatically register Z.

Unbox doesn't make you do a ton of manual bootstrapping to specify dependencies, when these can be safely resolved and injected, so it is very refactoring-friendly and very quick to set up. The only thing it doesn't do, compared with auto-wiring containers, is make assumptions about whether something can be safely bootstrapped, whether as a singleton or implicit factory.

Only a developer can know which is correct, or whether a certain dependency can even be injected from the container at all and doesn't require a factory of some sort.

Ask anyone on our team who has worked with and without auto-wiring - it saves zero time in practice.

What does save time is auto-resolution of constructor arguments - we have that, and it's doing 95% of the work that you currently think auto-wiring is saving you, but what's really saving your time is auto-resolution; we're talking about two different things.

You can't know unless you try it, so either try it and see for yourself, and/or let's end this discussion.