Question about a design decision of MVC Controller actions?

[Hall_of_Famer]

Well lately I’ve been writing some ASP.NET MVC code, I noticed that the controller actions are defined much more easily and cleanly since C# supports method overloading and data annotation. Take an example of the edit action for user profile, in ASP.NET MVC you would define it like this:

public ActionResult Edit(int? id){
    //code for the default edit action.
}

[HttpPost]
public ActionResult Edit(UserProfile $profile){
     //code for edit action when a form is submitted.
}

This way, handling controller actions based on whether the user has submitted a form in a view page is very straightforward. Unfortunately, it is not possible with PHP, and in many PHP frameworks we have to use an if condition to check whether a forum is submitted, like this:

public function edit($id = NULL){
    if($this->request->method == "POST"){
        //action for form processing.
    }
    else{
        //default action for edit.
    }
}

Of course with return statement the else block can be omitted to make this look slightly better, it is still a bad design, especially considering the same if condition is checked in many controllers and actions for a typical application(unless your entire site has one and only form). The presence of this if statement itself is a code smell, and I definitely want to avoid it. Since PHP does not support method overloading, it’s impossible to apply the same technique in ASP.NET here.

I know there are several alternatives to remove the if statement. One way is to prefix or suffix the action method with HTTP method, thus splitting the action into two methods with different names, such as editGet() and editPost(). The controller’s base/parent class can intercept calls to action to determine which version of the action method to invoke. Another approach is to create action as class rather than method, and the action class can have methods such as get() and post(), or maybe even more, to handle different branches for a controller action. A third alternative is similar to the second one, but to split the controller into two controller, one GetController and one PostController, so the interception of action method based on request method is handled at controller level.

But the question is, what is the best solution to this problem? Of course I can stick to checking request method condition at each controller method, but as I said before I dont like it, repeated checks of the same if condition violates basic OOP principle, the presence of such an if condition is a code smell. But the three alternatives I brought up above all have issues as well, either making the interface ugly, or to introduce unnecessary complexity to the application. Do anyone of you know if there are better alternatives? If not, which one of the three alternative solutions I should look for? Thnks.

[Jeff_Mott]

@Hall_of_Famer

This way, handling controller actions based on whether the user has submitted a form in a view page is very straightforward. Unfortunately, it is not possible with PHP

How about:

    /**
     * @Route("/edit")
     * @Method("GET")
     */
    public function editAction()
    {
        // ...
    }
    
    /**
     * @Route("/edit")
     * @Method("POST")
     */
    public function updateAction()
    {
        // ...
    }

Two actions, same URL, distinguished only by GET/POST, just like in your ASP code where the actions are distinguished by the [HttpPost] annotation. That the PHP action methods have different names isn’t important. What’s important is whether the system knows which one to call.

[s_molinari]

Routing (Symfony Docs)

Routing (Symfony Docs)

Two different ways you can go about it with Symfony.

Scott

[mAAdhaTTah]

Most PHP frameworks have a router which will basically do that: Check the request type and call the correct method on the controller.

[TomB]

This is actually one of the fundamental issues with “Web MVC”: Poor separation on concerns. By having editAction and listAction in the same class, it forces view selection logic into the controller. Where you have multiple actions that need the same view (View the form and submit it) this often creates repeated code because both actions have to select the same view or you end up with controller action chaining ( submitAction() calls editAction() if there were errors), by improving the separation of concerns and removing the controllers responsibility for selecting the view, this redundancy is removed entirely, consider:

class MyForm {
	
	private $form;
	private $model;
	private $redirectUrl;

	public function __construct(Form $form, Model $model, $redirectUrl) {
		$this->form = $form;
		$this->model = $model;
		$this->redirectUrl = $redirectUrl;
	}

	public function showForm($id) {
		//Get some data based on $id and populate the form
		$this->form->populate($this->model->get($id));
	}

	public function save() {
		$this->form->populate($post);
		if ($this->form->isValid()) {
			if ($this->model->save($_POST)) $this->redirect($this->redirectUrl);
		}

	}
}

The beauty of a class like this is that it’s reusable for any form because it’s generic. The view is then generated from the same form object (so can itself check whether it was submitted/valid and display relevant errors), the validation is encapsulated in the form and the controller is usable with any form.

n.b, this is a simple example, assuming all forms will only redirect on success is a bad idea

[Jeff_Mott]

@TomB

n.b, this is a simple example, assuming all forms will only redirect on success is a bad idea

And not just redirection either. This simple example relies on other assumptions too, and as a result when I look at this, all I see are ways that this will make things harder, not easier. Personally, I don’t think this helps explain your point of view.

