MVC Refactor, Take 2

I think we lost @Jeff_Mott? Jeff. What do you think of this direction?

I am on a short business trip. When I get back on Saturday, I’d like to try out the URL object mapping and formatting bit.

Scott

Still here. :slight_smile: Just been busy the past few days.

2 Likes

:smile:

Ahh, I was looking at it from the perspective of the router, where both approaches seemed to do essentially the same thing (instantiate a class then call a method that renders a template):

    $view = new SomeView('newThing.html.php');
    $response = $view->render();
    $controller = new SomeController();
    $response = $controller->someAction();

But it sounds like you’re actually talking about the overhead of writing an extra class, such as a controller class.

In Web MVC, if we truly had several wholly static pages (no user input, no database content), then we would make a generic controller. We might call it SimpleRenderController or something like that, and it would take the template to render as an argument from the router. Essentially we’d do it the way you’re doing it, but in Web MVC it’s not a rule to always do it that way. We would do it that way in cases where it helps (such as static pages).

The date and time examples are essentially also static pages (no user input, no database content), so in Web MVC, we’d probably use a generic controller for that too. So instead, let’s try some dynamic content (that is, database driven), which is by far the more common scenario in web applications. Let’s say there’s a template “thingList.html.php”, which we’ll reuse for “recently created list of things”, “recently viewed list of things”, and “search results list of things”.

First, a piece of code that both our approaches would use: a repository.

class ThingRepository
{
    public function getRecentlyCreated() { /* ... SQL */ }
    public function getRecentlyViewed() { /* ... SQL */ }
    public function search() { /* ... SQL */ }
}

And now we differentiate. In Web MVC, we would have three actions.

class ThingController
{
    // ...

    public function recentlyCreatedAction()
    {
        $things = $this->repository->getRecentlyCreated();
        return $this->templating->render('thingList.html.php', ['things' => $things]);
    }

    public function recentlyViewedAction()
    {
        $things = $this->repository->getRecentlyViewed();
        return $this->templating->render('thingList.html.php', ['things' => $things]);
    }

    public function searchAction()
    {
        $things = $this->repository->search();
        return $this->templating->render('thingList.html.php', ['things' => $things]);
    }
}

Contrasted with ToMVC, where we’d have three model classes.

class RecentlyCreatedModel
{
    // ...

    public $things;

    public function getThings()
    {
        $this->things = $this->repository->getRecentlyCreated();
    }
}

class RecentlyViewedModel
{
    // ...

    public $things;

    public function getThings()
    {
        $this->things = $this->repository->getRecentlyViewed();
    }
}

class SearchModel
{
    // ...

    public $things;

    public function getThings()
    {
        $this->things = $this->repository->search();
    }
}

If down the road the thingList template were to change (maybe it needs an extra argument), then I’d have to edit all three controllers and you’d have to edit all three model classes.

So here’s what I think I’ve concluded. For static pages, your approach is useful. Though, Web MVC can (and in real life does) use generic controllers to cater to this specific case. But for dynamic pages, which are by far the more common, the benefit of your approach seems severely diminished.

Depends a little on the specific circumstances. If the templates are related in some way (such as in our format example), then the controller can usually select the appropriate template through some string interpolation. But if we need to choose between several unrelated templates (seems unusual), then the code you showed is probably how we’d do it. In this scenario, your alternative approach would probably save a line of code or two (the one-line actions that invoke the common controller code). And we’d both need to add a route for each template.

Yes, that’s true for both of us. If I want my controllers to be able to use any of twig/blade/plain, or if you want your view classes to be able to use any of twig/blade/plain, and if those template engines don’t have compatible interfaces, then in both our approaches, we’d have to write adapter classes.

Web MVC doesn’t force us to use (or not use) either service locator or dependency injection. If you want to be able to substitute the model, then you can still take it as a constructor argument and let a dependency container construct the controller.

The problem with this approach, of course, is if the template needs some information from a model (See my clock example using clock.html.php and date.html.php). There’s no way you can do this using a generic controller because in web mvc the controller passes the data (in this case the date/time) to the model. You have to write a controller to achieve this. There are lots of cases where there is a read but no write: Displaying the latest 10 news stories, displaying a list of “new arrivals” products, displaying “on sale” products. All of these read from the database but require no interaction from the user.

