Don't let TwigMiddleware messing with container

[bangzek]

First the $this->view only works for Pimple with their __get() implementation.
Second most container use the same instance as default or can be configured to.
Third messing with container is considered a code smell.
Fourth it's better to save the view in Request attribute rather than mess around with container.

[adriansuter]

To me, the view (template engine) is a component that belongs into the container. After all, the container is exactly for this kind of scenario and should serve as dependency injection.

[bangzek]

Well normally the template engine doesn't have business messing around with container.

If there's a good excuse they can use the container engine as service locator anti pattern, but that just using the ->get() method. See my LazyTwigMiddleware for a good excuse to use container.

One good reason for a library not setting the Container cause the user knows more about their current setup than library author. For example PHP-DI has special compiled mode that must be run before any library code. If the view setting the container after it, it just make the compiled code useless.

[l0gicgate]

We could do something similar to RouteContext @adriansuter.

The one thing that worries me is that we're going to break the synthax that users were used to before. We could leave both in place perhaps?

TwigContext::fromRequest($request) and pass in view as well within the $request object.

[adriansuter]

But I still don't understand, why the template engine should be added to the request object. To me, that belongs to the container (dependency injection - it is a service after all).

The only reason I can see is, if someone wants to use Slim and Twig without DI container.

I agree, that actually we should not set the instance to the container in the middleware, but rather we should let the user set it beforehand (because of php-di container compilation). The middleware should get twig from the container.

[l0gicgate]

@adriansuter it’s really just a hackish way to transport data honestly. It pains me that we have to do it in Slim as well.

And yes, that is the only reason why someone would want to transport it in another way. Not use a container or having container constraints due to pre-compilation or other.

Ultimately we can’t please everyone though.

[bangzek]

There's probably a misunderstanding here. Let me clarify:

First are we in agreement that the current way of TwigMiddleware set container in ->process() is wrong?

My suggestion that we set it in Request attribute probably not good either, that's why I made it optional.

Let's get back to the v2 way, let user handle the responsibility to set container's view service with the Twig object. The middleware just add runtime loader and extension to the passed Twig object. So the user must get the Twig object from container and pass it to TwigMiddleware::create() or the constructor if he want to build it from scratch. Most DI container will get the same object so it's not a problem.

[bangzek]

Please review my latest pull request. I believe it's handled all the comment of this bug.