Deconstructing Symfony: Part II – Clean up your project

I have some controversial practices with Symfony projects. One of them is to remove the bundles that are really for newcomers and that actually I would have wished never deal with. But all has pros and cons. The bad practices that were introduced in some standard distributions are meant to make accessible it to more, however, in the long run these practices affect greatly the path of learning. But once you are over them you can look back and understand it like in the explanation that follows.

So many people would like to generate code, and I understand the point, however code generation is never perfect and causes in the long run a world of pain because is not code you wrote and is not code you can call maintainable code. So if you hear me saying “remove all your generator bundles” please do!

Let’s start with a sample of the dreadful “`php app/console controller-generate or bundle-generate here“`, even doctrine have some similar commands, avoid them please. Ok now close your pristine clean code eyes because what you are about to see has no forgiveness:

<?php
 
namespace Vendor\TrashBundle\Controller;
 
use Symfony\Bundle\FrameworkBundle\Controller\Controller;
use Vendor\TrashBundle\Entity\Category;
use Vendor\TrashBundle\Form\CategoryType;
 
class CategoryController extends Controller
{
    public function indexAction()
    {
        $em = $this->getDoctrine()->getManager();
 
        $entities = $em->getRepository('VendorTrashBundle:Category')->findAll();
 
        return $this->render('VendorTrashBundle:Category:index.html.twig', array(
            'entities' => $entities,
        ));
    }
 
    public function showAction($id)
    {
        $em = $this->getDoctrine()->getManager();
 
        $entity = $em->getRepository('VendorTrashBundle:Category')->find($id);
 
        if (!$entity) {
            throw $this->createNotFoundException('Unable to find Category entity.');
        }
 
        $deleteForm = $this->createDeleteForm($id);
 
        return $this->render('VendorTrashBundle:Category:show.html.twig', array(
            'entity'      => $entity,
            'delete_form' => $deleteForm->createView(),
        ));
    }
 
    public function newAction()
    {
        $entity = new Category();
        $form   = $this->createForm(new CategoryType(), $entity);
 
        return $this->render('VendorTrashBundle:Category:new.html.twig', array(
            'entity' => $entity,
            'form'   => $form->createView(),
        ));
    }
 
    public function createAction()
    {
        $entity  = new Category();
        $request = $this->getRequest();
        $form    = $this->createForm(new CategoryType(), $entity);
        $form->bind($request);
 
        if ($form->isValid()) {
            $em = $this->getDoctrine()->getManager();
            $em->persist($entity);
            $em->flush();
 
            return $this->redirect($this->generateUrl('category_show', array('id' => $entity->getId())));
        }
 
        return $this->render('VendorTrashBundle:Category:new.html.twig', array(
            'entity' => $entity,
            'form'   => $form->createView(),
        ));
    }
 
    public function editAction($id)
    {
        $em = $this->getDoctrine()->getManager();
 
        $entity = $em->getRepository('VendorTrashBundle:Category')->find($id);
 
        if (!$entity) {
            throw $this->createNotFoundException('Unable to find Category entity.');
        }
 
        $editForm = $this->createForm(new CategoryType(), $entity);
        $deleteForm = $this->createDeleteForm($id);
 
        return $this->render('VendorTrashBundle:Category:edit.html.twig', array(
            'entity'      => $entity,
            'edit_form'   => $editForm->createView(),
            'delete_form' => $deleteForm->createView(),
        ));
    }
 
    public function updateAction($id)
    {
        $em = $this->getDoctrine()->getManager();
 
        $entity = $em->getRepository('VendorTrashBundle:Category')->find($id);
 
        if (!$entity) {
            throw $this->createNotFoundException('Unable to find Category entity.');
        }
 
        $editForm   = $this->createForm(new CategoryType(), $entity);
        $deleteForm = $this->createDeleteForm($id);
 
        $request = $this->getRequest();
 
        $editForm->bind($request);
 
        if ($editForm->isValid()) {
            $em->persist($entity);
            $em->flush();
 
            return $this->redirect($this->generateUrl('category_edit', array('id' => $id)));
        }
 
        return $this->render('VendorTrashBundle:Category:edit.html.twig', array(
            'entity'      => $entity,
            'edit_form'   => $editForm->createView(),
            'delete_form' => $deleteForm->createView(),
        ));
    }
 
