Feedback on my Model Layer Approach Symfony 2 Model Layer + Doctrine

Question

Please provide feedback on the following approach of creating a Model Layer which consists of business rules which utliises Doctrine for data access.

My current approach is based around the notion that the Model is a ContainerAware class / object where all the non-library, business specific domain logic is in.

I find that I have to hammer Frameworks to do things this way, which is why part of my brain questions my approach.

I'm currently using Symfony 2 which, like all modern PHP MVC frameworks, utilise an ORM layer like Doctrine 2 and inevitably regards it as the Model Layer. I'm guessing the situation will be similar with ZF2, so although my example is written in SF2, regard this as a framework agnostic question.

Concrete Example

As a concrete example, consider the following scenario:

A Message Requirement

• As a user, I can create a Message that belongs to me.
• As a user, I can update a Message that belongs to me.
• As a user, I can archive a Message that belongs to me.

The Controller

In Symfony2, these requirements are coded up as Actions in the controller layer. I'll skip the extraneous code which checks if a message actually belongs to a user, but obviously, this should be part of the domain logic also. In a method "belongsToUser" or similar.

 // Vendor\MessageBundle\DefaultController 
 public function archiveAction(Request $request) {
    // ... 
    $em = $this->getDoctrine()
               ->getManager();
    $message = $em->getRepository('MessageBundle:Message');
                    ->getManager()
                    ->getRepository('MessageBundle:Message')
                    ->find($request->get('id'));
    $message->setIsArchived(true);
    $em->persist($entity);
    $em->flush();
    $this->flashMessage('Message has been archived.');
    // ...
}

The Model

If I was to put this into a model, it would look like the following:

class MessageModel 
{
     public function archive($messageId) {
     // ... 
        $em = $this->getDoctrine()
                   ->getManager();
        $message = $em->getRepository('MessageBundle:Message')
                    ->find($messageId);
        $message->setIsArchived(true);
        $em->persist($entity);
        $em->flush();
     // ... return true on success, false on fail.
     }
}

Revised Controller

My Revised Controller will now look like this:

 // Vendor\MessageBundle\DefaultController 
 public function archiveAction(Request $request) {
    // ... 
    $model = new MessageModel(); // or a factory.
    $result = $model->archive($request->get('id'));
    if($result) {
        $this->flashMessage('Message has been archived.');
    } else { 
        $this->flashMessage('Message could not be archived due to a system error.');
    }
    return array('result'=>$result);
    // ...
}

The other two requirements will also be implemented on the model.

My Approach in a nutshell

In a nutshell, this is my current approach:

• Controller - less logic
• View - stays the same
• Model - container aware and houses all business logic, accesses Doctrine as data access.
• ORM - Regarded as part of the Model Layer, but not regarded as THE model layer.
• Service Layer - when required, I can use the Service Layer to work with multiple layers, but I've found that I've only had to use it in a few occasions due to the simple nature of the applications I've had to build.

My Question(s)

• Is my approach in line with what others are doing?
• Am I missing something glaringly obvious?
• Have you tried anything similar to this and found it good / bad?

Thank you in advance.

Answer 1

At first you can use a ParamConverter to simplify your controller. An exception is automatically thrown if the entity is not found.

I would call the message methods archive and restore but that's a matter of preference.

A nice way to integrate security checks is provided by JMSSecurityExtraBundle. Use ACLs to check wether your current user is allowed to archive a message.

Your MessageModel is similar to the Manager (interface/abstract and i.e. doctrine ODM/ORM implementation) pattern you can find in the FOSUserBundle.

These managers are building the bridge between storage layer and your controller and all share the same Interface (i.e. UserManagerInterface). This way you can exchange i.e. doctrine with propel easily.

Improve by turning your controller into a service and injecting your Manager service instead of injecting the whole container (i.e. by extending Symfony\Bundle\FrameworkBundle\Controller\Controller) and getting it from there.

You should inject only the dependencies you need into your services to ease testing. In the example of doctrine orm/odm a manager service would retrieve the classname parameter, the entity-/documentmanager service, and the repository service. (service definition)

You can find additional inspiration for creating a controller-utilities service in Benjamin Eberlei's blog post "Extending Symfony2: Controller Utilities".

Another often used technique is to create an abstract parent service for your most commonly used controller dependencies. (example)

The last quick tip i have is moving the creation of flashmessages to event listeners/subscribers. Just check wether the method returns an object of type Message or better MessageInterface and add the success-flashmessage. If the entity is not found you can catch the exception and add the flashmessage with the error message.

You could even ommit the return at the end of the method when using a view response listener like FOSRestBundle's that automatically assumes you're returning the original argument if the method returns nothing - read about it here.

Finally you can end up with a method in a nicely testable controller service that looks like this:

/** 
 * @PreAuthorize("hasPermission(#message, 'ARCHIVE')")
 */
public function archiveAction(Message $message) 
{
    $message->archive();
    $this->messageManager->update($message, true);
}

The manager->update() method behaves like the one found in FOS\UserBundle\Doctrine\UserManager in my example.