You’re making a distinction between dynamic and static that isn’t really correct. There are actually 3 different cases:

  1. Static pages which are just rendering a template (e.g. Thank you for your enquiry we will get back to you shortly). You can achieve this in web mvc and tomvc with a generic controller

  2. Pages which display some information from the model/database in a predefined format e.g. latest news, latest products, special offers (which might use the same template as latest products!). You cannot achieve this in webmvc with a generic controller, with tomvc you can

  3. Pages which display some information from the model/database and allow the user to manipulate how it’s displayed (forms, search results, pagination, loading a specific record into a template e.g. viewing a product by ID)

Of course, but the point you were making is that by adding a ‘view’ I didn’t have one less class in the system:

Once you account for your template wrapper, ToMVC does have that one fewer class. If I want to support other template systems I add: TwigView, BladeView etc.

In which case, where is the decision made about which model to inject? If it’s in the router then my point from earlier still stands: If the router selecting the model and the controller, what is the problem with having it also select the view? If as you say it’s in the DIC, have the DIC also construct a view.

The problem with putting it in a DIC is that the configuration is spread among two places and potentially complex. Consider the clock example with two models that supply the same data from multiple sources. You need to now route to the DIC and fetch two differently configured instances of the controller on different routes, one for the NTP version and one for the standard version. This makes the router far more complex, something you were trying to avoid previously.

There is a simple conceptual issue with this and with a “true” MVC architecture. Normally, the controller in MVC has no interaction with the view. The controller is there purely for the manipulation of the model. The view then always calls the model to get the modified state.

The issue we have with web technology and MVC is, the actual “view” is in the web browser. On top of that, the “commands” needed for the controller are also coming from the web browser. There is a “conversion” to HTTP (or a web socket) required between the server and the browser. This interface simply needs to convert the HTTP request, run it through the MVC to get a response. Then the converter must send the response back. This is exactly what my example does.

Um, there is user input in my mini-MVC example. And it wouldn’t be hard at all to add a database layer to my example either. In fact, I’d say it is easier and more logical to add the database layer in this type of web MVC, now that all the data MUST be manipulated in the model (like it should be).

Scott

There’s another big problem with Web MVC as well: It ignores the fact there is such a thing as application state. It understands domain state but makes no attempt to account for the state of the application. As such, the application state is stored in the controller action in a very transient way.

Application state is things like:

  • What is the ID of the record being viewed/edited
  • Was the form submitted or not?
  • Is the submitted data valid?
  • Which page of results are we on?
  • What is the filter for the current set of results? e.g. search term, search order, category filter

The problem with storing this in temporary variables in the controller is that the controller has to give this information to anything that needs it and there’s no way of supplying the application state from anywhere else. Using a proper MVC approach where the state is in the model, the model can read the state from anywhere.

To be clear, here is how Web MVC would probably handle your clock/date scenario.

class GenericClockController
{
    public function doAction($clockObj, $template)
    {
        return $this->templating->render($template, ['time' => $clockObj]);
    }
}
if ($_SERVER['REQUEST_URI'] === '/clock') {
    $response = new GenericClockController()->doAction(new Clock(), 'time.html.php');
} else if ($_SERVER['REQUEST_URI'] === '/date') {
    $response = new GenericClockController()->doAction(new Clock(), 'date.html.php');
} else if ($_SERVER['REQUEST_URI'] === '/clock-ntp') {
    $response = new GenericClockController()->doAction(new NtpClock(), 'time.html.php');
} else if ($_SERVER['REQUEST_URI'] === '/date-ntp') {
    $response = new GenericClockController()->doAction(new NtpClock(), 'date.html.php');
}

Like I said in my last post, we’d do it essentially the way you’re doing it. I do agree this is the better approach for this specific scenario. To be clear, this scenario is static content with multiple variations. The reason you don’t see this kind of implementation very often in Web MVC applications is because this scenario doesn’t actually occur very often. Static pages are already few and far between (maybe a terms of service page, an about us page, things like that), and multiple variations of those pages almost never happens in real life (excluding i18n, which is typically handled in a completely different way). So, again, to be clear, Web MVC can and does shift to your kind of solution when it’s helpful to do so, but it usually isn’t.

