Single Task Controllers RFC

[Mathewlenning]

These are single task controllers that are intended to be used directly. I.E. Not inherited. Each controller is responsible for one task. Extensions can use these controllers to execute each task so long as their model supports the interface the controller expects.

This is a big change compared to the old legacy system where extension developers inherited functionality. The key benefits of this design are:

1. Reduction of duplicate code.
2. Ability to add new tasks without worrying about BC breaks
3. Clear separation of responsibility
4. Cleaner code.
5. Move business logic into the model
6. One model can be used for list and form views
7. No need to create empty controllers.
8. 100% backwards/forwards compatible

To understand how this system works please see the dispatcher class as it is the key to the entire system.
Extensions will have to create a controller that extends JCmsControllerDispatcher and then execute it in their extensionName.php entry point.

I've also created model classes that support these new controllers. See https://github.com/Mathewlenning/joomla-cms/tree/CMS-Model-Classes

[wilsonge]

I don't think we need to have DIRECTORY_SEPARATOR here - it should be fine with just a forward slash (which is why J removed the DS constant after 3.5)

[Mathewlenning]

@wilsonge Thank you very much for catching the slips. Now all I need to do is figure out how to get past Travis CI. These controllers assume auto-loading, so how can I explain that to Travis?

[wilsonge]

It's because you have the autoloading of your classes wrong (I think) :P the CMS starts the autoloader in the cms folder. So for the location libraries/cms/controller/base.php it's expecting a class JControllerBase (which it doesn't find) - hence the error.

So you need to do two things
A) Remove the CMS before each class name
B) Rename JCmsControllerBase because that will conflict with JControllerBase in the core class. Just rename it basecms.php and call it class JControllerBasecms or similar :) it's your choice what to actually use

[wilsonge]

I'd much rather see us return parent::execute in situations like this - because that can return false or true as well

[Mathewlenning]

I agree. I'll make the change when I wake up tomorrow.

Sincerely,
Mathew Lenning

Babel-university.com

P.S. This message was sent via iPhone, so please forgive any errors

On Apr 26, 2014, at 12:34 AM, George Wilson [email protected] wrote:

In libraries/cms/controller/add.php:

• */
• public function execute()
• {

•   $config = $this->config;
  

•   $prefix = $this->getPrefix();
  

•   $model = $this->getModel($prefix, $config['subject'], $config);
  

•   if (!$model->allowAction('core.create'))
  

•   {
  

•       $msg = $this->translate('JLIB_APPLICATION_ERROR_CREATE_RECORD_NOT_PERMITTED');
  

•       $url = 'index.php?option='.$config['option'].'&task=display.'.$config['subject'];
  

•       $this->setRedirect($url, $msg, 'error');
  

•       return false;
  

•   }
  

•   parent::execute();
  

  I'd much rather see us return parent::execute in situations like this - because that can return false or true as well

�
Reply to this email directly or view it on GitHub.

[Mathewlenning]

Thanks! Is there a reason why we aren't including the "cms" in the naming scheme?

A true representation of the directory structure seems like it would make the lib easier to navigate (autoloading & human) as well as prevent naming conflicts.

I get the reasoning behind libraries/joomla because JJoomla is insane, but CMS specific classes should make that explicitly clear in their name.

Sincerely,
Mathew Lenning

Babel-university.com

P.S. This message was sent via iPhone, so please forgive any errors

On Apr 26, 2014, at 12:24 AM, George Wilson [email protected] wrote:

It's because you have the autoloading of your classes wrong :P the CMS starts the autoloader in the cms folder. So for the location libraries/cms/controller/base.php it's expecting a class JControllerBase (which it doesn't find) - hence the error.

So you need to do two things
A) Remove the CMS before each class name
B) Rename JCmsControllerBase because that will conflict with JControllerBase in the core class. Just rename it basecms.php and call it class JControllerBasecms or similar :) it's your choice what to actually use

�
Reply to this email directly or view it on GitHub.

[wilsonge]

