2
\$\begingroup\$

This is probably going to be my final question regarding controllers. I feel like I have a good enough understanding of them now and am able to write them cleanly.

I've recently adapted a less error prone way of communicating with the corresponding View. But I would still like to know if there is anything I could further improve about this LoginController class?

namespace Controller;

use View\LoginView;
use Http\HttpRequest;
use Controller\Controller;
use Model\Application\AntiCsrfService;
use Model\Application\AuthenticationService;
use Model\Application\FormValidationService;

class LoginController extends Controller
{
    private $antiCsrfService;
    private $formValidationService;

    public function __construct(
        HttpRequest           $httpRequest,
        AuthenticationService $authenticationService,
        LoginView             $loginView,
        AntiCsrfService       $antiCsrfService,
        FormValidationService $formValidationService
    ) {
        parent::__construct($httpRequest, $authenticationService, $loginView);

        $this->antiCsrfService       = $antiCsrfService;
        $this->formValidationService = $formValidationService;

        if ($this->authenticationService->findLoggedInUser()) {
            $this->view->setIsUserLoggedIn(true);

            $this->view->index();
        }
    }

    public function index()
    {
        $this->view->setFormValueOf('csrfToken', $this->antiCsrfService->getToken());

        $this->view->index();
    }

    public function submit()
    {
        $token = $this->httpRequest->findParameter($this->antiCsrfService->getToken()['name']);

        if ($this->antiCsrfService->isTokenValid($token)) {
            $isLoginFormValid = $this->formValidationService->isLoginValid(
                trim($this->httpRequest->findParameter('email')),
                     $this->httpRequest->findParameter('password')
            );

            // Only attempt to log the User in when the form has no validation errors.
            if ($isLoginFormValid) {
                $user = $this->authenticationService->loginUser(
                    trim($this->httpRequest->findParameter('email')),
                         $this->httpRequest->findParameter('password'),
                         $this->httpRequest->findParameter('remember') ? true : false
                );
            }

            // Two type of errors in one statement: failed login and form validation (DRY).
            // ↓ Either one would evaluate to true, but never both.
            if (!$isLoginFormValid || !$user) {
                // If login form inputs are valid, login has failed.
                // If login form inputs are invalid, login has not failed.
                // Logging in is not attempted when the login form inputs are invalid.
                $this->view->setHasLoginFailed($isLoginFormValid ? true : false);

                $this->view->setFormValueOf('csrfToken', $this->antiCsrfService->getToken());
                $this->view->setFormValueOf('email',     $this->httpRequest->findParameter('email'));
                $this->view->setFormValueOf('remember',  $this->httpRequest->findParameter('remember', false));

                foreach ($this->formValidationService->findErrorsOf('Login') as $key => $value) {
                    $this->view->setFormErrorCodeOf($key,  $value);
                }

                $this->view->index();

                return null;
            }

            // Set persistent user login cookie if User opted to stay logged in.
            if ($this->httpRequest->findParameter('remember')) {
                $this->view->setPersistentUserLoginCookie(
                    (string) $user->getIdentifier(),
                    (string) $user->getAuth()->getSeriesNumber(),
                    (string) $user->getAuth()->getToken()
                );
            }

            $this->view->setIsUserLoggedIn(true);

            // Token has been succesfully used.
            $this->antiCsrfService->regenerateToken();
        } else {
            $this->view->setIsFormTokenValid(false);
        }

        $this->view->index();
    }
}

You can view the LoginView class that's being controlled by this controller here, if interested.

\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

All in all, I'd say your controller is looking pretty good. There are no real issues concerning separation of concern, just like there were nonein your previous code I reviewed.
I have noticed one or two things that I'd describe as being odd, and even slightly worrying:

  • Possible violation of the Liskov principle, or basic inheritance rules in general (Contract violation)
  • Lack of abstraction in the constructor
  • Likelihood of heavy dependency on the dispatcher

Let me canter through these worries, and clarify what I mean:

Violation of Contract:
Your LoginController is extending the (abstract?) base Controller class. In doing so, it's overriding the constructor, adding a couple of specific dependencies that the controller needs. That, in itself, isn't a huge deal, but it doesn't sit well with me:

