When PHP Framework Sucks Series: Business logic free Controllers

Business logic in controller

Let’s start with Controllers and your business logic. Rule number one: Do not put business logic in controllers at any cost! Business logic in controller is particularly problematic because controllers usually derive from the framework. Also, when they extend the framework base controller class, they usually know too much (containers). As the name implies, business logic is your code which you want to reuse and make independent of libraries and frameworks. It should be well separated and encapsulated since you might probably call the same method from different places.

Here is the example of Symfony controller best practice page:

namespace App\Controller;

use App\Entity\Post;
use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
use Symfony\Component\Routing\Annotation\Route;

class DefaultController extends AbstractController
{
    /**
     * @Route("/", name="homepage")
     */
    public function index()
    {
        $posts = $this->getDoctrine()
            ->getRepository(Post::class)
            ->findLatest();

        return $this->render('default/index.html.twig', [
            'posts' => $posts,
        ]);
    }
}

Here, business logic sits in an inappropriate place. Instead, we should have a class which returns us the latest posts. We can call that class for the sake of simplicity PostsFromDatabase.

namespace ExampleNamespace;

class PostsFromDatabase
{
    /** @var ManagerRegistry**/
    private $doctrine;

    public function getDoctrine()
    {
        return $this->doctrine;
    }

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

    public function getLatest()
    {
        $posts = $this->getDoctrine()
            ->getRepository(Post::class)
            ->findLatest();

        return $posts;

    }

}

Test for this class, in this case, is straightforward

namespace ExampleNamespace;

use Doctrine\Common\Persistence\ManagerRegistry;
use Doctrine\ORM\EntityRepository;

class PostsFromDatabaseTest extends \PHPUnit_Framework_TestCase
{
    /**
     * @covers \ExampleNamespace\PostsFromDatabase
     */
    public function testGetLatestShouldReturnSameCollectionAsFindLatestReturns()
    {
        $managerRegistryMock = $this->createMock(ManagerRegistry::class);
        $repository = $this->createMock(EntityManagerInterface::class);

        $managerRegistryMock
            ->expects($this->once())
            ->method('getRepository')
            ->with(Post::class)
            ->willReturn($repository);
        $repository
            ->expects($this->once())
            ->method('findLatest')
            ->willReturn(['post1', 'post2']);

        $postsFromDatabase = new PostsFromDatabase($managerRegistryMock);
        $latestPosts = $postsFromDatabase->getLatest();

        $this->assertEquals(['post1', 'post2'], $latestPosts);
    }
}

Now our controller looks like this

namespace App\Controller;

use ExampleNamespace\PostsFromDatabase
use App\Entity\Post;
use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
use Symfony\Component\Routing\Annotation\Route;

class DefaultController extends AbstractController
{
    /** @var PostsFromDatabase **/
    private $postsFromDatabase;

    public function getPostsFromDatabase(): PostsFromDatabase
    {
        if (null === $this->postsFromDatabase) {
            $this->postsFromDatabase = new PostsFromDatabase($this->getDoctrine());
        }

        return $this->postsFromDatabase;
    }

    public function setPostsFromDatabase(PostsFromDatabase $postsFromDatabase)
    {
        $this->postsFromDatabase = $postsFromDatabase;
    }

    /**
     * @Route("/", name="homepage")
     */
    public function index()
    {
        $posts = $this->getPostsFromDatabase()->getLatest();

        return $this->render('default/index.html.twig', [
            'posts' => $posts,
        ]);
    }
}

The controller is free of business logic and now only deal with routing, rendering and know which service to call. But there is one more benefit you might not spot so far:

Half of our code, now have a unit tests 🤩🤩🤩🤩🤩🤩

And you should not stop there, now controller is super easy to test (that's why I left 'set' and 'get' methods for PostsFromDatabase), but that I will cover in the next chapter of this series.

Stay here for a second. Did you ever saw that framework tutorials in their examples, implies you should test your code? Most modern frameworks have a "placeholder" for tests. But I don't remember that I saw one framework which has implemented unit test measure, tests required, or tests in their tutorial examples. I'll leave this question here for some future blog posts.

Comments

[Grzegorz Ziemonski]

I agree that most business logic should not happen inside the controllers but I'd say that calling a database query like this is hardly "business" logic. I think I'd reserve applying the principle for real business invariants and algorithms, and avoid artificial creations like PostsFromDatabase here.

To add a little "it depends" factor, there is a good case for moving out simple orchestration logic like this out of the controllers. That's when (and only when) this logic needs to be reused in another place e.g. we do the same thing both in response to an HTTP request and when we receive an event via a message queue.

[Damnjan Jovanovic]

Hi Grzegorz, thanks for your comment.
I see that many devs do not see a problem in this particular example, at least nobody complains about that on Symfony website :)
I still believe that which posts coming from a database is business logic.

Let me elaborate on this. If you have method getPostsFromDatabase than you can explicitly specify in this method exact criteria which posts you want to show — for example only active, only newer than one week, etc. Query criteria is than, an example of business logic.
On the other hand, as you already mentioned, if we have to execute a database call from two different places, it has to go to a separate class.
I believe that keeping database calls in controllers are evil, no matter do we using it in some other places. What do you think about that?

[Jonathan SIMONNEY]

just stepping in for a little typo in the controller example : you wrote

public fucntion getPostsFromDatabase(): PostsFromDatabase

instead of

public function getPostsFromDatabase(): PostsFromDatabase

(and there is the same mistake for the setPostsFromDatabase line.

Thank you very much for the posts, they are extremely interesting.

[Damnjan Jovanovic]

Hei Jonathan, thank you very much for pointing out this typo :D I replace fucntion with function.
Unfortunately, this is typo happen to me so often :( I have to find a way to stop making it. My IDE (PhpStorm) helps me a lot, but when I type without IDE, it is hard to spot misspelling.

Thank you very much for the positive feedback!

Cheers!

[Al Chen]

To Grzegorz's point, people's interpretation of "business logic" could affect whether mission critical logic is stored in the controller vs. a class. When you're trying to build something quick and dirty, it was just easier to stuff all the logic into the controller. At some point when your controllers get out of hand, a re-factor becomes inevitable.

[Damnjan Jovanovic]

Hi Joey, thank you very much for putting the effort in so descriptive comment!
First I have to say that I read your comment a couple of times and I agree with some points 100%.

• The first point which you mention is that I make mock way to strict, that's what I going to change right now. Thanks for this comment. There is also another colleague of mine who recently pointed to me a couple of places when I did the same lock-to-implementation. So it is definitely something I have to improve :)
• I also agree that mock code took a great number of line in the test, but I have an excuse for that since I make it for readability, so the one who read this article get it immediately.

When it comes to the improvement proposition you mentioned I personally strongly disagree with point one (Consider simply not having that test. Is it really useful?). I hear that opinion along with point two (Make it another type of test, such as an integration test (you can use sqlite in memory, etc).) from time to time, but I think it is not the way we should go. I can imagine that you also think that chasing 100% unit test coverage is the right goal, and that's the point where I usually disagree with my colleagues who have a similar approach like yours. In my opinion, the unit test should have 100% coverage, and one test class per one production class, integration, and functional tests should also take place in the repo but not as a replacement for unit tests.

Just to be clear, I don't think my opinion and approach is necessary right in the point where we disagree, I just found it useful and beneficial from my experience.

I'm also curious, what is an argument in your opinion, for not having a unit test for some class?
Cheers!