Well part of it was because we're moving stuff over from libraries/joomla to libraries/cms and making them both as parents was because it kept b/c when we moved stuff across (eventually the joomla folder will be removed and we'll just have the framework classes in the composer vendor folder and the other classes in libraries/cms)

[Bakual]

Thanks! Is there a reason why we aren't including the "cms" in the naming scheme?

Because for the J prefix it doesn't matter if the file is in the cms, legacy or joomla folder. It will search in all places.

[wilsonge]

@Bakual not quite. We still have to load each folder individually

the /cms is here https://github.com/joomla/joomla-cms/blob/staging/libraries/cms.php#L30
the /legacy here https://github.com/joomla/joomla-cms/blob/staging/libraries/import.legacy.php#L59
and /joomla here https://github.com/joomla/joomla-cms/blob/staging/libraries/loader.php#L396

We just choose to give them all the J Prefix. It's not a requirement

[wilsonge]

same with parent::execute() being returned here

[wilsonge]

So one of the most obvious major issues I see with this is permission checking. Currently we're running the checks like

if (!$model->allowAction('core.create'))

which is nice for core. But means you have to extend the controller for any extension with it's own permissions section you need to override the whole core permissions structure - which seems excessive. So I'd like to see the permission to be a class variable?

Longer term maybe we could even try and think of a way of seeing if the component permissions exist?

Finally I'm not a software logic person so i might well be wrong but to me permissions checking belongs in the controller and not the model??

[Mathewlenning]

Since these controllers are not intended to be overridden putting permission checks in the controller would defeat the purpose, because you would have to override the controller (I.E inherit the controller class)

However since the actual check is done in the JUser object, pushing this into a model function allows the developer to do the override there.

The permission checks were/are already in the model in legacy, the difference is that now all checks (controller & model) use the same function.

This means that to customize the ACL for you only need to do it in one place.

Sincerely,
Mathew Lenning

Babel-university.com

P.S. This message was sent via iPhone, so please forgive any errors

On Apr 26, 2014, at 9:23 AM, George Wilson [email protected] wrote:

So one of the most obvious major issues I see with this is permission checking. Currently we're running the checks like

if (!$model->allowAction('core.create'))
which is nice for core. But means you have to extend the controller for any extension with it's own permissions section you need to override the whole core permissions structure - which seems excessive. So I'd like to see the permission to be a class variable?

Longer term maybe we could even try and think of a way of seeing if the component permissions exist?

Finally I'm not a software logic person so i might well be wrong but to me permissions checking belongs in the controller and not the model??

�
Reply to this email directly or view it on GitHub.

[wilsonge]