Didn’t I cover exactly this with the “recently created”, “recently viewed”, and “search” list of "thing"s??

Again, this case describes the “list of things” that I gave example code for in my last post. In Web MVC, we’d make three controllers, and in ToMVC, you’d make three model classes. Both the controllers and your model classes would be coupled to the template.

The Template class was obviously generically named, but what it is is a plain PHP template rendering engine. It could have just as easily been, for example, a Twig template rendering engine. But what you did in your examples was remove the rendering engine and instead bake the template rendering logic straight into your view class. In real life, or even if we just expanded our examples a little further, you’d wouldn’t have been able to get away with that. Instead, your view classes should use a template rendering engine.

The dependency container config would define a variety of controller services, and the router would pick a particular service.

Because you’d be limiting your view selection options for no good reason. For example, here’s sample code from Fowler’s book:


He chooses between a couple templates based on what he gets back from the database. But if the router tells you which template you’re supposed to use, then you don’t get this kind of freedom.

I was talking about Tom’s date/time examples.

Well, I did make my example after his. There is a select in Tom’s example, where the user can select the time zone.

Scott

But that’s display logic based on a result. Put the if statement in the template. Chances are, some of the information about a Classic Album and an Album is the same (or we wouldn’t be using the same repository) so we can just use the if to dictate which version to show. The same is true of “Not Found”. This is clearly display logic “If no album is found, display an error” so should go in the view (the template).

By doing this, we can make a generic controller. I’ll use the same album example. The framework could supply a generic controller and interface:

class SingleEntityController {

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

	public function get($id) {
		$this->model->load($id);
	}
}


interface EntityModel {
	public function load($id);
}

The developer would then provide the model and template:

class AlbumModel implements EntityModel {

	public function __construct(AlbumRepository $repository) {
		$this->repository = $repository;
	}

	public function load($id) {
		$this->entity = $this->repository->findById($id);
	}

	//For the template to call..
	public function getEntity() {
		return $this->entity;
	}
}

Ok it’s half a dozen of one and six of another, but wait! That can be simplified can’t it, I’d have to write a model for Albums, Blogs, Cars, Products, etc. Consider this:

class RepositoryModel implements EntityModel {

	public function __construct(Repository $repository) {
		$this->repository = $repository;
	}

	public function load($id) {
		$this->entity = $this->repository->findById($id);
	}

	//For the template to call..
	public function getEntity() {
		return $this->entity;
	}
}

Now I can write a route that looks like this:

$model = new RepositoryModel(new AlbumRepository());
$controller = new SingleEntityController($model);
$view = new View('album.html.php', $model);

Or like this:

$model = new RepositoryModel(new BlogRepository());
$controller = new SingleEntityController($model);
$view = new View('blog.html.php', $model);

or

$model = new RepositoryModel(new ProductRepository());
$controller = new SingleEntityController($model);
$view = new View('product.html.php', $model);

In the example you provided, because there are domain concepts in AlbumController (is it an album or a classic album, does it exist or not?) as well as hardcoded template names, this kind of flexibility just isn’t possible and requires a new controller with similar logic for each and every type of domain entity.

edit: The difference between the two approaches is entirely about IoC.

All of this information(except for “is valid”) is stored in the request object. Why would anyone think it’s stored in the controller?

1 Like

It doesn’t seem that way at all to me. In the Web MVC examples, the repository and template rendering objects are still being injected. But in your examples, you’re not just abstracting away the objects themselves, you’re trying to abstract away the invocations we make on those objects, and trying to abstract away the arguments we pass to the abstracted invocations. Just because Web MVC doesn’t go to that extreme doesn’t mean its dependency objects aren’t still injected.

Chances are, sure, but not guaranteed. The two might actually be very different that we prefer to keep them as separate templates. But your approach that you advertise as more flexible doesn’t give us this choice.

Compared to Fowler’s original album = Album.find(id), your alternative strikes me as significantly more complex. I hope we get a lot of bang for this buck.

