What is the proper way to abstract common functionality in Symfony 2 controllers

Question

We have a fairly large symfony2 code base. Generally our Controller actions would look something like

public function landingPageAction(Request $request) {
 //do stuff      
 return $this->render("view_to_render", $template_data);
}

We have two functionalities that are very generic between all of our controllers:

1. We tend to pass Controller level template parameters to all of the actions in a specific controller - let's call these "Default Parameters"
2. We set HTTP cache headers at the end of each Action

Understandably we want to abstract this logic away. In doing so we came up with two approaches. We are not certain which approach is better, both in terms of general OO and SOLID principles, but also in terms of performance and how SF2 recommends things be done.

Both approaches rely on having the controller extend an interface that indicates if the controller has "Default Parameters" (later we are considering also adding Cacheable interface)

use Symfony\Component\HttpFoundation\Request;
interface InjectDefaultTemplateVariablesController {
public function getDefaultTemplateVariables(Request $request);
}

Approach 1

This approach is based on events. We define an object that will store our template variables, as well as (in the future) cache indicators

class TemplateVariables {

protected $template_name;

protected $template_data;

public function __construct($template_name, $template_data) {
    $this->template_name = $template_name;
    $this->template_data = $template_data;
}

/**
 * @param mixed $template_data
 * @return $this
 */
public function setTemplateData($template_data) {
    $this->template_data = $template_data;

    return $this;
}

/**
 * @return mixed
 */
public function getTemplateData() {
    return $this->template_data;
}

/**
 * @param mixed $template_name
 * @return $this
 */
public function setTemplateName($template_name) {
    $this->template_name = $template_name;

    return $this;
}

/**
 * @return mixed
 */
public function getTemplateName() {
    return $this->template_name;
}

}

We also define events that will be triggered on render and which call the views

class InjectDefaultTemplateVariablesControllerEventListener {

/** @var DelegatingEngine */
private $templating;

private $default_template_variables;

public function __construct($templating) {
    $this->templating = $templating;
}

public function onKernelController(FilterControllerEvent $event) {
    $controller = $event->getController();

    if (!is_array($controller)) {
        return;
    }

    if ($controller[0] instanceof InjectDefaultTemplateVariablesController) {
        $this->default_template_variables = $controller[0]->getDefaultTemplateVariables($event->getRequest());
    }
}

public function onKernelView(GetResponseForControllerResultEvent $event) {
    $controller_data = $event->getControllerResult();

    if ($controller_data instanceof TemplateVariables) {
        $template_data = (array)$controller_data->getTemplateData();

        $template_data = array_merge($this->default_template_variables, $template_data);

        $event->setResponse($this->templating->renderResponse($controller_data->getTemplateName(), $template_data));
    }

}

} 

Finally our Action now becomes

public function landingPageAction(Request $request) {
 //do stuff      
 return new TemplateVariables("view_to_render", $template_data);
}

Approach 2

This approach is based on putting the common logic into a BaseController from which every other controller inherits. We are still keeping the approach of having Child controllers also extend an interface in case they want to use "Default Parameters".

The following is the new method in the base controller that determines if Default Parameters need to be merged with the specific template parameters. Later this method will also handle cache headers using ttl parameter.

public function renderWithDefaultsAndCache($view, array $parameters = array(), Response $response = null, $ttl = null)
{
  $default_template_variables = array(); 
  if ($this instanceof InjectDefaultTemplateVariablesController ) {
    $default_template_variables = $this->getDefaultTemplateVariables();
  }
  $template_data = array_merge($default_template_variables, $parameters);
  return $this->render($view, $template_data, $response);
 }

Action now becomes

public function landingPageAction(Request $request) {
 //do stuff      
 return $this->renderWithDefaultsAndCache("view_to_render", $template_data);
}

Discussion

So far the main arguments for the first approach were that it follows SOLID principles and is easier to extend - iin case more common logic were to be added, it can be put directly into Event Listeners without affecting the controllers.

The main arguments for the second approach were that the logic we are trying to abstract away actually does belong to the controller and not an external event. In addition there was a concern that using events in this manner will result in a poor performance.

We would be really grateful to hear from the experts on which approach is better or possibly suggest a third one that we have missed.

Thank you!

Answer 1

First off I am in no way claiming to be a Symfony 2 architecture expert.

I have a game schedule program which outputs a number of different types of schedules (public, team, referee etc). The various schedules are all similar in that they deal with a set of games but vary in details. The schedules need to be displayed in various formats (html,pdf,xls etc). I also wanted to be able to further tweak things for individual tournaments.

I originally used your second approach by creating a ScheduleBaseController and then deriving various individual schedule controllers from it. It did not work well. I tried to abstract common functionality but the schedules were just different enough that common functionality became complicated and difficult to update.

So I went with an event driven approach very similar to yours. And to answer one of your questions, adding some event listeners will not have any noticeable impact on performance.

Instead of focusing on template data I created what I call an Action Model. Action models are responsible for loading the games based on request parameters and (in some cases) updating the games themselves based on posted data.

Action models are created in the Controller event listener, stored in the request object and then passed to the controller's action method as an argument.

// KernelEvents::CONTROLLER listener
$modelFactoryServiceId = $request->attributes->get('_model');    
$modelFactory = $this->container->get($modelFactoryServiceId);
$model = $modelFactory->create($request);
$request->attributes->set('model',$model);

// Controller action
public function action($request,$model)
{
    // do stuff

    // No template processing at all, just return null
    return null;
}
// KernelEvents::VIEW listener
$model = $request->attributes->get('model')
$response = $view->renderResponse($model);

So the controller is mostly responsible for form stuff. It can get data from the model if need be but let's the model handle most of the data related stuff. The controller does no template processing stuff at all. It just returns null which in turn kicks off a VIEW event for rendering.

Lot's of objects? You bet. The key is wiring this up in the route definition:

// Referee Schedule Route
cerad_game__project__schedule_referee__show:
path:  /project/{_project}/schedule-referee.{_format}
defaults: 
    _controller: cerad_game__project__schedule_referee__show_controller:action
    _model:      cerad_game__project__schedule_referee__show_model_factory
    _form:       cerad_game__project__schedule_referee__show_form_factory
    _template: '@CeradGame\Project\Schedule\Referee\Show\ScheduleRefereeShowTwigPage.html.twig'
    _format:     html
    _views:
        csv:   cerad_game__project__schedule_referee__show_view_csv
        xls:   cerad_game__project__schedule_referee__show_view_xls
        html:  cerad_game__project__schedule_referee__show_view_html
requirements:
    _format:  html|csv|xls|pdf

Each part is broken up into individual services which, for me at least, makes it easier to customize individual sections and to see what is going on. Is it a good approach? I don't really know but it works well for me.