Looks like in typical Joomla fashion we split it between controller + model :P (https://github.com/joomla/joomla-cms/blob/staging/libraries/legacy/controller/form.php#L191 - for example). So in that sense I guess as long as we make it consistent. I dunno if anyone knows what's architecturally the right way??

Anyhow my point is the developer cannot override in the model some of the time because the permission to be checked is being defined in the controller (see https://github.com/joomla/joomla-cms/pull/3503/files#diff-b2b39560579bd072dcfeb6bbc1d454bbR39 for example). How in my component foobar I don't want to check core.create i want to check foobar.create (and in the docs http://docs.joomla.org/Adding_ACL_rules_to_your_component#Add_Rules_Field_to_Your_Form action asterman.edit.basic). So if you don't want people overriding the model I don't think defining the permission name in the controller is a viable option

[Mathewlenning]

I think that in order to really grasp what how this design works, one needs to change the way we think about the Model.

Most people think of the model as a data provider or an extension of the data layer, but to me the model is more like a dictionary. In the sense that it gives meaning to the terms used in the MVC.

The controllers I've built are clueless. They don't know and don't care what the significants of $model->allowAction('core.edit) means, they just want a true or false.

This gives the developer the freedom to give meaning without subclassing the controller.

An added benefit is that when you need to make these checks in the view (think toolbar) you use the model instead of JFactory::getUser();

Does this make sense?

Sincerely,
Mathew Lenning

Babel-university.com

P.S. This message was sent via iPhone, so please forgive any errors

On Apr 26, 2014, at 9:23 AM, George Wilson [email protected] wrote:

So one of the most obvious major issues I see with this is permission checking. Currently we're running the checks like

if (!$model->allowAction('core.create'))
which is nice for core. But means you have to extend the controller for any extension with it's own permissions section you need to override the whole core permissions structure - which seems excessive. So I'd like to see the permission to be a class variable?

Longer term maybe we could even try and think of a way of seeing if the component permissions exist?

Finally I'm not a software logic person so i might well be wrong but to me permissions checking belongs in the controller and not the model??

�
Reply to this email directly or view it on GitHub.

[mbabker]

You have to override somewhere to deal with non-standard or generic
behavior, be it controller or model. Pick your poison.

Given the core.action convention is our standard, I have no issue with that
being hard coded in and requiring overrides to do anything else; in fact
I'm pretty sure it's that way right now.

On Fri, Apr 25, 2014 at 8:39 PM, George Wilson [email protected]:

Looks like in typical Joomla fashion we split it between controller +
model :P (
https://github.com/joomla/joomla-cms/blob/staging/libraries/legacy/controller/form.php#L191- for example). So in that sense I guess as long as we make it consistent.
I dunno if anyone knows what's architecturally the right way??

Anyhow my point is the developer cannot override in the model some of the
time because the permission to be checked is being defined in the
controller (see
https://github.com/joomla/joomla-cms/pull/3503/files#diff-b2b39560579bd072dcfeb6bbc1d454bbR39for example). How in my component foobar I don't want to check
core.create i want to check foobar.create (and in the docs
http://docs.joomla.org/Adding_ACL_rules_to_your_component#Add_Rules_Field_to_Your_Formaction
asterman.edit.basic). So if you don't want people overriding the model I
don't think defining the permission name in the controller is a viable
option

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3503#issuecomment-41453847
.

[mbabker]

Longer term maybe we could even try and think of a way of seeing if the component permissions exist?

Our current system defaults true if permissions aren't explicitly defined IIRC.

[Mathewlenning]

Actually I do want people overriding the model. If they don't want to use core.create then they can translate it to whatever action they like in the model.

Since it's consistent throughout the controllers a simple

$actions = explode('.', $action);

$mycustomeizedaction = whatever.whatever.$actions[1];

Would do the trick, but of course there are a billion ways this could be achieved, so leaving that up to the developer is our best bet.

Sincerely,
Mathew Lenning

Babel-university.com

P.S. This message was sent via iPhone, so please forgive any errors

On Apr 26, 2014, at 10:39 AM, George Wilson [email protected] wrote:

Looks like in typical Joomla fashion we split it between controller + model :P (https://github.com/joomla/joomla-cms/blob/staging/libraries/legacy/controller/form.php#L191 - for example). So in that sense I guess as long as we make it consistent. I dunno if anyone knows what's architecturally the right way??

Anyhow my point is the developer cannot override in the model some of the time because the permission to be checked is being defined in the controller (see https://github.com/joomla/joomla-cms/pull/3503/files#diff-b2b39560579bd072dcfeb6bbc1d454bbR39 for example). How in my component foobar I don't want to check core.create i want to check foobar.create (and in the docs http://docs.joomla.org/Adding_ACL_rules_to_your_component#Add_Rules_Field_to_Your_Form action asterman.edit.basic). So if you don't want people overriding the model I don't think defining the permission name in the controller is a viable option

�
Reply to this email directly or view it on GitHub.

[Bakual]

So one of the most obvious major issues I see with this is permission checking. Currently we're running the checks like if (!$model->allowAction('core.create')) which is nice for core. But means you have to extend the controller for any extension with it's own permissions section you need to override the whole core permissions structure - which seems excessive.

core.create is an ACL permission provided by core which can and should also be used by extensions. It's not meant to be used only by our core extensions. So this usage is perfectly fine.

[beat]

I see a lot of "duplicate" (duplicate is not the right word here as it's over a dozen copy-pastested lines of) code, and that's for me an indication of a software architecture issue.

[Mathewlenning]

Are you honestly suggesting there is more copy paste in these controllers than the current implementation?

Sincerely,
Mathew Lenning

Babel-university.com

P.S. This message was sent via iPhone, so please forgive any errors

On Apr 26, 2014, at 5:04 PM, beat [email protected] wrote:

I see a lot of "duplicate" (duplicate is not the right word here as it's over a dozen copy-pastested lines of) code, and that's for me an indication of a software architecture issue.

�
Reply to this email directly or view it on GitHub.

[Mathewlenning]

But perhaps I'm miss understanding your comment. Please explain in more detail.

Sincerely,
Mathew Lenning

Babel-university.com

P.S. This message was sent via iPhone, so please forgive any errors

On Apr 26, 2014, at 5:04 PM, beat [email protected] wrote:

I see a lot of "duplicate" (duplicate is not the right word here as it's over a dozen copy-pastested lines of) code, and that's for me an indication of a software architecture issue.

�
Reply to this email directly or view it on GitHub.

[dongilbert]

Be sure to run these through PHP CodeSniffer using the Joomla rules, which can be found here - https://github.com/joomla/coding-standards

[dongilbert]

For your $this when not in object context error, you'll need to search the code for places where you might have put $this and shouldn't have. These would include static methods, standalone functions and things like that.

[wilsonge]

Don't stress about the tests - it's not due to your code - it's a Joomla problem with the tests after travis updated PHPUnit - I have a fix pending for it at #3550

[Mathewlenning]

@wilsonge Glad to hear it! I was surprised because I don't have any statics methods/classes in either the controllers or the models.

@dongilbert I'll run them through the sniffer before I commit tonight.

[Mathewlenning]

So now we have the ability to do an internal redirect to allow for chaining, but I'm not 100% happy with it yet.

[Mathewlenning]

I just want to make one more attempt to show how this design will make life easier for every component developer. I might be kicking a dead horse here, but at least I can say I tried.

First lets start with the way we build components now. (admin area only)

1. Define your views.
2. Create 2 MVC triads for each view in the administrator area.
   (1 view with list and form = 2 controllers, 1 model, 2 views = 5 classes)
3. If you are using one model for both list and item views then you override JControllerAdmin::getModel for all your list view controllers
   4.Override the item view constructor and set $this->view_list to the plural form.
4. Override JModelAdmin::canDelete, JModelAdmin::canEditState to customize your ACL

Now lets look at the MVSC way

1. Define your views
2. Create Models and Views for each view in the administrator area
   ( 1 view with list and form = 0 controllers, 1 model, 1 view = 2 classes)
3. Override JModelCms::allowAction to customize your ACL

As you can see the MVSC way reduces the number of classes you need to build/maintain from 5 to 2

But the question of how to handle "Special edit tasks" on one view

Current Method:

1. Override JControllerForm::save which is exactly 241 lines long.

Of course since you had to create the controllers for every view regardless of customization, so overriding them isn't that big of a deal. However different overrides for these functions makes the overall behaviors inconsistent, because depending on the developer the override will be different.
Now if you need the same functionality in multiple views, then you would either need to create a parent class between JControllerForm and the controllers in question, or copy/paste each override into multiple controllers.

MVSC Method

1. Create a new task controller here we have two options
   a. extend JControllerUpdateEdit (best for overloading)
   b. extend JControllerUpdateBase (best for complete customization)
2. From here we have several options
   a. Override JControllerUpdateBase::commit and customize the part that works with the model
   b. Override JControllerUpdateBase::execute and customize there
   c. Overload JControllerUpdateBase::execute (this is good for custom redirects)

As you can see this isn't much difference between the current MVC and MVSC as far as how to create a custom edit for one view. Except there are more options with MVSC. The real power of this system comes in when we try an use the same functionality in multiple views. Because once we've created the custom task controller we can use it in any view by simply changing the task in our view. There is no need to create a parent class between JControllerForm and extend from it.

Also if a third party developer creates a controller that does something awesome, contributing it will be easier because it is independent of all the other controllers.

I guess that is the best argument I can give for this for the moment.

If anyone has any use cases that they don't think this will work for, then please add a comment explaining it. Explaining how it will make your life easier via examples will be far easier than explaining it off the top of my head.

[HermanPeeren]

Thank you very much Methew! I will study every letter you write and coded and I'm sure more people will seriously look at your work. I'm hitting a deadline (as too often) and unfortunately no time to react immediately. I wanted to let you know, so you know it is no disinterest from my part.

Your proposal is very interesting and and has many valuable aspects. I also appreciate it very much that you come with concrete code and examples, which also challenges others to come with concrete counterexamples when having other opinions, as I do have for some parts of your proposal. Together we will get on. Thanks again for code and explanation.

[beat]

Thanks Mathew!

As a general reply to your last comment: Less files and less (or, even better: no) code copy-pasting for extensions is great news and very valuable in terms of ease of development, maintainability and security.

Still need time to get my head into your doc and code proposals and give feedbacks.

[Mathewlenning]

@HermanPeeren
Thanks for the letting me know. I was a little worried that in my attempts to explain the concept, I alienated everyone from the conversation. I think 4 years of pushing through bureaucratic red tap in a extremely conservative small town in Japan has made me a little too aggressive when expressing my ideas.

On a side note, THANK YOU for giving me a reason to take another look at PHPStorm! After our conversation, I figured that a hundred bucks would be worth it for the diagramming alone. But after a few days with it, I cannot believe I was coding without it!

@beat
That is what I thought too. I've also been thinking about how it could be applied to the categories implementation. I'm going to try and build out a displayCategories controller, either this weekend or early next week. Since the importance of the "option" param is relative to the controller, I think we can initialize the categories model, push it into the category view , and render the page without the calling component knowing the difference.

We might even be able to actually use the 1 to many capability of our view class and push the calling components model into the category view too. I'll link here when I've validated/disproved this possibility.

[phproberto]

Hey @Mathewlenning we are trying to move things forward and make some of your proposals happen. Would you like to join us on a skype group to work on this? If so please add me to skype "phproberto"

Thanks!!

[HermanPeeren]

@phproberto Please add me to to a Skype group too, for I'm working on a better proposal (based on this and other proposals). Got some good ideas after talking to several people about Joomla's MVC on JAB. I also assume the Skype workgroup will be in cooperation with the GSOC-project for the new MVC.

[HermanPeeren]

Facebook is a big player in the PHP world. Hence for instance the PHP-compiler they are building (HHVM) and their static typed PHP-variant Hack. And they are refactoring their MVC too. Worth looking at these recent developments. Not in any way intended to derail this discussion, but as inspiration.

Video "Hacker Way: Rethinking Web App Development at Facebook", Published on 4 May 2014: https://www.youtube.com/watch?v=nYkdrAPrdcw
Discussion in "Facebook: MVC Does Not Scale, Use Flux Instead": http://www.infoq.com/news/2014/05/facebook-mvc-flux?utm_source=infoq&utm_medium=popular_links_homepage
I also saw some other discussions about this on mailinglists I follow. Sometimes hard to seperate the dogmatic, almost religious, points of view from rational arguments.

MVC is the basis of our system at Joomla, so we should take care not to just introduce something without considering the different architectural options we have. For me the starting point must be: what problem do we solve? What are the benefits and the costs of different solutions?

[HermanPeeren]

The two main benefits I'm working from for a newer MVC are:

• to make it easier to build RESTful webservices. Core of REST is the uniform interface, limiting the number of actions, for instance to 4, like in CRUD. The rest of the actions are then transformed to basic actions on other resources. For instance: a publish/unpublish action is then an update-action on a content.state resource. I think one-task controllers will only work well for a limited interface, otherwise you get an explosion of classes (and redundant code) as you can also see at Mathew's proposal.
• to make it easier to decouple core components (and optionally install them). The main starting point for making a new MVC was the tight coupling the old MVC had with several other parts of the CMS. That coupling (the "object oriented spaghetti code") is what makes it difficult to optionally install the core components.

I'd like to do this as much as possible in a backwards compatible way, so without changing any of the db and when a call to a component would change in any way then make an adapter to keep everything the same. Of course code must be DRY, so fallback principles like used by Mathew and in FOF should be applied: code that is not different from a default should not be coded.

[phproberto]

Good points @HermanPeeren. Can you add me to skype "phproberto" or give me your skype id to add you to the group?

[HermanPeeren]

My Skype-name: herman.peeren

[Mathewlenning]

@herman,

Thanks for posting those links. I was surprised because the MVSC really resembles flux conceptually.

You just have to think of the task controller as the action, the model as the store and well the view is the view.

The only partthat isn't implemented is the one cycle at a time imperative, but I do like the idea.

There is also the difference in that the view can manipulate the store(model) in MVSC, but I don't think anyone that has been doing this for a while uses more than the get functions in the view.

The only real way to prevent the view from changing the model, would be to take the model out.

However, I'm using the model as the hub for ACL, so having access to Model::allowAction in the view is a key point.

I've been building some components for my site over the last few weeks that use MVSC and the truth is I cannot imagine doing it any other way. 1 model running both list & form on both the front and back without hacking or overriding half the system.

The only part that I regret is the choice not to use singleton for the table. In retrospect this is one case that a singleton makes sense.

If the next JMVC can top this system, then I am positive that Joomla's troubles will be over.

Sincerely,
Mathew Lenning

Babel-university.com

P.S. This message was sent via iPhone, so please forgive any errors

On Jun 5, 2014, at 4:09 PM, HermanPeeren [email protected] wrote:

Facebook is a big player in the PHP world. Hence for instance the PHP-compiler they are building (HHVM) and their static typed PHP-variant Hack. And they are refactoring their MVC too. Worth looking at these recent developments. Not in any way intended to derail this discussion, but as inspiration.

Video "Hacker Way: Rethinking Web App Development at Facebook", Published on 4 May 2014: https://www.youtube.com/watch?v=nYkdrAPrdcw
Discussion in "Facebook: MVC Does Not Scale, Use Flux Instead": http://www.infoq.com/news/2014/05/facebook-mvc-flux?utm_source=infoq&utm_medium=popular_links_homepage
I also saw some other discussions about this on mailinglists I follow. Sometimes hard to seperate the dogmatic, almost religious, points of view from rational arguments.

MVC is the basis of our system at Joomla, so we should take care not to just introduce something without considering the different architectural options we have. For me the starting point must be: what problem do we solve? What are the benefits and the costs of different solutions?

�
Reply to this email directly or view it on GitHub.

[Mathewlenning]

@herman,

I'm looking forward to seeing your proposal.

@phproberto I added you this morning, let me know when the discussions start.

Sincerely,
Mathew Lenning

Babel-university.com

P.S. This message was sent via iPhone, so please forgive any errors

On Jun 5, 2014, at 4:52 PM, HermanPeeren [email protected] wrote:

The two main benefits I'm working from for a newer MVC are:

to make it easier to build RESTful webservices. Core of REST is the uniform interface, limiting the number of actions, for instance to 4, like in CRUD. The rest of the actions are then transformed to basic actions on other resources. For instance: a publish/unpublish action is then an update-action on a content.state resource. I think one task controllers will only work well for a limited interface, otherwise you get an explosion of classes (and redundant code) as you can also see at Mathew's proposal.
to make it easier to decouple core components (and optionally install them). The main starting point for making a new MVC was the tight coupling the old MVC had with several other parts of the CMS. That coupling (the "object oriented spaghetti code") is what makes it difficult to optionally install the core components. I'd like to do this as much as possible in a backwards compatible way, so without changing any of the db and when a call to a component would change in any way then make an adapter to keep everything the same.
�
Reply to this email directly or view it on GitHub.

[Mathewlenning]

I think we've all wasted enough time on this. Happy Joomla!ng