This relies on the very big assumption that albums, blogs and products all use the exact same logic. But one might load by ID while another loads by slug, or another might want to load using a pre-loading query, any of which would foil your reusability and force you to make separate model classes. Or one might be loading a single entity but still have other input to consider such as session data, which would force you to make separate controller classes.

I’m tempted to call your approach “premature refactorization”. You start with the assumption that certain chunks of code will be identical, then refactor as appropriate. But they often won’t be identical, and your prematurely refactored code ends up making things more rigid rather than more flexible.


I feel like this topic is almost talked out. I think the code comparisons were a lot better than our usual hand wavy claims; hopefully others thought so too. But obviously I’m not sold.

I’ll let you have the last word.

This is exactly my thinking too and why the request in my example MVC is passed on to the view, controller and model. The information needed to get all jobs done is in the request. However, the request is just the returned/ modified answer from the previous response, which was formed by the model. The view (the actual response) is only a representation of the model, be it HTML, JSON, PDF, etc. When you talk in “page controller terms”, you simply can’t say that really. The response in web MVC is a mixed representation of model and in some cases, too much controller logic (the dreaded fat controller). In other words, having to figure out what representation/ response needs to be sent in the controller with which model means a single controller class automatically breaks SRP. It is also why controllers in web MVC in the popular frameworks are rarely portable and hard to test.

Scott

If you ask me, the template selection excerpt of Fowler’s code belongs in the view, in the main “Album” template itself. So, depending on what data and state the model holds, the proper representation would be selected. This controller class is also clearly breaking SRP, IMHO. If the template decision needs to change, like a third album type is added, this controller would need to change. If the model source needs to change, I’d need to change this controller too. Two reasons for change at two different times = broken SRP. Granted, the model source probably won’t change at all. But, that is beside the point.

Scott

I realize now, I missed the most important part in my post above. The client clock model class. Doh! :open_mouth:

I am almost finished with including parameters in the URL to control anything the client wants. I’ll post everything again anew.

Scott

You’re constructing a template with a hardcoded filename. Just because it’s not a class doesn’t mean it’s not the same as new Template(). The same is true of a model, in most web mvc implementations the model is chosen using a service locator, again, going against IoC because the decision of what to include is being made in the controller, which means that the controller logic cannot easily be used with another template.

That’s just not true though is it? there’s no reason the template can’t do this:

if ($album instanceof ClassicAlbum) include 'classicalbum.html.php'
else include 'album.html.php';

This is a lot better than having the logic in the controller because the same controller can be used with this template or another template that does something entirely different.

That’s rather disingenuous. If we’re doing an apples to apples comparison it’s the model I provided vs fowler’s controller. In fowler’s example, the developer would need to write the entire controller, in my first example the developer needs to write a slightly shorter model. In my second, the developer has to write only the router configuration.

Which is true. Let’s say one of the templates needs some session data. If that’s the case you need to write a whole new controller action that loads the template and passes the session. Let’s start with fowler’s example (ported to php)

class AlbumController {
	
	
	public function doGet() {
	    $album = Album::find($request->getParameter('id'));
	    if ($album === null) $this->template->render('missingalbumerror.tpl.php');
	    else {
	    	if ($album instanceof ClassicAlbum) $this->template->render('classicalbum.tpl.php', ['album' => $album]);
	    	else $this->template->render('classicalbum.tpl.php', ['album' => $album]);
	    }

	}

}

Now from this base point, suppose I want a different template which displays something from the session and the album. I now need to add another function in the controller:

class AlbumController {
	
	
	public function doGet() {
	    $album = Album::find($request->getParameter('id'));
	    if ($album === null) $this->template->render('missingalbumerror.tpl.php');
	    else {
	    	if ($album instanceof ClassicAlbum) $this->template->render('classicalbum.tpl.php', ['album' => $album]);
	    	else $this->template->render('classicalbum.tpl.php', ['album' => $album]);
	    }

	}


