For my Symfony2 project, I'd like to stick with the Fat Model Skinny Controller approach. I've got things working, however, there's a lot of duplication / unnecessary business logic in my controller. What's the best approach for cleaning things up?
The Controller
<?php
namespace TestApp\PeopleBundle\Controller;
use Symfony\Bundle\FrameworkBundle\Controller\Controller;
class PeopleController extends Controller {
public function indexAction() {
return $this->render('TestAppPeopleBundle:People:index.html.twig');
}
public function loadAction() {
$repository = $this->getDoctrine()->getRepository('TestAppPeopleBundle:People');
$request = $this->getRequest();
$parsedFilters = array();
$extFilterParser = $this->get('ext_filter_parser');
$filters = $request->query->get('filter');
if(empty($filters) === FALSE) {
$parsedFilters = $extFilterParser->setFilters($filters)->parse()->getParsedFilters();
}
$format = $request->getRequestFormat();
$people = $repository->getListOfPeople($parsedFilters);
$data = array('success' => true, 'people' => $people);
return $this->render('::base.'.$format.'.twig', array('data' => $data));
}
public function exportGridAction($format) {
$repository = $this->getDoctrine()->getRepository('TestAppPeopleBundle:People');
$request = $this->getRequest();
$parsedFilters = array();
$extFilterParser = $this->get('ext_filter_parser');
$filters = $request->query->get('filter');
if(empty($filters) === FALSE) {
$parsedFilters = $extFilterParser->setFilters($filters)->parse()->getParsedFilters();
}
$format = $request->getRequestFormat();
$people = $repository->getListOfPeople($parsedFilters);
$grid = $request->get('grid');
return $this->render('::extgrid.'.$format.'.twig', array('grid' => $grid, 'data' => $people));
}
}
The Entity Repository (People)
<?php
namespace TestApp\PeopleBundle\Repository;
use Doctrine\ORM\EntityRepository;
/**
* PeopleRepository
*
* This class was generated by the Doctrine ORM. Add your own custom
* repository methods below.
*/
class PeopleRepository extends EntityRepository {
public function getQbForListOfPeople(array $filters) {
$queryBuilder = $this->createQueryBuilder('p');
$queryBuilder->select('p.id', 'p.firstName', 'p.lastName', 'p.dateOfBirth', 'p.address', 'p.city', 'p.state', 'p.zipCode');
foreach($filters as $filter) {
$queryBuilder->andWhere('p.' . $filter['expression'] . ' ' . $filter['value']);
}
$queryBuilder->setMaxResults(50);
return $queryBuilder;
}
public function getListOfPeople() {
return $this->getQbForListOfPeople(array())->getQuery()->getResult();
}
}
My thought was to put the logic for parsing filters out of the $_GET or $_POST inside the repository, but the repository doesn't have access to the Request Object like the controller does.
My second thought was to put the logic for parsing filters out of the $_GET or $_POST inside the ExtFilterParser Service, but the service doesn't have access to the Request Object like the controller does.
I would do your second thought and inject @request (see my answer with code on this question - How to inject the @request into a service? ). This approach let you move away this code from controller into service:
$parsedFilters = array();
$extFilterParser = $this->get('ext_filter_parser');
$filters = $request->query->get('filter');
if(empty($filters) === FALSE) {
$parsedFilters = $extFilterParser->setFilters($filters)->parse()->getParsedFilters();
}
Second, $request will be populated with Request object without having to specify $request = $this->getRequest(); if you use this:
use Symfony\Component\HttpFoundation\Request;
public function loadAction(Request $request) { ... }
I don't see too much business logic in your actions. Generally your controllers/actions role is to be a gateway for your domain model and services, and handle protocol specific stuff. CLI commands deals with command line arguments, controllers with HTTP requests, so your action methods do what they supposed to.
You have a lot of options to refactor your code to eliminate these kind of duplicate codes in your controller actions using some refactoring patterns. Extract method to move these lines into a protected method of your controller. You can extract a base class for these similar controllers and move the methods there.
In more complicated scenarios you can move controller logic (extracting domain specific information from the HTTP request) into services, especially if you need some other DIC managed services to complete this task. There is nothing wrong accessing the request object in these services. You can make this kind of services request scoped, or implement ContainerAware and get the request from the container.
<service id="controller-helper-service" class="%helper.class%" scope="request">
<argument type="service" id="request" />
<argument type="service" id="some-other-service" />
</service>
Consider eliminating all the doctrine code (repository stuff) from your controller and just have a PeopleManager service into which you inject the entity manager. So you would have:
$peopleManager = $this->get('people.manager');
$people = $peopleManager->getListofPeople($parsedFilters);
That saves some code and it eliminates the need for your controller to know anything about the doctrine module.
Inject the request directly into your controller methods with
public function loadAction(Request $request) {
That knocks out the getRequest and eliminates possible issues with embedded controllers.
No so sure about injecting Request into your parser. But I would eliminate some of the error checking and just have:
$extFilterParser = $this->get('ext_filter_parser');
$filters = $request->query->get('filter');
// Return empty array if no filters
$parsedFilters = $extFilterParser->setFilters($filters)->parse()->getParsedFilters();