    public function deleteAction($id)
    {
        $form = $this->createDeleteForm($id);
        $request = $this->getRequest();
 
        $form->bind($request);
 
        if ($form->isValid()) {
            $em = $this->getDoctrine()->getManager();
            $entity = $em->getRepository('VendorTrashBundle:Category')->find($id);
 
            if (!$entity) {
                throw $this->createNotFoundException('Unable to find Category entity.');
            }
 
            $em->remove($entity);
            $em->flush();
        }
 
        return $this->redirect($this->generateUrl('settings_category'));
    }
 
    private function createDeleteForm($id)
    {
        return $this->createFormBuilder(array('id' => $id))
            ->add('id', 'hidden')
            ->getForm()
        ;
    }
}

Now you can open your clean code eyes. Ok let’s just first describe the monster. Besides using old php5.3 ugly array syntax, picture if you generated CRUDs for just 4 entities of your bundle. It will be 4 times the code your poor eyes just saw, and code that does the same thing, in the same way, it comes with typos and it comes with errors in formatting as well (yes you have to fix them if you want to maintain them). Some persons in this world, that do this generating of these CRUDs for a bundle, call themselves developers. Of course we all are bad and good ones but just saying this so we take a bit of shame and react.

Let’s go from 153 lines of madness to this:

<?php
 
namespace Vendor\ImprovedBundle\Controller;
 
class CategoryController extends BaseController
{
    protected $class = 'Vendor\ImprovedBundle\Entity\Category';
    protected $handle = 'VendorImprovedBundle:Category';
    protected $basePath = 'category';
    protected $type = 'Vendor\ImprovedBundle\Form\CategoryType';
}

This is after we have refactored some repetitive code and moved it into a base controller, made each method independent of the class the CRUD is handling, taking away the names for paths and form types and classes to make them class variables, and after we have used some helper methods from the repository.

Let’s see the base controller:

<?php
 
namespace Vendor\ImprovedBundle\Controller;
 
use Symfony\Bundle\FrameworkBundle\Controller\Controller;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
 
class BaseController extends Controller
{
    protected $handle = 'handle_here';
    protected $class = 'class_here';
    protected $basePath = 'base_path_here';
    protected $type = 'type_here';
 
    public function indexAction()
    {
        return $this->render('index', ['entities' => $this->findAll()]);
    }
 
    public function showAction($id)
    {
        return $this->render(
            'show',
            [
                'entity' => $this->find($id),
                'delete_form' => $this->createDeleteForm($id)->createView(),
            ]
        );
    }
 
    public function newAction()
    {
        $entity = $this->createEntity();
 
        return $this->render(
            'new',
            [
                'entity' => $entity,
                'form' => $this->getForm($entity)->createView(),
            ]
        );
    }
 
    public function createAction(Request $request)
    {
        $form = $this->getForm($entity = $this->createEntity());
 
        if ($form->handleRequest($request)->isValid()) {
            $this->save($entity);
 
            return $this->redirectTo($this->basePath.'_show', ['id' => $entity->getId()]);
        }
 
        return $this->render(
            'new',
            [
                'entity' => $entity,
                'form' => $form->createView(),
            ]
        );
    }
 
    public function editAction($id)
    {
        $editForm = $this->getForm($entity = $this->find($id));
        $deleteForm = $this->createDeleteForm($id);
 
        return $this->render(
            'edit',
            [
                'entity' => $entity,
                'edit_form' => $editForm->createView(),
                'delete_form' => $deleteForm->createView(),
            ]
        );
    }
 
    public function updateAction(Request $request, $id)
    {
        $editForm = $this->getForm($entity = $this->find($id));
        $deleteForm = $this->createDeleteForm($id);
 
        if ($editForm->handleRequest($request)->isValid()) {
            $this->save($entity);
 
            return $this->redirectTo($this->basePath.'_edit', ['id' => $id]);
        }
 
        return $this->render(
            'edit',
            [
                'entity' => $entity,
                'edit_form' => $editForm->createView(),
                'delete_form' => $deleteForm->createView(),
            ]
        );
    }
 
