Framework: [Proposal] Let’s return to MVC?

Created on 12 Apr 2017  ·  12Comments  ·  Source: laravel/framework

Current implementation of controller results in a ‘fat controller’, which is fat due to business logic it contains.

'Fat controller' violates some basic principles, for example:

  • Separation of concerns and single responsibility – in fat controller we have two responsibilities: business data gathering and model-view dispatching,
  • Business model role is moved from Model to Controller,
  • Obvious controller’s code structure repetition and disbalance:
    class Controller {
        public function action() {
            // fat block of business rules which should belongs to Model
            ...
            ...
            // A single row of dispatching code actually belongs to Controller
            return view(...);
        }
    }

The solution is obvious - we can move all the business logic from controller into some special class. This class belongs to Model component of MVC pattern. It describes the business (or application, or view) data. We will end up with two important sub-components of Model :

  • Domain Model – a raw data from DB, session, config, constants,…
  • { View | Business | App } Model – “compiled” row data made for every view.

Such solution is a kind of well known MVVM (Model – View - ViewModel) variation of MVC.

Below is a simple example to describe this proposal.
Current ‘fat controller’ implementation with DomainData access and business logic involved:

class UserController extends Controller
{
    // Display user info
    public function info($id)
    {
        // Mixed dispatch and domain data gathering responsibilities
        $user = User::find($id);            // Domain data access
        $name = $user->first_name.’ ‘.$user->last_name; // Business logic
        return view('info', ['name' => $name]);
    }
}

Proposed separation between ‘skinny controller’ and ViewModel with DomainData access and business logic implementation:

class UserController extends Controller
{
    // Display user info
    public function info($id)
    {
        // Only View-Model dispatch responsibility
        $data = UserViewModel::info($id);
        return view('info', $data);
    }
}
class UserViewModel extends ViewModel
{
    // Gather user info
    public function info($id)
    {
        // Domain data gathering responsibility only
        $user = User::find($id);            // Domain data access
        $name = $user->first_name.’ ‘.$user->last_name; // Business logic
        return ['name' => $name];
    }
}

Proposal consequences (without regards to be good or bad):

  • MVC compliance & strict separation of concerns between base MVC components.
  • Skinny controllers or even their elimination (router is already good in dispatching).
  • New valuable class ViewModel (Model for View) with potentially wide perspective, implementing well known MVC sub-component.
  • Quite simple migration: existing controllers becomes ViewModels with little changes.
  • Dramatic conceptual change from commonly accepted PRIME Controller to some unknown ViewModel.
  • ???

Most helpful comment

@svratenkov oh no. I completely agree now. I'm on board. Let's enforce standards that we don't state we follow because they are THE WAY TO GO!

I propose we also drop Active Model for built in support for Data Mapper component. Data Mapper is so much better (I'm sure the community will agree with me on this one).

I agree with your media point, also. We should drop all of the 'simple' examples from the documentation and have them follow strict rulesets that we can impose on all potential users of the framework (whether they are trying to understand it or returning users doesn't matter). All of our examples should be much more complex, multi-file, pseudo-randomly selected standard enforcing example applications.

With this proposal I reckon the controller example should be split out into three files. A model, a view and a controller. The controller should simply return the view and the model should simply provide the data. The view will simply render this data. The catch, you say? we do all the logic inside of middleware. That's right! One middleware for every single controller method, that way we keep all logic out of the controller and more tightly follow MVC architecture.

I sit, patiently, in excitement for your follow-up response. I'm eager to get things moving with you – you seem so very accepting of other methodologies that you must be some sort of senior senior web devloper!

/s
_Can we close this shit?_

All 12 comments

The repo is for bug tracking only. Its better that it should be discussed in laravel/internals.

You'll only have a 'fat' controller if you code them that way 😄

Laravel isn't very opinionated at this level, you're free to structure your controllers/models/views as you wish.

I would suggest investigating the Repository pattern which would achieve what you're after by the sounds of it, this goes hand in hand with dependency injection as I notice in your examples you're using eloquent directly.

This series is great https://laracasts.com/series/laravel-from-scratch-2017/episodes/23

@svratenkov

class UserController extends Controller
{
    // Display user info
    public function info(User $user)
    {
        // $name = $user->first_name.’ ‘.$user->last_name; This could be moved to a getNameAttribute function inside the model and use $user->name
        return view('info', ['name' => $name]);
    }
}

This is a more clean way to do it, an it uses a simple line 😄

Why would you complain about the structure of controllers in a way that heavily depends on misuse of a standard for it to break said standard? Nothing in Laravel forces you to hamfist business logic into the controller at all. Infact, there are loads of things to help around this, the biggest one that comes to mind (for me) is form requests.

This is, in parity, like using Symfony to develop an eCommerce app then complaining that the structure should change because there is no built in method to fly you to Mars. Symfony should fly you to Mars, change it immediately!1!11!

@ConnorVG Man that was brutal :laughing:.

@ConnorVG

Why would you complain about the structure of controllers in a way that heavily depends on misuse of a standard for it to break said standard? Nothing in Laravel forces you to hamfist business logic into the controller at all.

Dear friends, I am not complaining. May be I was not quite clear. What I told is smth like built in support for Model MVC component, which is absent now.
It's absence provokes non MVC programming. Do you need an example? - The best example is Controller section in the Laravel help. Of course, all other frameworks are similar in this aspect. But why Laravel should not be better?

How is it absent? You still call controllers which call your views and models. Did I miss something?

@svratenkov oh no. I completely agree now. I'm on board. Let's enforce standards that we don't state we follow because they are THE WAY TO GO!

I propose we also drop Active Model for built in support for Data Mapper component. Data Mapper is so much better (I'm sure the community will agree with me on this one).

I agree with your media point, also. We should drop all of the 'simple' examples from the documentation and have them follow strict rulesets that we can impose on all potential users of the framework (whether they are trying to understand it or returning users doesn't matter). All of our examples should be much more complex, multi-file, pseudo-randomly selected standard enforcing example applications.

With this proposal I reckon the controller example should be split out into three files. A model, a view and a controller. The controller should simply return the view and the model should simply provide the data. The view will simply render this data. The catch, you say? we do all the logic inside of middleware. That's right! One middleware for every single controller method, that way we keep all logic out of the controller and more tightly follow MVC architecture.

I sit, patiently, in excitement for your follow-up response. I'm eager to get things moving with you – you seem so very accepting of other methodologies that you must be some sort of senior senior web devloper!

/s
_Can we close this shit?_

@ConnorVG no issues. this has to be closed since we don't accept proposals here but in the internals repo. So don't worry bro :)

@ConnorVG Thank you, sir. We can close it now.

@svratenkov buddy you can close this PR as well since you are the author :smirk:

)))

Was this page helpful?
0 / 5 - 0 ratings