I remember a long time ago I asked you for step-by-step refactoring. Just like a bug report, start by making the simplest Web MVC example you can that demonstrates the problem you’d like to solve. Then, in very small, very incremental steps, show how you alleviate that problem. It doesn’t need to be fully OOP; even the benefits of Web MVC can be shown with simple procedural.

If you want to give that conversation another go around, we can start a new thread.

[s_molinari]

@Jeff_Mott

If you want to give that conversation another go around, we can start a new thread.

I’d be interested in this thread.

Scott

[Hall_of_Famer]

@TomB

This is actually one of the fundamental issues with “Web MVC”: Poor separation on concerns. By having editAction and listAction in the same class, it forces view selection logic into the controller. Where you have multiple actions that need the same view (View the form and submit it) this often creates repeated code because both actions have to select the same view or you end up with controller action chaining ( submitAction() calls editAction() if there were errors), by improving the separation of concerns and removing the controllers responsibility for selecting the view, this redundancy is removed entirely, consider:

class MyForm {

private $form;
private $model;
private $redirectUrl;

public function __construct(Form $form, Model $model, $redirectUrl) {
	$this->form = $form;
	$this->model = $model;
	$this->redirectUrl = $redirectUrl;
}

public function showForm($id) {
	//Get some data based on $id and populate the form
	$this->form->populate($this->model->get($id));
}

public function save() {
	$this->form->populate($post);
	if ($this->form->isValid()) {
		if ($this->model->save($_POST)) $this->redirect($this->redirectUrl);
	}

}

}

The beauty of a class like this is that it’s reusable for any form because it’s generic. The view is then generated from the same form object (so can itself check whether it was submitted/valid and display relevant errors), the validation is encapsulated in the form and the controller is usable with any form.

n.b, this is a simple example, assuming all forms will only redirect on success is a bad idea

Yeah I agree that the main issue here is that form processing and displaying the form are supposed to be two actions, but the traditional way of checking whether the form has been submitted merged them into one single action. This may make it easier to write code, it is in fact a bad design and something I want to get rid of.

I think your approach is very interesting, but I am also a bit confused. What does the controller do now that all the logic has been moved to a Form class? Can you show an example of how the MVC action controller now looks like after its major logic has been refactored into the Form class? Thanks.

[TomB]

@Jeff_Mott

I remember a long time ago I asked you for step-by-step refactoring. Just like a bug report, start by making the simplest Web MVC example you can that demonstrates the problem you’d like to solve. Then, in very small, very incremental steps, show how you alleviate that problem.

I’m happy to have that conversation another go with a couple or provisos:

• You provide the simple example of web mvc, when I’ve done this in the past by taking an existing tutorial as a basis you’ve said “You haven’t chosen a very good example”. Let’s use a basic form submission as above as it is a common, easily understood use-case. And as you say, simple is better, let’s keep examples that aren’t coupled to specific libraries/frameworks.

• We’re discussing architectures rather than specific implementations

• There are hundreds of ways of solving any given problem. Some of your previous arguments have rested on " $framework can achieve that using $component" which clearly isn’t helpful, instead demonstrate how $framework solves the problem with a minimal code example that contains only the relevant code (or just an interface that shows how $component would work)

• I’m not arguing against the premise that “$framework can already do that” what I’m interested in is the efficacy of the different approaches, instead of saying “You can do that in $framework” explain why it’s better (And how you’re measuring better) than other approaches discussed. Generally speaking the metric I’m using is better programming practices relating to flexibility: Better encapsulation, better SoC not breaking LoD/Tell Don’t Ask, composition over inheritance and easier to test/reuse components. I’ll try to highlight individual advantages where possible.

[TomB]

@Hall_of_Famer

I think your approach is very interesting, but I am also a bit confused. What does the controller do now that all the logic has been moved to a Form class? Can you show an example of how the MVC action controller now looks like after its major logic has been refactored into the Form class? Thanks.

I’ll take this a step back a little. Ignoring a form class for now, let’s just consider an MVC model (These are not domain models!) which handles all the business logic. The SoC here isn’t perfect and I’ll ignore the View for now but hopefully it should be a good stepping stone.

The framework (or developer) would supply a reusable controller:

class FormController {

	private $model;

	public function __construct(FormModel $model) {
		$this->model = $model;
	}

	public function showForm(...$args) {
		if ($args) $this->model->load(...$args);
	}

	public function save() {
		$this->model->populate($_POST);
		if ($this->model->isValid()) {
			if ($this->model->write()) $this->model->success();
			else $this->model->failure();
		}
		else $this->model->failure();

	}
}

and an interface for models that use it:

interface FormModel {
	public function load(...$args);
	public function populate($data);
	public function isValid();
	public function write();
	public function success();
	public function failure();
}

The application developer then provides only the model that implements the interface:

class MyForm implements FormModel {
	private $data;
	private $errors = [];

	public function load(...$args) {
		$this->data = $this->orm->find($args[0]);
	}

	public function populate($data) {
		$this->data = $data;
	}

	public function isValid() {
		//Validate $this->data, as this is its own class it can ask for any validator/domain model classes it needs to do this in its constructor
		//During validation, write any errors found to $this->errors
	}

	public function write() {
		$this->orm->save($data);
	}

	public function success() {
		$this->app->redirect('/thanks');
	}

	public function failure() {
		//Log the failure, write a message to a variable for use in the view, whatever
	}
}

Just to be totally clear: The application developer would only write this class and the relevant view. The view then has direct access to the model, so the application developer can supply any additional methods needed by the view in the model. The view can then do this:

if (!$model->isValid()) {
	?>
	Sorry there was a problem submitting your form, please check the following: <?= implode(', ', $model->getErrors());

	<?php
}

?>

<form action="form/save">

<input type="text" name="firstname" value="<?= $model->getData('firstnane');" />

</form>

As you can see, to make the view work I’ve added a couple of extra methods to the model.

The router, rather than routing to a controller action, would route to a view, model and controller (Alternatively the router can work out which controller to use based on the interface implemented by the model) allowing the same controller/view to be used with different models, different models to be used with the same view/controller or substituting the controller and using the same view/model or any combination thereof, use the same model with a different controller and a different view.

[s_molinari]

If I understand your mini-MVC example correctly, I’d like to suggest one little refactor. In the FormController, I’d like to see the method showForm() just be show(), as I would imagine the URL calling the view would be /form/show/args. That would be in alignment with the URL for the form’s submission /form/save. Or, change the save() method to saveForm(), just for the sake of standardization.

And how would I get from the front controller to the form controller and/ or to call the view? You lost me on the routing bit.

Edit: btw, I am very keen on this different way to go about web MVC. I see and want the potential, because it is breaking away from the old page controller pattern, which a lot of frameworks I’ve seen try to still follow, which also makes, as I see it, reusability of client side code practically impossible??? (it is a question )

Scott

[TomB]

@s_molinari

And how would I get from the front controller to the form controller and/ or to call the view? You lost me on the routing bit.

Without a router (as I think it’s easier to explain first) the MVC triad gets instantiated like this:

$model = new MyForm();
$controller = new FormController($model);
$view = new TemplateView('form.tpl', $model);

if ($request->method == 'POST') $controller->submit();
else $controller->show($_GET['id'] ?? null);

echo $view->output();

Assume that TemplateView is a simplistic class that loads a .tpl file (Which I provided in the last post) and makes $model available to it.

The router needs to take a url and match it to model/view/controller. How this works internally isn’t really important

interface Router {
 public function register($route, $triad, $actions);
 public function match($url, $method);
}