public function __construct(
    HttpRequest $hR,
    AuthenticationService $as,
    LoginView $lv,
    AntiCsrfService $acs,
    FormValidationService $fs
) {
    parent::__construct($hr, $as, $lv);//<-- this is a worry

Apart from the fact that you're passing the request object, the view and three services to the controller, you're invoking the parent constructor, passing it the first three arguments. Nothing wrong with that in and of itself, but it's the type-hints I'm worried about.

The Liskov principle states that preconditions (ie the type-hints) defined in the parent can't be strengthened in the child*. The LoginController's view type-hint, for example, is specifically targeting the LoginView class. It seems to me to be neigh on impossible that the base Controller is hinting at anything other than the base View class. If so, you are, technically breaking the inherited contract. This can, when you play around with abstract methods/classes, traits and interfaces lead to a fatal error (details, and an example here).

To put it in simple terms, if you create a LoginController instance, you're also creating a Controller instance (it extends Controller, therefore it is a Controller. Just check var_dump($loginControllerInstance instanceof Controller);, it'll dump true). This implies that any method or function that expects a Controller to be passed as an argument, expects that the value of that argument contains the methods and properties of the Controller class. Any instance you pass, then, must abide by the contract, set up by the Controller class. You violate this principle. Here's a basic example to demonstrate this:

abstract class Base
{
    protected $data = null;
    //contract states the constructor expects an array
    public function __construct(array $foo)
    {
        $this->data = $foo;
    }
}
//inherits contract, nothing wrong
class GoodChild extends Base
{}
//override, changing signatures... this is wrong
class InvalidChild extends Base
{
    //breaks the contract
    public function __construct(stdClass $data)
    {
        $this->data = $data;
    }
}
function testContract(Base $instance)
{
    $class = get_class($instance);//get class name
    //Base constructor expects an array,
    //so I assume (correctly) that its children do, too
    $data = array('foo' => 'bar');
    $newCopy = new $class($data);
}
$fine = new GoodChild(array());//<-- fine
$bad = new InvalidChild(new stdClass);//<-- this still works, but...
testContract($fine);//<-- no issues here
testContract($bad);//<---- fatal error

the instance of InvalidChild is accepted by the testContract function, on the basis that it is an instance of Base. The function hints for a Base instance, and has every right to assume that the methods defined in Base (names, visibility and signature) are available. By overriding the constructor, the function causes a fatal error, not because it's badly written, but because InvalidChild breaks the contract.
This is a dangerous situation. In case of a contructor, it can take a while before this causes problems (it might never cause you any grief, even), but that doesn't mean it isn't a problem. Sooner or later, ignoring the contract will come back and bite you. If not in this situation, not understanding/applying this principle to its fullest is not what you want.
If you're familiar at all with C programming, think of overriding a method, and strengthening the preconditions as code that might invoke undefined behaviour in certain cases. Undefined behaviour is a bug, waiting to happen.

Solution:
There are a few ways to address this issue. For example, ZendFramework's workaround (the old one, version 1.x) was to implement a _init method, and not use a constructor. This init method was automatically invoked, and in that method, you could set up your controllers individually.
Another option is to not define a constructor, and use the generic Controller::__construct, passing it the request, and through use of some magic involving get_called_class, or if your PHP version permits it, late static binding wizardry (ie using static:: instead of self::), you could work out what controller you're initializing and load the dependencies accordingly.

