Silverstripe-framework: Controllers for SiteTree instances should be configurable

[robbieaverill]

At the current state of 4.0.0-alpha4 the controller for a SiteTree model must live in the same namespace as the model itself. This leads to controllers that are namespaced like Batman\Batcave\Model\MyPageController, where it might be more relevant to put it into the Batman\Batcave\Controllers namespace.

Via Slack it was briefly discussed, and an option is to add a configuration static to SiteTree which can define the controller name.

Options:

• The property (e.g. $frontend_controller) would be null by default
• You can define it for your own SiteTree classes
• If not defined, the SiteTree __construct would define it as the current behaviour, which is the FQ class name with Controller on the end (and _Controller) for deprecated support
• The logic from getControllerName can be moved to SiteTree::__construct in this case

Considerations:

• If the controller name is defined for a SiteTree instance, and it can't be found, we should throw an exception with a useful error message. Something like "Check your model's $frontend_controller property - it was defined but could not be found!"
• If we use a configuration static for this property it gives the benefit of being able to configure it in YAML, however;
• If we use a configuration static for this property, it opens the way for a developer to entirely replace a controller from a different module with their own. I personally like this way of extending functionality, but it's not something that exists in SilverStripe at this stage outside of the Injector - so do we want to introduce it for the sake of this?
• If not, we should use a protected property instead - this way you'd need to use the Injector to achieve the above behaviour (as you would currently anyway)

[sminnee]

I generally think this is a good idea but I think that extra logic in SiteTree::__construct() is a really bad idea: it's called literally hundreds of times on every pageview.

Instead, I think that a new method, SiteTree::getFrontendController() should implement both the config lookup and the legacy functionality, and return a controller instance.

[robbieaverill]

And how about the considerations around a member property vs a config static?

[sminnee]

use a config static i think, and pass its value to Injector::inst()->get($value) to get the controller, rather than using new

[robbieaverill]

Sorry, this should have been logged on the CMS module.

[dhensby]

From a purist's point of view shouldn't Controllers nominate the Model they handle and not the other way round? Controllers act on Models and so need knowledge about the Model but the Model doesn't act on a control and should be agnostic to what Controller(s) can "handle" it?

With a Config value don't we have a problem where every Model now needs this config? How about we have a SiteTree (or Page) config which is the controller namespace rather than the FQNS for the Controller class?

---

Edit: Maybe the namespace config won't work elegantly either modules extend Page and will have a different namespace

[sminnee]

From a purist's point of view shouldn't Controllers nominate the Model they handle and not the other way round? Controllers act on Models and so need knowledge about the Model but the Model doesn't act on a control and should be agnostic to what Controller(s) can "handle" it?

That's a good point. Strictly the ModelAsController would need a family of configuration that let is dispatch to the right controller based on the model that it looked up.

If this was done on an instance basis you'd be able to have two parallel websites based on the same SiteTree database, which would be interesting. It might lead to a better way of implementing things like multi-lingual sites and/or draft views.

With a Config value don't we have a problem where every Model now needs this config? How about we have a SiteTree (or Page) config which is the controller namespace rather than the FQNS for the Controller class?

You've already identified that simple rules could get too magical. However, another way to approach this would be to define a dispatcher interface that was tied into ModelAsController as a dependency:

class DataObjectControllerDispatch {
  function controllerFor(DataObject $model);
}

Then you could have a few different implementations:

• Suffix Controller or _Controller (a configurable array of possible suffixes would work)
• A remapping of model-namespace to controller-namespace with an optional suffix value
• A specific mapping of DataObject class to Controller class.

To work with modules and the like you'd probably need to have a cascade of such handlers, which return null if they couldn't find a match.

[robbieaverill]

Sorry, this is the issue - reopening!

[dhensby]

Do we want to allow @robbieaverill to get away with the most basic option (the original suggestion) which provides a resolution now and then we (cough @robbieaverill) can look into the alternatives?

[robbieaverill]

I think that something should be decided on and put in before beta, but I don't see a whole lot of merit in doing it in stages to be honest

[dhensby]

OK :)

[dhensby]

Well, I'd always like to follow the purist's path... ;)

[robbieaverill]

I like the idea of having a "dispatch" model for defining the relationship for a controller to a DataObject.

In a way I agree with @dhensby that it shouldn't be the job of the DataObject to define its controller, but at the same time I also don't think it should be the job of a controller to define the DataObject that it controls (unless you can define multiple), because it then limits a controller to 'controlling' a single DataObject.

I'm just trying to think... we want the configuration for this to either have a default, or be easy to configure... but we also want it to work so that if you have MyPage that extends Page, you don't necessarily need a controller for MyPage, and it would fall back to the controller for Page if not defined.

We potentially get into an area where the traditional one-to-one DO-to-controller becomes many-many, because a DataObject could be controlled by multiple controllers, and a controller could control multiple DataObjects 😄

We also need DataObject to be able to return its controller, so the inverse relationship needs to be easy enough to look up from wherever it is defined. If we define Controller: controls: MyDataObject in config then we have problems looking it up quickly because MyDataObject is value, not a key.

TL;DR