$router->register('/myform/{action}/{args}*', ['model' => 'MyForm', 'controller' => 'FormController', 'View' => 'TemplatedView'], ['show' => ['GET', 'showForm'], 'submit' => ['POST', 'submit']);

Some of this could be worked out using a convention-over-configuration approach but for simplicity I’ll just assume we’re using configuration for everything.

The obvious problem with this? It’s impossible to provide the constructor arguments to the class names, it’s an easy fix, each entry (apart from action) has two parts, a name and args. I’ll do this in JSON as it’s clearer:

routes.json

{
	[
		{
			"url": "/myform/{action}/{args...}",
			"model": ["MyForm", ["Constructor Arg 1", "Constructor Arg 2"]],
			"controller": ["FormController", ["Constructor Arg 1", "Constructor Arg 2"]],
			"view": ["TemplatedView", ["myform.tpl"]],
			"actions": {
				"show": {"method": "GET", "action": "showForm"},
				"submit": {"method": "POST", "action": "submit"},
			}			
		}
	]
}

Again, this could easily be achieved with a convention over configuration approach:

• The url could map to the model, so /myform/* assumes the model MyForm
• The controller can be worked out from the model, if it implements FormModel it can be assumed that FormController should be used
• The View could share a name with the model and uses a default template class unless otherwise specified, e.g. /myform/* loads myform.tpl using the TemplatedView class
• The actions are worked out by reflecting the controller’s methods

When $router->match() is called with /myform/show/1234 it returns the data structure stored in the router (Whether this is manually configured or uses the convention-over-configuration approach I just described isn’t really relevant, but assume that when:

$_SERVER['REQUEST_URI'] = '/myform/show/1234';
$_SERVER['REQUEST_METHOD'] = 'GET';

$route = $router->match($_SERVER['REQUEST_URI'], $_SERVER['REQUEST_METHOD']);

//$route object:
{
			"model": ["MyForm", ["Constructor Arg 1", "Constructor Arg 2"]],
			"controller": ["FormController", ["Constructor Arg 1", "Constructor Arg 2"]],
			"view": ["TemplatedView", ["myform.tpl"]],
			"action": "show",
			"args": ["1234"],		
		}

Again, all this is implementation details so isn’t really anything to do with MVC. The dispatcher then simply constructs the MVC triad from $route. A DIC is almost certainly required here otherwise the dispatcher needs to be told about all the dependencies of the classes it’s instantiating

//Assume param 1 is class name and param 2 is supplied arguements
$model = $dic->create($route->model[0], $route->model[1]);
$controller = $dic->create($route->controller[0], array_merge($model, $route->controller[1]));
$view = $dic->create($route->view[0], array_merge($model, $route->view[1]));

$controller->{$route->action}(...$route->args);

echo $view->render();

Hopefully that makes sense, there’s a fine line between completeness and simplicity in examples!

@s_molinari

btw, I am very keen on this different way to go about web MVC.

Just to be clear, this isn’t a new idea and it’s not something I can claim to have come up with, this is just MVC as it was described before web developers got their grubby little hands on it and decided MVC models weren’t required

[s_molinari]

@TomB

Hopefully that makes sense, there’s a fine line between completeness and simplicity in examples!

It makes sense to me. The only thing I am wondering about is the constructor args. What are they and where are they coming from?

Scott

[TomB]

@s_molinari

It makes sense to me. The only thing I am wondering about is the constructor args. What are they and where are they coming from?

Ah sorry, I should have elaborated on that. It’s possible (Likely?) that the Controller/Model/View classes will have some specific constructor arguments. The view, in this example, requires the name of the template to load ‘myform.tpl’. By just having a router that routed to a class name e.g.

{
	[
		{
			"url": "/myform/{action}/{args...}",
			"model": "MyForm",
			"controller": "FormController",
			"view": "TemplatedView,
			"actions": {
				"show": {"method": "GET", "action": "showForm"},
				"submit": {"method": "POST", "action": "submit"},
			}			
		}
	]
}

There’s no way to specify that when TemplatedView is loaded, that it needs to be passed the name of the template to load. Similarly, the Model and Controller may also have some configuration options which need to be set in the constructor. For example, a model may need access to an ORM, or Session data in its constructor, or just some specific configuration for that instance. Of course you can let a DIC handle this entirely, but that spreads the configuration into two different places which isn’t ideal.

By specifying the constructor arguments as part of the route configuration:

{
	[
		{
			"url": "/myform/{action}/{args...}",
			"model": ["MyForm", ["Constructor Arg 1", "Constructor Arg 2"]],
			"controller": ["FormController", ["Constructor Arg 1", "Constructor Arg 2"]],
			"view": ["TemplatedView", ["myform.tpl"]],
			"actions": {
				"show": {"method": "GET", "action": "showForm"},
				"submit": {"method": "POST", "action": "submit"},
			}			
		}
	]
}

The view can be correctly passed ‘myform.tpl’ as an argument.

[TomB]

{
	[
		{
			"url": "/myform/{action}/{args...}",
			"model": ["MyForm", ["Constructor Arg 1", "Constructor Arg 2"]],
			"controller": ["FormController", ["Constructor Arg 1", "Constructor Arg 2"]],
			"view": ["TemplatedView", ["myform.tpl"]],
			"actions": {
				"show": {"method": "GET", "action": "showForm"},
				"submit": {"method": "POST", "action": "submit"},
			}			
		}
	]
}

I just want to add that a real benefit of this approach is that unused components can be ignored. Consider a static view that doesn’t load any data from a model and doesn’t have any user actions. Perhaps the “Thank you for submitting your enquiry, we’ll be in touch” page that the form submission redirects to:

{
	[
		{
			"url": "/myform/thanks/",
			"view": ["TemplatedView", ["thanks.tpl"]],
	        }
	]
}

Or perhaps something that shows the latest 5 news stories. Again, no user interaction, the developer supplies a model:

class LatestNewsModel {
	
	public function getNews() {
		return $this->orm->limit(5)->sort('date DESC');
	}
}

The relevant view:

foreach ($model->getNews() as $news) {	
	echo $news->title .'<br />';
}

and then adds the route. No need for a controller

{
	[
		{
			"url": "/latestnews",
			"model": ["LatestNewsModel"]
			"view": ["TemplatedView", ["news.tpl"]],
		}
	]
}

And by using a convention-over-configuration approach the routes don’t even need to be added Just add thanks.tpl in the right place and it’s available.

[s_molinari]

And now all I can think about is how to get devs to stop thinking about “actionThis” or “actionThat”. LOL!

Scott