	public function doGetWithSession() {
	    $album = Album::find($request->getParameter('id'));
	    if ($album === null) $this->template->render('missingalbumerror.tpl.php');
	    else {
	    	if ($album instanceof ClassicAlbum) $this->template->render('classicalbum.tpl.php', ['album' => $album, 'session' => $_SESSION['whatever']]);
	    	else $this->template->render('classicalbum.tpl.php', ['album' => $album, 'session' => $_SESSION['whatever']]);
	    }

	}

}

Admittedly, it would be possible to refactor the class and move the repeated logic into its own method… but this takes time!

On the other hand, with my approach, there is no need for this refactoring, you’d add the model:

class AlbumRepositoryModel extends RepositoryModel {
	public function getSession() {
		return $_SESSION['whatever'];
	}
}

and add the route accordingly to pull in the new template and the new model.

Alternatively if you need to load via a URL slug or another field in the database then it’s simple enough to make a generic model and controller to handle it (If it’s a common function like that).

I’m not sure I agree with this. Having used this approach for several years now (After migrating away from Web MVC as I found myself repeating code far too frequently and having to write far too much code to do simple tasks). With controllers for: Forms, Result sets (With extensions for paginate, search) and single entities, this handles 90%+ of my use-cases. On the odd occasion I do have to write a controller but in web mvc I’d have had to have written that controller anyway, and I’d also have had to have written it the other 90% of the times where I didn’t need to.

One last question if you don’t mind: I feel that this topic has been me explaining the advantages of a more mvc-like architecture and you mostly saying “You can do that in WebMVC too if you make these changes”, what I would like is a brief list of what you think the advantages are of web mvc over my suggested approach.

I’ll do the same for my approach:

  • Have to write significantly less code when just viewing a static template
  • Have to write significantly less code when loading data from a model without any user input
  • Have to write less code for common use cases (Forms, pagination, loading a single entity by ID)
  • Have to write the same amount of code otherwise
  • The model, view or controller can easily be substituted without rewriting the controller action
  • Which overall significantly lowers development time
  • Programming around interfaces gives a much more standardised approach so it’s easier for new developers to understand
  • Controllers don’t break SRP
  • Controllers don’t break encapsulation (Knowing what response type the view is)
  • Controllers don’t break LoD/Tell, Don’t ask
  • Looser coupling overall (Controllers are tightly coupled to models and templates in web mvc most of the time)

Ok, here goes nothing #2. This is going to start over, explaining my idea for MVC without a router. It also entails the “what ifs” Tom threw at me like what if the format needs to be different or the data needs to come from somewhere else. So I made a parameter builder. I’ll explain later.

Ok. Again, our entry is a front controller like Symfony’s.

app.php

<?php

/*
 * Generic entry point/ front controller.    
 */

require_once('HTTPConverter.php'); #


$httpConverter = new HTTPConverter();
$request = $httpConverter->createRequest();
$response = $httpConverter->handleRequest();
$httpConverter->sendResponse();

We call HTTPConverter.php

HTTPConverter.php

<?php


/*
 * A converter to convert HTTP to a request object and handle it to get a response
 *
 */

require_once ('Request.php');
require_once ('View.php');
require_once ('Controller.php');
require_once ('Model.php');

class HTTPConverter
{

    private $request;
    private $response;
    private $controller;
    private $model;


    public function __construct()
    {

        // prepare anything necessary

    }


    public function createRequest()
    {
        // create the request object
        $this->request = new Request();

    }

    public function handleRequest()
    {
        $this->model = new Model($this->request);

        // if the request is a write request (i.e. a post, delete or put), do the controller first
        if ( $this->request->isWrite() ) {
            $this->controller = new Controller($this->request, $this->model);
        }

        // you'll always need a response, so call the view to get it.
         $this->response = new View($this->request);
    }

    public function sendResponse()
    {
        echo  $this->response->render($this->model);

    }

}

This controls our MVC entry and exit points with request and response.

Like in the diagram I presented earlier.

We need the request object.

Request.php

<?php

/*
 * Create the Request object from global variables.
 */

class Request
{
    public $object;
    public $method;
    public $path;
    public $postData;
    public $urlParams;
    public $urlParamCount;

    public function __construct()
    {
       $this->setObject();
       $this->setURLParams();
       $this->filterPostData();
       $this->setMethod();

    }