    public function deleteAction(Request $request, $id)
    {
        $form = $this->createDeleteForm($id);
 
        if ($form->handleRequest($request)->isValid()) {
            $this->remove($this->find($id));
        }
 
        return $this->redirectTo($this->basePath);
    }
 
    public function render($view, array $parameters = [], Response $response = null)
    {
        return parent::render($this->handle.':'.$view.'.html.twig', $parameters);
    }
 
    public function redirectTo($path, array $params = [])
    {
        return $this->redirect($this->generateUrl($path, $params));
    }
 
    public function getForm($data = null, array $options = [])
    {
        return parent::createForm(new $this->type, $data, $options);
    }
 
    protected function createDeleteForm($id)
    {
        return $this
            ->createFormBuilder(['id' => $id])
            ->add('id', 'hidden')
            ->getForm()
        ;
    }
 
    protected function find($id)
    {
        return $this->getRepo()->find($id);
    }
 
    protected function findAll()
    {
        return $this->getRepo()->findAll();
    }
 
    protected function save($object)
    {
        $this->getRepo()->save($object);
    }
 
    protected function remove($object)
    {
        $this->getRepo()->remove($object);
    }
 
    protected function getRepo()
    {
        return $this->get('doctrine')->getRepository($this->handle);
    }
 
    protected function createEntity()
    {
        return new $this->class;
    }
}

After doing this refactoring our controller methods and actions are very readable and maintainable should there be any change or new requirement. Then we can override actions if need be and play with customizations further. So far it is enough and our tests still pass, of course if we had them, did they also get generated? of course not! Notice we have not even mentioned some other bundles because we don’t need them or want them for a legacy system and we want to have first sight of what we are working with and learn or relearn better practices. The truth is all code written one hour ago is already legacy, so it can be improved. When improving code you want to stay close to the domain, else a world of hurt awaits by entering into your project other people’s domains and codes.

Let’s take a look a the new entity repository:

<?php
 
namespace Vendor\ImprovedBundle\Entity;
 
use Doctrine\ORM\EntityRepository;
 
class CategoryRepository extends EntityRepository
{
    public function save($object, $flush = true)
    {
        $this->getEntityManager()->persist($object);
        if ($flush) {
            $this->getEntityManager()->flush();
        }
    }
 
    public function remove($object, $flush = true)
    {
        $this->getEntityManager()->remove($object);
        if ($flush) {
            $this->getEntityManager()->flush();
        }
    }
}

Many people are all over the map as to where to do the flushes so you can pass the handle to do it in or outside. In CRUDs in particular this should be no problems since you are not manipulating but one entity and its related ones anyway, but just in case you want to reuse the methods we provide those accessors options.

Go now and start removing many lines of code (LOC) at work, refactor legacy and share your findings. My advise at the beginning of a project from scratch, do what I did in the first blog post of this series, and remove the bundles and plug the commands by hand. That way nobody on your team will have even an option to think on generating code.

If you like this and want to support me raise funds for presenting at DrupalCon and to print more Gush stickers please paypal me at cordoval@gmail.com.

Thanks for your support!

your friend, @cordoval

4 thoughts on “Deconstructing Symfony: Part II – Clean up your project

  1. I’ve mentioned it before, but i think attempting to “optimize” the persist/flush stuff is a mistake. They are meant to be separate functions

    • it is not optimization, you can still use it, this is in particular for CRUD and a use case and also more DDD oriented, but it is the first step not the final.

  2. Instead of rely on properties in your BaseController, it would be better to rely on an Interface or abstract functions, like: getHandler, getClass, getBasePath, getType and so on.

    great series, keep coming 😉

    • yes that is the series coming and that added to doctrine custom repository to empower it for more DDD.

      responding a tweet from sylius resource bundle, it is not a fit for the reasons we don’t want to make things complex or add bundles, it is just plain wrong. Not saying is a bad bundle, i even considered, but why you want to add a many line bundle instead of solving your problem in a simpler way? with your code and that everybody understands and make it so there is no need to learn a bundle? Resource bundle is good for other use cases. Keep it simple.

Leave a Reply to Aaron Cancel reply

Your email address will not be published. Required fields are marked *