[RFC] Improving Controller Reusability

[rossriley]

I think this is mainly in reference to #6058

The original reason for coming up with the BoltResponse object was so that we could have a single response object that was passed around and modified until it came time to render when we would have a single object with a template name, some context variables and some globals which could then render.

I'm just in the process of doing some migrations to 3.3 and it doesn't look like we've got any capability now to do in-controller modification. Here's a few simplified examples of what was possible before.....

# in MyApp/Controller/Frontend
public function record(Request $request, $contenttypeslug, $slug = '')
{
        $response = parent::record($request, $contenttypeslug, $slug);
        $response->addGlobals(['siteconfig'=>$this->getCustomSiteConfig()]);
    
        return $response;
}


# A more indepth example
public function record(Request $request, $contenttypeslug, $slug = '')
{
	$response = parent::record($request, $contenttypeslug, $slug);
	if($record = $render->getGlobalContext()['record']) {
            $showRelated = $record->get('showother');
            if ($showRelated) {
                $otherRecords = $this->app['storage']->getContent($showRelated.'/latest/4');
    		$response->addGlobals(['other'=>$otherRecords]);
            }   
	}
    
	return $response;
}

What this allowed was for you to override the default Frontend controller methods without having to completely rewrite them.

By pre-rendering the template we're now only left with the option of completely re-implementing the entire 60ish lines of method code which isn't really ideal when we don't really need to change things like the record lookup, the templatechooser etc.

Plus it means that if we change anything in the base controller methods without having a decent public API to extend more gracefully, then we lose the ability to update how things are done without breaking a load of application code in the wild.

So, whilst there's good reasons with Twig deprecations etc to need to do this, is there anything we can put in its place to keep the principle of BoltResponse in the controller code?

[GwendolenLynch]

Would updating the render() with an event suffice?

e.g.

    protected function render($template, array $context = [], array $globals = [])
    {
        $twig = $this->app['twig'];
        $dispatcher = $app['dispatcher'];
        $template = $twig->resolveTemplate($template);

        foreach ($globals as $name => $value) {
            $twig->addGlobal($name, $value);
        }
        $globals = $twig->getGlobals();

        // Event class with getter/setters for $template, $context & $globals
        $event = new ControlerPreRenderEvent($template, $context, $globals); 
        $dispatcher->dispatch('bolt.controller.pre_render', $event);

        $response = new TemplateResponse($template->getTemplateName(), $event->getCcontext(), $event->getGlobals());
        $response->setContent($template->render($context));

        return $response;
    }

So in the overridden controller method:

    public function record(Request $request, $contenttypeslug, $slug = '')
    {
        $dispatcher = $app['dispatcher'];
        $dispatcher->addListener('bolt.controller.pre_render', function (ControlerPreRenderEvent $event) {
            $event->appendGlobal('siteconfig', $this->getCustomSiteConfig());
        });
        $response = parent::record($request, $contenttypeslug, $slug);
    
        return $response;
    }

[rossriley]

I think that would suffice, but I'm not sure it's the ideal solution long term since we don't really want apps to have to descend into more complex event driven development.

Maybe we could add the event into the 3.3 branch as a quick patch, and then perhaps look into separating the method responsibilities a bit, in the same way as we try and do in the Repository classes, have one method that builds the context and another that injects the context into the response.

public function record(Request $request, $contenttypeslug, $slug = '')
{
    $context = $this->buildRecordContext($request, $contenttypeslug, $slug);
    $template = $this->templateChooser()->record($context['record']);
    
    return $this->render($template, [], $context);
}

[GwendolenLynch]

I think that would suffice, but I'm not sure it's the ideal solution long term since we don't really want apps to have to descend into more complex event driven development.

Well to be fair, if you're at the level of being able to extend controllers to this level of complexity, and don't get something as simple as event dispatching … it's probably time to permanently step away from a computer and take up landscape gardening 😆

Humour aside, I've been thinking through this all day, and am otherwise coming up blank. We're really heading (deeply) into NIH, and and potentially a host of anti-patterns, again. The repositories are at the model level, but templates (should mostly) be view, and this is re-injecting business logic back into the view.

I know, I get to buy the next round of beers for making both an "anti-pattern" and "MVC" comparisons in the one reply. 😆

Just thinking though, maybe extending how we're planning to use (specifically) the context DTOs might be some middle ground … maybe I just need more sleep … or 🍺 … but that would still require some way of interfacing with the event system.

@CarsonF I know you're on holidays, and humble apologies for tagging you on this, but I am pretty sure GMO have to approach some of this, any hints on how that is being achieved?

[GwendolenLynch]

Quoth @CarsonF in Slack:

Hey I do want to comment on that frontend controller issue but no time to open my laptop on vacation and type out my thoughts. Hope it can wait until Tuesday :)

Works for me, The discussion is a valid one, and I'd prefer we successfully cover as many bases as practically possible … Esp. when you look at the size of the new deprecation counts from my #6389 PR

[rossriley]

OK, here are a few suggestions I can think of.

1: Manage this purely with an event that gets called in Base::render and BackendBase::render

2: Add a method build('action') to both the above classes, then modify the render method which carries on as now, unless it is in build mode, then it will skip the Twig rendering and return the view components instead.

3: Add an abstract doRender() method to both of the above classes that is called a bit like $this->doRender($template, $context, $globals) just before the current call: $response->setContent($template->render($context)); The method can be empty by default, anyone extending the Bolt controllers can implement the method and intercept just before the render happens.

4: Support a <action>Context() method for each action in all controllers. The render method in both the base classes, Base::render and BackendBase::render can check to see if the method exists before calling if it does, eg: $context = $this->homepageContext($context) will allow the context to be modified.

[GwendolenLynch]

1: Manage this purely with an event that gets called in Base::render and BackendBase::render

Personally this is my vote I know @CarsonF want to comment further as well on this, but th two of us have agreed to take the weekend off the project … no prize for guessing why 😉

[CarsonF]

I suggest we just take advantage of the view event.

Something like:

// Open to suggestions for class name.
class TemplateView // doesn't extend Symfony HTTP Response
{
    // public for brevity
    public $template = '';
    public $context = [];
    public $globals = []; // if we have to
    public $headers = [];
}

is returned from controller methods. Then in the view event we convert it to a template response (which is where the twig render happens).
This allows the methods to be overridden and modify the data before it's given to twig. Yet there still is a defined render point.

The only downsides I see to this are:

1. Return value from Controllers is different. This should be a problem as they shouldn't be invoked directly.
2. Without a Response object, we can't specify any cache headers in the controller method. They can still be added later, which is actually what's already happening in Frontend::before.

I do think the controllers should be thinner, but I don't think breaking up the methods into helper methods is the best path. I would rather define them as services if we see fit.
Another way to make them thinner is using route converters. So instead of the controller method getting a contenttypeslug string, it gets a ContentType object. This saves a step if you need to override the method completely.

Both of these, objects returned with view event to convert to response, and converters, are idiomatic with Silex.

---

I can PR if we agree upon this direction.

[GwendolenLynch]

• Silex view event 👍
• public $globals = []; 👎

Without a Response object, we can't specify any cache headers in the controller method. They can still be added later, which is actually what's already happening in Frontend::before.

This concerns me a little still, as I thought the intent was to kill that out of the ->before() … but does not concern me enough to object.

Both of these, objects returned with view event to convert to response, and converters, are idiomatic with Silex.

I'll send emergency 🍻 supply right now … get coding! 😉

[bobdenotter]

Closed by #6443.