    private function setObject()
    {
        $urlElements = explode('/', $_SERVER['REQUEST_URI']);
        $this->object = $urlElements[1];
    }

    private function setURLParams()
    {
        $urlElements = explode('/', $_SERVER['REQUEST_URI']);
        // because the object is always the first element and we already define its own variable, drop it
        $this->urlParams = array_slice($urlElements, 2);
        $this->urlParamCount = count($this->urlParams);


    }


    private function filterPostData()
    {
        //no filtering yet, but you get the point
        $this->postData = $_POST;
        return $this->postData;
    }

    private function setMethod()
    {
        switch ($_SERVER['REQUEST_METHOD']) {
            case 'POST':
                $this->method = 'update';
                break;

            case 'PUT':
                $this->method = 'create';
                break;

            case 'DELETE':
                $this->method = 'delete';
                break;

            case 'GET' :
                $this->method = 'read';
                break;

            default: 'read';
        }
    }

    public function isWrite()
    {

        return $this->method == 'update' OR $this->method == 'create' OR $this->method =='delete' ? true : false;

    }

    public function isRead()
    {
        return $this->method == 'read'? true : false;
    }

}

I added some logic to create the urlParams. They will be decisive in the later logic.

Now our first call to our application is only a read, so let’s show the view only.

View.php

<?php

/*
 * View class for creating an output
 *
 */

class View {

	private $template;
	private $model;
        public $headers;


	public function __construct(Request $request)
        {
            $this->template = $request->object . ".html.php";
	}

	public function render($model)
	{
            $headers = [];
            ob_start();
            require ( "./templates/".$this->template );

            return  ob_get_clean();
	}
}

In our HTTPConverter, we had first called on the initial model, which is all we need to render.

We have currently two choices (the parameter mapping needs to be expanded for more flexibility I know, but humor me on this). We can just call “/clock” or we can now call “/clock/ntp/date-only”. The initial result is the same.

Now we make a selection and need to go through a controller for the update and update the model.

Controller.php

<?php

/*
 * Our very, very, very thin controller! The thinnest controller ever!!!
 *
 */

class Controller {

	private $model;
        private $request;

	public function __construct(Request $request, Model $model) {
                $this->request = $request;
		$this->model = $model;
                $this->decideAction();
	}


        private function decideAction()
        {
            switch ($this->request->method){
                case "create":
                    $this->model->create();
                    break;
                case "update":
                    $this->model->update();
                    break;
                case "delete":
                    $this->model->delete();
                    break;
            }

        }

}

Here is our model.

Model.php

<?php

/*
 * A class for our model (building)
 *
 */

require_once('ClientModelFactory.php');



class Model {

	public $violations = [];
        private $request;
        public $clientModel;

        public function __construct(Request $request)
        {
            $this->request = $request;
            $this->buildClientModel();

        }

        public function buildClientModel()
        {
            $this->clientModel = ClientModelFactory::build($this->request);
            return $this->clientModel;
        }

	public function create()
	{
	    // create something new
	}

        public function update()
	{
	    $this->clientModel->update($this->request->postData);
	}

        public function delete()
        {
            // delete somthing
        }

}

And the ClientModelFactor.php

<?php

/*
 * A stimple factory to build our clientModels
 *
 */


class ClientModelFactory
{
    public static function build(Request $request)
    {
        $class = (ucfirst($request->object))."Model";
        require_once ("models/". $class .".php");
        if (class_exists($class)) {
            return new $class($request);
        }
        else {
            throw new Exception("Invalid object given.");
        }
    }
}

The trick now is the added logic to the client’s ClockModel.php, where the client creates all the necessary logic. This is actually where I want the parameters (the configuration) too, as it is ALL in one place. No crazy config files needed really. But, we can discuss the need, as there will be some for sure. At any rate, what was missing in my above example was the client’s model.

ClockModel.php

<?php

/*
 * Our clock model class
 *
 */

require_once('ObjectParameters.php');


class ClockModel {

    private $request;
    public $timezones = ['Europe/London' => 'London',
                          'America/New_York' => 'New York',
                          'America/Los_Angeles' => 'Los Angeles'];