I'm not really producing anything worthwhile here, but basically I think for the sake of performance, we need to have a relationship from the DataObject to the Controller (in that direction) in the way it currently behaves. I'm 100% open to ideas about how to do the inverse efficiently though!!

Questions

• Do we want to aim for a many-many model to controller relationship for the future of SilverStripe, a one-to-one, one-to-many (model to controller) or many-to-one (model to controller)?
• If the controller defines the relationship, how can we establish that relationship quickly from the model?
• How much of this scope to we want to try and implement before beta? I assume that the solution we choose should be considered in whatever we implement now.

[kinglozzer]

I'm just trying to think... we want the configuration for this to either have a default, or be easy to configure... but we also want it to work so that if you have MyPage that extends Page, you don't necessarily need a controller for MyPage, and it would fall back to the controller for Page if not defined.

I think we could achieve that with Sam’s proposal. It’d just be a case of shifting the logic from getControllerName() into a dispatcher. As he also suggested, we could have “A remapping of model-namespace to controller-namespace with an optional suffix value”. That may be too expensive a look-up to go through on each page load, but you get the idea - there’s enough flexibility there to cater for most. What we’d provide by default I’m not sure!

We also need DataObject to be able to return its controller

I think we can do away with this - SiteTree::getControllerName() is the only way we do this in core (I think?), and as Dan said it’s arguably an anti-pattern anyway. We could still have a convenience method ::controller_for() somewhere - it’d just look through the dispatchers instead. For the VirtualPage issue this code originally targeted, we’d need to push an extra dispatcher to the front of the queue to handle VirtualPage specifically.

• Do we want to aim for a many-many model to controller relationship for the future of SilverStripe, a one-to-one, one-to-many (model to controller) or many-to-one (model to controller)?

My assumption is that it’d _effectively_ be many-to-one (model to controller), as the dispatcher lookup would stop as soon as it found the first suitable controller - even if many others are available for that model. There’d be nothing to stop you manually constructing a different controller instance for a model though.

[dhensby]

I think Controllers can/should only handle one type of Model - that's how it is at the moment anyway. At least where "type" is a parent class of the model that can be handled.

Page_Controller can only handle Page model (or any descendants of it).

A BlogListingPage_Controller can't handle a HomePage model because it won't conform to the correct API or have the right relationships.

I think a controller should be able to say it handles one object and that's about it. The default dispatcher will prioritise controllers in mysite first and then find the first defined controller from modules.

Regarding performant lookup by Model name, this can be cached as part of the manifest and accessed through ClassInfo.

[robbieaverill]

@dhensby that sounds good to me.

[sminnee]

I think that the idea that things should be bound based on the class that they belong to is "static thinking" and so generally should be avoided.

A dispatcher (or content-controller factory) is able to provide either a model-specific controller, or a controller designed to work with multiple models (e.g. any model that has a particular extension attached), depending on its implementation.

Interface:

namespace SilverStripe\CMS\Controllers;

interface ModelAsControllerDispatch {
  /**
   * Returns the controller used to handle the given model
   * @return Controller 
   * @throws OutOfBoundsException If the given model isn't handled by this controller
   */
  function controllerFor(DataObject $model);

  /**
   * Returns true if this dispatcher is able to provide a controller for the given model
   * @return boolean
   */
  function hasControllerFor(DataObject $model);
}

Class:

/**
 * Default mode-as-controller behaviour - suffixes the class with "_Controller" or "Controller"
 */
class DefaultModelAsControllerDispatch implements ModelAsControllerDispatch  { ... }

Built-in yaml:

SilverStripe\CMS\Controllers\ModelAsController:
  controller_dispatch:
   - SilverStripe\CMS\Controllers\DefaultModelAsControllerDispatch

Custom yaml:

SilverStripe\CMS\Controllers\ModelAsController:
  controller_dispatch:
   - Me\MyApp\SomeDispatcher

Resulting calls:

• The controller_dispatch config setting is an array of services names
• hasControllerFor() is called on each dispatcher in order until it returns true
• controllerFor() is called on that dispatcher

[sminnee]

With that in place we could provide a RemappedDispatch that had a config setting providing a number of namespace replacements to perform.

SilverStripe\CMS\Controllers\ModelAsController:
  controller_dispatch:
   - SilverStripe\CMS\Controllers\RemappedDispatch

SilverStripe\CMS\Controllers\RemappedDispatch:
  namespace_mappings:
    Me\MyApp\Models: Me\MyApp\Controllers
  class_suffix: ""

[sminnee]

You could also have a ModelSpecifiedDispatch that takes the coupled approach that was initially suggested:

<?php

/**
 * Allow models to provide a getFrontendController() method for their class
  */
class ModelDrivenDispatch implements ModelAsControllerDispatch {
  function hasControllerFor(DataObject $model)
  {
    return $model->hasMethod('getFrontendController');
  }

  function controllerFor(DataObject $model)
  {
    return $model->getFrontendController();
  }
}

[robbieaverill]

There's been a lot of talk about this. Currently in SS4 you can use controller_name config in your DataObject to control this behaviour. Is that enough to close this issue?

[kinglozzer]

I think so