Slim: Idea: implement __set() for Container class

[adambro]

SlimContainer class implements __get() and __isset() magic functions, which are handy for fetching dependencies like that:

$container = new \Slim\Container();
$dao = $container->dao;

Unfortunately you can't use the same syntax for adding objects to container. You have to fall-back to syntax from Pimple, like ArrayAccess. It results in mixed syntax, like:

$container = new \Slim\Container();
$container['dao'] = new stdClass();
$dao = $container->dao;

Could we implement __set() in order to use the same object (pseudo)property syntax for setting object on container, please?

[ppetermann]

that won't work.
the slim container uses a(ny) container that implements the container-interop interface
the interface does not contain a "set", that is left to the single container implementations
thus there is no way the slim container could do that without limiting the containers
which can be used.

[geggleto]

I personally think the array-access method of setting is fine.

[adambro]

@ppetermann I understand the InteropContainer interface has got only get() and has() methods defined, but you have to somehow _set_ objects in DI Container. By default Slim uses Pimple, where only way is to use ArrayAccess syntax, which results in code like:

$container['notFoundHandler'] = function ($c) {
    return function ($request, $response) use ($c) {
        $c['logger']->warning("No matching route has been found: " . $request->getUri(), $request->getParams());
        return $c['response']->withJson(['msg' => "No matching route has been found"], 404);
    };
};

This is fine as @geggleto mentioned, but Slim provides this magic __get() method, so I can use object property notation. That's personal preference simply. What surprised me is that the same object property syntax does not work for assigning objects in DIC. Example below won't work:

$container->dao = new stdClass();

It's just the matter of consistency. If we have magic for __get(), what about same magic for __set()?

In order to have truly replaceable containers we'd have to define some common way of setting (an interface). Relaying on container implementing ArrayAccess (which is an implicit assumption currently) seems to be good trade-off.

[KuroThing]

You are already implementing non-interface access methods, to replicate the functionality of get(...) and has(...), and expecting \ArrayAccess to set objects. What's more, the \Slim\Container was once upon a time declared final, so extending wasn't even possible and to use a new container you just re-defined the existing elements in a container, so if a container doesn't support these methods they can ether be extended in, or not override the \Slim\Container, and polyfill the required services.

I don't see the problem in implementing further non-standard access methods that improve consistency between access methods (Either Array or Object syntax), but if you are going to insist that this change shouldn't be implemented, even if it's just to proxy to the offsetSet() of \ArrayAccess, than shouldn't the existing Magic Methods be removed for violating whatever __set() would?

If you want to maintain full interoperability between containers, you should not be presuming that the container implements \ArrayAccess, you should run through a list of common setters for Containers, and using method_exists($this, "..."); to check to see if the method exists, not assuming it does, or using something like Acclimate.

Sorry in advanced if this is too 'aggressive', but the fact that some magic methods are fine, but others are not is serious case of hypocrisy. It should be All or Nothing, unless a valid reason is provided.

[ppetermann]

I had a look at the code, and my personal conclusion is that there is something really wrong.

a) slims container class should use the interoperability container, not extend it. (and for some weird reason I assumed that it was refactored to that way a while ago.) That would allow the added methods such as __get to be used without breaking everything when replacing the interoperability container.

b) using the array access is something that is/should only be done when setting up the container. And that is only when using a container that needs to be configured that way. Other Containers than pimple don't necessarily work the same way, and that is fine.

c) all access during the run time (after the setup) should be done through methods slims container provides,where slims container should use the interoperability interface to do so. As currently that's not slims container works, working with slims container in your application should always use the interfaces methods,otherwise stuff will break when exchanging the container. This also means in the current state of the container, the magic methods that are there shouldn't be.

[silentworks]

You can easily add this in your own implementation if you wish, but I don't think this belongs in the core. This was the reason why we made the Container class not final anymore, its so you can extend and do as you please with it.

@KuroThing you do realise this is just code right, you can do as you please with your own, not sure how hypocrisy comes into play when the framework is now opinionated. If Container was still final then I would understand your case, as it no longer is you are pleased to do what you will.

All magic methods that we decided to implement are purely for shortcut use, and since every container that uses container-interop follow the same get and has, it is easy enough to have magic methods in Slim to provide access to these. set is not defined by container-interop hence why you need to do all your set on the Container directly.

[silentworks]

@ppetermann you can do _b_ and _c_ with Slim as it currently is, but you can also go about it not using _b_ and _c_. We realised anyone coming from Slim 2 are likely to want to do $this->cache->get in their route closure when using anonymous functions, for developer experience we thought that is way simpler than doing $this->getContainer()['cache']->get(). In cases where you are not using that style of coding, you would inject your dependencies using whatever method your chosen Dependency Injection Container provides.

[geggleto]

Well put @silentworks :)

[KuroThing]

@silentworks I had not considered that the reason the magic methods for __get and __isset were only implemented because they match the Container-Interop interface, sorry.

I also realize that it's just code, but i find it rather silly and even hypocritical that there is inconsistencies between access methods for no reason (Which is not the case here). I was also quite joyed when the \Slim\Container was no longer final, because it made my life a lot easier, even just being able to use __set and PhpDocs.

[adambro]

I seem to agree with @ppetermann that something is not quite right here. Slim container is in fact Pimple (extends Pimple) and requires ArrayAccess for setting object, as we can see in registerDefaultServices method:
https://github.com/slimphp/Slim/blob/3.x/Slim/Container.php#L97

Few lines below there's another hardcoded dependency on Pimple in exceptionThrownByContainer method.

I'm not saying it is wrong, actually such integration makes it convenient, so understand that I'm just looking for more convenience in using $container->property = $obj object notation for setting dependencies on DI Container.

[adambro]

Maybe it's an idea for Slim 4 (see #1686)?

[akrabat]

__get and __isset exist for BC with Slim 2. We don't encourage their use in new applications.