    // this is the URL structure. The first elements is always the object, everything after the object will be your object parameters
    // we'll need to come up with a more flexible method later
    public $urlPathMap = '/clock/dataSource/dateFormat';

    // since you have them in your path map above, you must also declare the variable defaults! This should also be automated!
    public $objectParams = ['dataSource' => 'default',
                                   'dateFormat' => 'default'];
    public $timezone;
    public $submitted = false;
    public $time;

    public function __construct(Request $request)
    {
        $this->request = $request;
        $this->getObjectParams();
    }

    private function getObjectParams()
    {
        $objectParameters = new ObjectParameters($this->request, $this->urlPathMap, $this->objectParams);
        $this->objectParams = $objectParameters->parameters;
    }

    public function create()
    {
        // create something new
    }

    public function update($postData)
    {
        if ( $this->isValid($postData) ) {
            $this->submitted = true;
            $this->timezone = $postData['timezone'];
        }
    }

    public function delete()
    {
        // delete something
    }

    public function isValid($post)
    {
        return (null !== isset($post['timezone'])) ? true : false;
    }

    public function getTime()
    {   //var_dump($this->objectParams->parameters);
         if ( $this->objectParams['dataSource'] === 'default' ) {
            $timezone = new DateTimeZone($this->timezone);
            $date = new DateTime;
            $date->setTimeZone($timezone);
            return $date;
        }elseif (  $this->objectParams['dataSource'] === 'ntp' ){
            $date = $this->getNTPTime();
            return $date;
        }
    }

    public function getNTPTime()
    {
        // go and get the time from some NTP server (for now just do the same as default as an example)
        $timezone = new DateTimeZone($this->timezone);
        $date = new DateTime;
        $date->setTimeZone($timezone);
        return $date;
    }

    public function getCity()
    {
        return $this->timezones[$this->timezone];
    }

    public function success()
    {
            $this->submitted = true;
    }
}

I realize this client model is getting very fat. But, it wouldn’t be at all hard to refactor and make some new sub-objects, like for the different tasks, like UpdateClockModel, DeleteClockModel, CreateClockModel.

The new part I’ve added it the Object Parameters object.

ObjectParameters.php

<?php

/*
 * This is where we setup the object parameters.
 */

class ObjectParameters
{
    public $parameters = array();
    private $request;
    private $clientURLPathMap;
    private $parameterKeys;
    public $defaultObjectParameters;

    public function __construct(Request $request, $urlPathMap, $defaulObjectParameters )
    {
        $this->request = $request;
        $this->defaultObjectParameters = $defaulObjectParameters;
        $this->clientURLPathMap = $urlPathMap;
        $this->setParameterKeys();
        $this->buildParameters();
    }

    private function setParameterKeys()
    {
         $mapElements = explode( '/',$this->clientURLPathMap);
         $mapLength = count($mapElements);
         $this->parameterKeys = array_slice($mapElements, 2, $mapLength - 1);

    }

    public function buildParameters()
    {
        // this is in flexible, as we can't set up for different parameter needs. Need to refactor.
        if ( $this->request->isRead() AND $this->request->urlParamCount === count($this->parameterKeys) ) {
            for($i=0; $i < $this->request->urlParamCount; $i++) {
                $this->parameters[$this->parameterKeys[$i]] = $this->request->urlParams[$i];
            }
       }else{
           $this->parameters = $this->defaultObjectParameters;
       }

       // this is ugly. Refactor later. But, needed to set up hidden fields for our parameters.
       if ( $this->request->isWrite() ) {
           foreach( $this->request->postData as $key => $value ) {
               foreach ( $this->parameterKeys as $parameterKey ) {
                   if ( $key === $parameterKey ) {
                       $this->parameters[$parameterKey] = $value;
                   }
               }
           }
       }
    }

}

I am not too proud of this class, as I know it is mofugly code. But, I hope it gets the point across at what I am attempting to accomplish.

Edit: Of course, it also works.

:blush:

Scott

This is an interesting approach but having the model decode the URL does feel like mixing responsibilities to me, the model is now making decisions about the application flow based on the path. Personally I’d split that into two different classes (because then you could have both the NTP model and normal model in different classes with the same interface)… effectively reintroducing a router. However, what I like about this is that it’s not a centralised routing table, each MVC triad has its own set of routes and makes its own decisions.