A more common, and IMO better, way of managing dependencies is through the implementation of patterns like a dependency injection container or a registry (I'm not a fan of the latter, but it is an option).

Lack of abstraction:
Because you've defined your own constructor, but the generic controller still requires the AuthenticationService for some reason, I'm wondering if you haven't made a couple of mistakes while writing this code. If you're passing a view, and a request object to the controller, but it still requires the AuthenticationService in all cases, I get the impression that you're still performing the basic user identification in the controller. By this I mean: is the Controller::__constructor the place where you're checking if the user has a session, if there are some cookies set, and/or what the referrer was? If this is the case: your base controller is violating the SRP, and you should probably put some more work into your router and/or dispatcher (or whatever you want to call it).

Solution:
Like I stated before: basic user authentication precedes the controller. Passing this service to the base controller also means that this service is initialized for every request, including any person who happens to stumble on your IndexController::index action. If this truly is what you want/need, you should understand that this reduces the flexibility of your code. In some cases, a light, non authenticating controller might be required. A controller that just requires the request object, and (possibly) a view. Nothing more, nothing less.

For that reason, it could prove useful to create an even more rudimentary base controller, and from it extend a AuthenticatingController, which is then used as the parent of all controllers that require the AuthenticationService

Dispatcher dependency:
Lastly: seeing as your controllers' constructors are the primary (and possibly only) points of injection, it seems likely that your dispatcher (where you initialize the controller) is where you check what controller is required, what dependencies it needs, initialize each and every one of those dependencies, and then construct the controller. That, to me at least, seems like a bit much for what is, essentially, one class in your system. It sounds like the job of a module to me.
There are, of course, a myriad of different ways to tackle this issue, and yes: dispatchers often end up looking either Very verbose, or messy and overly complex. That's simply because, in my experience, writing this piece of code as generically as possible is really rather difficult.

Solution
There is no best way to do this, but if my description of your hypothetical dispatcher is fairly accurate, I would urge you to rethink your approach. I recall writing a basic framework myself a few years back, and found this part particularly challenging, too. I ended up using an array of reflection tools (not only ReflectionClass, but also using the request, in combination with get_class_methods, looking for injection methods with an expression like /set(.+)Service/ to sniff out methods like setAuthenticationService and such...).

In the end, I always seem to advocate the approach I am currently using, and liking, which is to use config files, that specify how to initialize a given class. An example of this could be:

//this class' constructor is defined by Controller, its parent
LoginController.constructor=Controller
//after initializing, call setView, and pass it
//an instance of LoginView
//how this class is initialized can be defined in the same way
LoginController.setView=LoginView

Or, seeing as we're not living in the nineties anymore, you can use JSON or Yaml files:

{"classes": {
    "LoginController": {
        "init": {
            "constructor": "Controller.constructor",
            "setView": ["LoginView"]
        },
    "Controller": {
        "init": {
            "constructor": ["HttpRequest"]
        }
    }
}

Something like that

\$\endgroup\$
12
  • \$\begingroup\$ 1) Yes, my Controller is abstract. Since I cannot strengthen the type hint, I need to keep the base View type hint as is and that would get rid of the violation? This Liskov Principle kinda goes over my head. :( 2) In the base Controller I'm only setting the dependencies. Although it takes the AuthService as a dependency, any interaction with this service is being done in it's children. What do you mean by placing more work into my Router? Is it its responsibility to do these authentication checks? And yes, I'll be creating a rudimentary base controller as you suggested. \$\endgroup\$ Sep 12, 2014 at 8:45
  • \$\begingroup\$ 3) I'm actually using a simple Router, where I set the route mappings with callables that contain something like $dic->get('HomeController')->index();, and in the DIC all the dependencies are instantiated and injected into the controller. So my router is pretty much not doing anything then just routing / dispatching callables for defined URL mappings. \$\endgroup\$ Sep 12, 2014 at 8:46
  • \$\begingroup\$ @KidDiamond: On 2): Yes, the Router should do the authentication: if the user is not allowed to access a certain page, the router should redirect, the actual view, controller and action of that forbidden page should never be initialized in the first place \$\endgroup\$ Sep 12, 2014 at 9:23
  • \$\begingroup\$ @KidDiamond: On 1): basically, you should do away with the constructor all together, seeing as the abstract constructor is already defined, and it defines the contract. Possibly pass it the container, too, so you can define an abstract protected function init();, and implement it so that each controller uses the DIC to get its dependencies. It's the easiest fix I can think of that solves the liskov violation, and it's quite easy to do \$\endgroup\$ Sep 12, 2014 at 9:29
  • 1
    \$\begingroup\$ @KidDiamond: You're welcome... always happy to help. And yes, there is no one right way to do this. Some might suggest using a factory, or registry (I wouldn't, though). Others will favour the init approach. Either way: do what feels most natural to you. Creating the controller, and having set<servicename>Service methods is another option you can implement through traits or interfaces. In the end, it's a case of personal preference and pros and cons applied to your needs. Anyway: have fun (PS: no promisses, but I might review your FormValidator class this weekend, too, if you don't mind) \$\endgroup\$ Sep 12, 2014 at 11:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.