Without trying this in a real project it’s difficult to assess if there will be any fundamental problems, but the lack of a controller entirely does feel like it would cause repeated logic somewhere and the fact you had to combine the two model variants is a red flag.

My other comment is this:

class Model {

	public $violations = [];
        private $request;
        public $clientModel;

        public function __construct(Request $request)
        {
            $this->request = $request;
            $this->buildClientModel();

        }

        public function buildClientModel()
        {
            $this->clientModel = ClientModelFactory::build($this->request);
            return $this->clientModel;
        }

	public function create()
	{
	    // create something new
	}

        public function update()
	{
	    $this->clientModel->update($this->request->postData);
	}

        public function delete()
        {
            // delete somthing
        }

}

This should almost certainly be an interface rather than a class. Let the application developer implement the interface rather than having these wrapper methods that don’t really do anything.

interface Model {
	public function create(Request $request);
	public function update(Request $request);
	public function delete(Request $request);
}

I am really glad you are open to the direction I’m looking at.

There definitely needs to be a logic for taking the purpose of the intended web page request and turning it into a correct output. Let’s not call it “URL decoding” or even a router/ controller mix, but application processing logic or workflow logic. It is the model of how the application/ framework works internally. I’d say this needs a real home too. Putting it in a router and then using the resulting parameters from the Request and the Router (objects) in a controller, the model and/ or the View is splitting the processing workflow out into 4 different areas. That, to me, avoids KISS.

The fact I have called for the application processing logic in the Model is only because I feel that is the right time to call it. But, theoretically, the application processing logic can be called to be built anywhere after the Request object is built.

I’d like to point out too, the “decoded URL” logic, as you called it, or rather the application processing logic, as I’d call it, is also needed, in most cases, in the View, which, according to the definition of MVC, should get its application state and data from the model. This is another reason for why I felt the call to build the application processing logic needs to be done in the Model.

LOL! Now we are sort of getting into the realm of Tony’s argument. :open_mouth: Is a model class that calls on the object to build application workflow logic (along with data and state) AND building the client’s Model (along with data and state) doing too much? I’d say no. Because, the definition of MVC calls for it. The base Model Class will need to do even more actually, like validation and authorization. However, it shouldn’t all be done in the single class, but through a good number of sibling classes (which can also be extended), from which the Model will get its functionality.

Absolutely and the actual beauty of the idea of doing a whole lot more, practically everything, in the model (and doing the absolute minimum, practically nothing, in the controller). This is what you opened my eyes to. The clock model can easily be refactored into separate classes, without having to duplicate any code. Doing the same with a router would be very close to duplicated code.

Agreed. I was thinking the whole time the client Model would need to follow a certain pattern/ interface, and I am going to be totally honest and say, I actually am not completely sure how to implement it properly. :blush: I am most definitely playing the student role here and am loving it! Thanks for taking the time to teach and discuss the alternatives. I hope Jeff isn’t terribly upset with me working in my ideas here too. :smile:

Scott

Ok. I’ve been thinking about the “ObjectParameters” a bit more and have decided this isn’t modeling the situation properly. For example, any parameters needed for View logic shouldn’t really be in a set of “object” parameters, but rather in a “view” parameter set. This would simplify calling the parameters in the view for any necessary formatting purposes.

I am considering using Symfony’s HTTP foundation to create the request object. It sets up ParameterBag objects within the Request object and in this fashion, I am thinking we need to have two additional sets of parameters added to the request object to make my suggested MVC system work properly. A view and an object parameter bag.

However, before we can create these parameter bags, we’d need to send off the request to a system, which would decode the request into the client developer’s intended “conditions”. This would be along the lines to the well known routing logic, but instead of saying “Ok, because the call to the server is this, do controller XYZ”, we simply create the necessary “conditions” for the view and the model and leave out the controller, because we don’t need it. These new parameters are then used to control either formating in the view or to add logic to help formulate or format the model.

Actually, let’s call it just that. FormatConditions.

This is what I am looking at now.

Now to put it into (proper) code.

Scott