?

User tests: Successful: Unsuccessful:

avatar Mathewlenning
Mathewlenning
25 Apr 2014

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

avatar Mathewlenning Mathewlenning - open - 25 Apr 2014
avatar Mathewlenning
Mathewlenning - comment - 25 Apr 2014

@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?

avatar Mathewlenning Mathewlenning - change - 25 Apr 2014
The description was changed
Description <p>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. </p> <p>This is a big change compared to the old legacy system where extension developers inherited functionality. The key benefits of this design are:</p> <ol> <li>Reduction of duplicate code.</li> <li>Ability to add new tasks without worrying about BC breaks</li> <li>Clear separation of responsibility </li> <li>Cleaner code.</li> <li>Move business logic into the model </li> <li>One model can be used for list and form views</li> <li>No need to create empty controllers.</li> </ol><p>To understand how this system works please see the dispatcher class as it is the key to the entire system.<br> Extensions will have to create a controller that extends JCmsControllerDispatcher and then execute it in their extensionName.php entry point. </p> <p>I've also created model classes that support these new controllers. See <a href="https://github.com/Mathewlenning/joomla-cms/tree/CMS-Model-Classes">https://github.com/Mathewlenning/joomla-cms/tree/CMS-Model-Classes</a></p> <p>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. </p> <p>This is a big change compared to the old legacy system where extension developers inherited functionality. The key benefits of this design are:</p> <ol> <li>Reduction of duplicate code.</li> <li>Ability to add new tasks without worrying about BC breaks</li> <li>Clear separation of responsibility </li> <li>Cleaner code.</li> <li>Move business logic into the model </li> <li>One model can be used for list and form views</li> <li>No need to create empty controllers.</li> <li>100% backwards/forwards compatible </li> </ol><p>To understand how this system works please see the dispatcher class as it is the key to the entire system.<br> Extensions will have to create a controller that extends JCmsControllerDispatcher and then execute it in their extensionName.php entry point. </p> <p>I've also created model classes that support these new controllers. See <a href="https://github.com/Mathewlenning/joomla-cms/tree/CMS-Model-Classes">https://github.com/Mathewlenning/joomla-cms/tree/CMS-Model-Classes</a></p>
Labels Added: ? ?
avatar wilsonge
wilsonge - comment - 25 Apr 2014

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

avatar Mathewlenning
Mathewlenning - comment - 25 Apr 2014

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 notifications@github.com 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.

avatar wilsonge
wilsonge - comment - 25 Apr 2014

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)

avatar Bakual
Bakual - comment - 25 Apr 2014

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.

avatar wilsonge
wilsonge - comment - 25 Apr 2014

@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

avatar wilsonge
wilsonge - comment - 26 Apr 2014

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??

avatar Mathewlenning
Mathewlenning - comment - 26 Apr 2014

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 notifications@github.com 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.

avatar wilsonge
wilsonge - comment - 26 Apr 2014

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

avatar Mathewlenning
Mathewlenning - comment - 26 Apr 2014

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 notifications@github.com 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.

avatar mbabker
mbabker - comment - 26 Apr 2014

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 notifications@github.comwrote:

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 GitHub#3503 (comment)
.

avatar mbabker
mbabker - comment - 26 Apr 2014

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.

avatar Mathewlenning
Mathewlenning - comment - 26 Apr 2014

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 notifications@github.com 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.

avatar Bakual
Bakual - comment - 26 Apr 2014

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.

avatar beat
beat - comment - 26 Apr 2014

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.

avatar Mathewlenning
Mathewlenning - comment - 26 Apr 2014

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 notifications@github.com 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.

avatar Mathewlenning
Mathewlenning - comment - 26 Apr 2014

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 notifications@github.com 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.

avatar beat
beat - comment - 26 Apr 2014

@Mathewlenning

The current code has looots of copy-pastes, and makes it very hard to maintain as such (e.g. to change a single UI thing for the UX review in whole backend, it had to be done in each single view, which was a huge hurdle in coding and testing).

I'm not saying this one is worse. But it still could have much less duplicate lines of code. ;-)

I didn't say I don't like the approach of standardized re-usable controllers. But when there is more than a couple of lines that get copy-pasted in each controller instance, those could be factored either into the parent class if there is one, or in a helper class. That's valid if it makes sense. Otherwise it could be an indication of a possibility to generalize each code instance into a single one with dependency injection of the variable parts. In that sense, the Delegation, Dependency Injectiion and Observer software patterns are valid candidates for a review of the architecture.

E.g. it could be an indication that a single JControllerInterface could define the interface, then we could have a single main controller class (but that would not be mandatory as any other controller implementing the interface could act as such). That controller class could with the Observer pattern (used in JTable already) be extended to add all controller behaviors needed, one by one, and with only single implmentations that become isomorphic.

Hope that clarifies and gives a few ideas for possible further improvements (as this is a RFC).

Best Regards,
Beat

avatar Mathewlenning
Mathewlenning - comment - 26 Apr 2014

Thanks for clarifying your comment. Since these are the first run, refactoring is to be expected.

Could you point out the code you are referring to?

Also please don't being in helper classes, the goal here is to reduce dependencies not introduce new ones.

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:22 PM, beat notifications@github.com wrote:

@Mathewlenning

The current code has looots of copy-pastes, and makes it very hard to maintain as such (e.g. to change a single UI thing for the UX review in whole backend, it had to be done in each single view, which was a huge hurdle in coding and testing).

I'm not saying this one is worse. But it still could have much less duplicate lines of code. ;-)

I didn't say I don't like the approach of standardized re-usable controllers. But when there is more than a couple of lines that get copy-pasted in each controller instance, those could be factored either into the parent class if there is one, or in a helper class. That's valid if it makes sense. Otherwise it could be an indication of a possibility to generalize each code instance into a single one with dependency injection of the variable parts. In that sense, the Delegation, Dependency Injectiion and Observer software patterns are valid candidates for a review of the architecture.

E.g. it could be an indication that a single JControllerInterface could define the interface, then we could have a single main controller class (but that would not be mandatory as any other controller implementing the interface could act as such). That controller class could with the Observer pattern (used in JTable already) be extended to add all controller behaviors needed, one by one, and with only single implmentations that become isomorphic.

Hope that clarifies and gives a few ideas for possible further improvements (as this is a RFC).

Best Regards,
Beat

\
Reply to this email directly or view it on GitHub.

avatar phproberto
phproberto - comment - 26 Apr 2014

Thanks for your work on this.

I don't like the concept of 1 task controllers at all. About the benefits you stated:

1. Reduction of duplicate code.

It seems the opposite to me. Correct me if I'm wrong:

  • Currently extensions have to extend one controller and most of the stuff is done. You only have to override the methods you want or add new ones.
  • With this model now I have to create XX controllers for my extension to just override the base functions? So in fact you probably have more lines in your extension?

Maybe a cleanup / improvement of the current controller system (like is being done with JTable) could decrease the duplicated code better?

2. Ability to add new tasks without worrying about BC breaks

Adding functions into one class doesn't break BC

3. Clear separation of responsibility

Without an example of the final structure I would describe this as controller chaos where big extensions that currently have ~100 controllers like redSHOP can become a total mess of files.

4. Cleaner code.

This has nothing to do with the proposal or the concept of single task controller.

5. Move business logic into the model

Where mostly resides now.

6. One model can be used for list and form views

Like with current controllers.

7. No need to create empty controllers.

Can you provide an example of how this would be used?

8. 100% backwards/forwards compatible

Non-related to the concept of single controllers. I mean we can have a full backwards/forwards compatible system with multitask controllers.

Why haven't you used namespaces? I would expect it for a new controller system.

As reference for single task controllers the minimum objective folder structure should be something like what is currently used in the new tracker:

https://github.com/joomla/jissues/tree/master/src/App/Tracker/Controller

So you don't have a folder with all the controllers messed up. That's why a good example of how to use this proposed system would be desirable so we see the big picture and how it would avoid us to create empty controllers.

While I understand your idea of plugin a new controller system to the old MVC I think it's a mistake spend our efforts here. We should focus on a new controller system in a new MVC.

avatar Mathewlenning
Mathewlenning - comment - 26 Apr 2014

@Roberto,
I think you might have overlooked the key concept here. These controllers are not intended to be inherited from like we currently do with the legacy controllers. (With the exception of the abstract base and the dispatcher classes)

So your assumption regarding creating xx controller is indeed incorrect.

With these controllers you would remove ALL of the controllers except those built for unique functionality.

What I mean is, if you only need CRUD, publish states, and import/export then your component will work with only one controller. In the front end and one in the back end.

And since you haven't inherited any of these classes, so long as the methods they call in the model don't change, you won't have to refactor.

If you checkout dispatcher.php class you should be able to understand how it works. But If you still want a working example check out the com_helloworld component that I posted to the CMS thread. https://groups.google.com/forum/m/#!topic/joomla-dev-cms/a5nU71UKrw8

Just remember to install the Babelu_lib library, because the example was build for the controllers before I renamed them for this PR.

Also ignore all the magic in the views because that is just how my components are designed to work and have nothing to do with the controllers.

Sincerely,
Mathew Lenning

Babel-university.com

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

On Apr 26, 2014, at 6:56 PM, Roberto Segura notifications@github.com wrote:

Thanks for your work on this.

I don't like the concept of 1 task controllers at all. About the benefits you stated:

  1. Reduction of duplicate code. It seems the opposite to me. Correct me if I'm wrong:

Currently extensions have to extend one controller and most of the stuff is done. You only have to override just the methods you want.
With this model now I have to create XX controllers for my extension to just override the base functions? So in fact you probably have more lines in your extension?
Maybe a cleanup / improvement of the current controller system (like is being done with JTable) could decrease the duplicated code better?

  1. Ability to add new tasks without worrying about BC breaks
    Adding functions into one class doesn't break BC

  2. Clear separation of responsibility
    Without an example of the final structure I would describe this as controller chaos where big extensions that currently have ~100 controllers like redSHOP can become a total mess of files.

  3. Cleaner code.
    This has nothing to do with the proposal or the concept of single task controller.

  4. Move business logic into the model
    Where mostly resides now.

  5. One model can be used for list and form views
    Like with current controllers.

  6. No need to create empty controllers.
    Can you provide an example of how this would be used?

  7. 100% backwards/forwards compatible
    Non-related to the concept of single controllers. I mean we can have a full backwards/forwards compatible system with multitask controllers.

Why haven't you used namespaces? I would expect it for a new controller system.

As reference for single task controllers the minimum objective folder structure should be something like what is currently used in the new tracker:

https://github.com/joomla/jissues/tree/master/src/App/Tracker/Controller

So you don't have a folder with all the controllers messed up. That's why a good example of how to use this proposed system would be desirable so we see the big picture and how it would avoid us to create empty controllers.

While I understand your idea of plugin a new controller system to the old MVC I think it's a mistake spend our efforts here. We should focus on a new controller system in a new MVC.

\
Reply to this email directly or view it on GitHub.

avatar beat
beat - comment - 26 Apr 2014

@Mathewlenning i was refering to your code of this pull request:
https://github.com/joomla/joomla-cms/pull/3503/files

At least 50% of the lines of code are duplicates, with single trivial parameter variations. And 75% of phpdoc comments are duplicates too.

For me that is an indice of a software architecture issue. I'm not a fan of helper classes, but I am a fan of software architecture, decoupling, and to freedom given to the extensions implementers.

avatar Mathewlenning Mathewlenning - close - 26 Apr 2014
avatar Mathewlenning Mathewlenning - change - 26 Apr 2014
Status New Closed
Closed_Date 0000-00-00 00:00:00 2014-04-26 11:59:35
avatar Mathewlenning Mathewlenning - close - 26 Apr 2014
avatar Mathewlenning Mathewlenning - reopen - 26 Apr 2014
avatar Mathewlenning Mathewlenning - change - 26 Apr 2014
Status Closed New
avatar Mathewlenning Mathewlenning - reopen - 26 Apr 2014
avatar Mathewlenning
Mathewlenning - comment - 26 Apr 2014

O.k. so the close button is not to close the comment box. I think this is the second time I've done this when trying to comment.

avatar Mathewlenning
Mathewlenning - comment - 26 Apr 2014

I don't really think that anyone has fully grasped this concept yet. So first read cms/dispatch.php then come back to this comment.

In the administrator/components folder we have exactly 28 components with exactly 101 controllers.
This doesn't even take into account the front end. Assuming we built controllers for each of the actual feature that are used in them (this pr takes care of the major ones) we can turn 101 into 28 and those 28 controllers will be just empty decedents of the JControllerDispatcher class.

Say we have a one off task that doesn't fit in the core, o.k. build a controller in the component to handle that task. All of these tasks are independent of all others with the exception of inherited relationships which are mostly abstract classes.

What if we want to create a new feature? O.K. build a new controller and now every Joomla extension built from this day forward can use that feature out of the box. So long as they implement the required model interface. WHICH didn't exist before the feature was added, so there won't be any backwards compatibility issues to worry about.

There has been a lot of personal preference comments and I understand we are coders and we like it done the way we like to do it, but no one has even discussed the technical merit or lack there of. Please check out the working example I posted in the CMS google group. https://groups.google.com/forum/#!topic/joomla-dev-cms/a5nU71UKrw8

avatar Mathewlenning Mathewlenning - reference | - 26 Apr 14
avatar Mathewlenning
Mathewlenning - comment - 26 Apr 2014

Added Jcode tracker item 33660

avatar Mathewlenning
Mathewlenning - comment - 26 Apr 2014

Did a quick test to see how much of an impact this would have in the core components.

Method: Simply deleted all the com_componentName/controllers folders

The result was 110 files and 9,663 lines of code.

Of course there are probably a lot of tasks that were not accounted for in each of these components, so the results aren't factual, however they represent the potential value of these single task controllers.

avatar betweenbrain
betweenbrain - comment - 26 Apr 2014

Matthew,

Thanks for working so hard on this. I look forward to taking a look at it
and will do my best to carve out some time to do so as soon as I can.

Matt Thomas
203.632.9322
http://betweenbrain.com/

Sent from mobile. Please pardon any typos or brevity.
On Apr 26, 2014 8:58 AM, "Mathew Lenning" notifications@github.com wrote:

I don't really think that anyone has fully grasped this concept yet. So
first read cms/dispatch.php then come back to this comment.

In the administrator/components folder we have exactly 28 components with
exactly 101 controllers.
This doesn't even take into account the front end. Assuming we built
controllers for each of the actual feature that are used in them (this pr
takes care of the major ones) we can turn 101 into 28 and those 28
controllers will be just empty decedents of the JControllerDispatcher class.

Say we have a one off task that doesn't fit in the core, o.k. build a
controller in the component to handle that task. All of these tasks are
independent of all others with the exception of inherited relationships
which are mostly abstract classes.

What if we want to create a new feature? O.K. build a new controller and
now every Joomla extension built from this day forward can use that feature
out of the box. So long as they implement the required model interface.
WHICH didn't exist before the feature was added, so there won't be any
backwards compatibility issues to worry about.

There has been a lot of personal preference comments and I understand we
are coders and we like it done the way we like to do it, but no one has
even discussed the technical merit or lack there of. Please check out the
working example I posted in the CMS google group.
https://groups.google.com/forum/#!topic/joomla-dev-cms/a5nU71UKrw8


Reply to this email directly or view it on GitHub#3503 (comment)
.

avatar beat
beat - comment - 26 Apr 2014

@Mathewlenning Thanks for the new explanations and your hard work in doing and explaining. It is not easy to understand code based on github diffs, as only diffs are visible, and the big picture is hidden. This makes us tend to focus on implementation details to understand architecture, instead of the other way around.

Less code for the same functionality should always be welcome. :+1:

Your diff was only adding code (+1826), not removing (-0 in the stats). So at first glance I hope you understand that it was hard to grasp that :wink:

Also I now understand that this requires a change in Models too to work properly.

Maybe an architectural design doc could help us understand ?

Finally, instead of pointing to a google group, can you point us to (github) code for the use exemple ?

avatar Mathewlenning
Mathewlenning - comment - 27 Apr 2014

@beat,
I realize now that looking at one part of this design isn't really enough to communicate it's significants.

So I'm going to convert the com_weblinks component over to use the system.

You are correct that changes have to be made to the model, but for the most part it's just adding the interfaces expected by the controllers (although unintentional most of the method names don't exist in the legacy models)

There also has to be some changes in JTable so it consistently throws exceptions when thing go wrong, but again with the exception of the delete method, most of the methods required don't exist.

Technically the table doesn't HAVE to change, but that means moving the responsibility for table errors into the model, which forces the model to know more about the table than it should.

I'll post the repo with the converted com_weblinks when there is some actual code to present.

Sincerely,
Mathew Lenning

Babel-university.com

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

On Apr 27, 2014, at 2:21 AM, beat notifications@github.com wrote:

@Mathewlenning Thanks for the new explanations and your hard work in doing and explaining. It is not easy to understand code based on github diffs, as only diffs are visible, and the big picture is hidden. This makes us tend to focus on implementation details to understand architecture, instead of the other way around.

Less code for the same functionality should always be welcome.

Your diff was only adding code (+1826), not removing (-0 in the stats). So at first glance I hope you understand that it was hard to grasp that

Also I now understand that this requires a change in Models too to work properly.

Maybe an architectural design doc could help us understand ?

Finally, instead of pointing to a google group, can you point us to (github) code for the use exemple ?

\
Reply to this email directly or view it on GitHub.

avatar wilsonge
wilsonge - comment - 27 Apr 2014

So bad news is we're not allowed to convert JError to exceptions straight off for b/c reasons. Anyone on the legacy MVC is going to expect a JError and when they don't get that and find an exception it all goes wrong :P So unfortunately moving JTable to use exceptions in some kinda b/c way is gonna be a pretty tricky process :/

avatar Mathewlenning
Mathewlenning - comment - 27 Apr 2014

The simple solution was to extend the table and build out an interface that does throw errors.

This isn't a BC issue because no one is using JTableCms and if they do they'll learn to expect errors.

I'm not sure if I've mentioned this, but all these controllers, their models and the view work in J3 and J2 without actually changing any of the core classes.

Sincerely,
Mathew Lenning

Babel-university.com

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

On Apr 27, 2014, at 5:40 PM, George Wilson notifications@github.com wrote:

So bad news is we're not allowed to convert JError to exceptions straight off for b/c reasons. Anyone on the legacy MVC is going to expect a JError and when they don't get that and find an exception it all goes wrong :P So unfortunately moving JTable to use exceptions in some kinda b/c way is gonna be a pretty tricky process :/

\
Reply to this email directly or view it on GitHub.

avatar Bakual
Bakual - comment - 27 Apr 2014

Just wondering: How are you handling cases where an edit task for one view needs different code in the controller than for another view? Like for example I have a different ACL I need to check.
Currently it's easy, I just add what is needed to the controller for the view.

Generally speaking I tend to think the same as Roberto. Most of this can be achieved as well with multi task controllers. It just needs improvement in the current JController classes. The new MVC would be a perfect time to create better controllers as one doesn't have to think that much about B/C there.
I don't see the big advantage of having a controller for each task yet which couldn't be done with a multitask controller already. It looks more like just another approach to solve the same problem. However I see a disadvantage for the extension developer because they need to learn a new way of doing the same things they did before. And usually, adoption will be slow for new things. Especially if there is not a big gain.

avatar Mathewlenning
Mathewlenning - comment - 27 Apr 2014

For your edge case there are several options available using this system.

  1. Create a custom edit controller in your component and add the extra checks there. This is similar to what we do now, but instead of one override per view, you build one and only one.

  2. Override the JModel::allowAction and put your checks here. This is the recommended method. The benefit here is that if you use this function when you do your checks in your view, then you have 0 access check duplication in any of your MVC classes.

Honestly the reason you don't see the benefit is because you still haven't grasped the principal concept here. That isn't your fault, I obviously haven't communicate it's significance accurately.

However the argument that we should stick to the way we've been doing it, because some people don't want to learn a better way, just isn't acceptable to me.

We are developers! Our mission is to change the world one line of code at a time.

Sincerely,
Mathew Lenning

Babel-university.com

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

On Apr 27, 2014, at 8:57 PM, Thomas Hunziker notifications@github.com wrote:

Just wondering: How are you handling cases where an edit task for one view needs different code in the controller than for another view? Like for example I have a different ACL I need to check.
Currently it's easy, I just add what is needed to the controller for the view.

Generally speaking I tend to think the same as Roberto. Most of this can be achieved as well with multi task controllers. It just needs improvement in the current JController classes. The new MVC would be a perfect time to create better controllers as one doesn't have to think that much about B/C there.
I don't see the big advantage of having a controller for each task yet which couldn't be done with a multitask controller already. It looks more like just another approach to solve the same problem. However I see a disadvantage for the extension developer because they need to learn a new way of doing the same things they did before. And usually, adoption will be slow for new things. Especially if there is not a big gain.

\
Reply to this email directly or view it on GitHub.

avatar Bakual
Bakual - comment - 27 Apr 2014
  1. Create a custom edit controller in your component and add the extra checks there. This is similar to what we do now, but instead of one override per view, you build one and only one.

That wouldn't work if I only need the special code for one view, right? Since the tasks are shared for all views (unlike what we have currently).

Honestly the reason you don't see the benefit is because you still haven't grasped the principal concept here.

That may well be. But I wouldn't communicate it as such to someone else.

However the argument that we should stick to the way we've been doing it, because some people don't want to learn a better way, just isn't acceptable to me.

It's of course subjective if it's better or not. :smiley:

avatar Mathewlenning
Mathewlenning - comment - 27 Apr 2014

You are correct that if you wanted to apply the additional access checks to one view only then #1 wouldn't be the best option (although it wasn't the best option of the two given to begin with) However a simple if($config['subject'] == 'view with special rules') would work.

On the other hand if you choose the recommended method #2 then you wouldn't even need the conditional.

And yes better is subjective. I agree. To me better is reducing the code base by 9000+ lines, 100% consistent behavior across all core components, removing the need to create a bunch of empty classes to do basic CRUD, as well as making it easier to learn, build and maintain Joomla components.

You've already decided that single task controllers can't be better than what we have now. I understand why you feel that way, I've seen some of the past attempts that people have made to apply this idea.

However if after I've finished converting weblinks, you can't see how this will make your life easier, then you still have the option to build the better multi-task controllers you envision and present them to the community.

Sincerely,
Mathew Lenning

Babel-university.com

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

On Apr 27, 2014, at 10:16 PM, Thomas Hunziker notifications@github.com wrote:

Create a custom edit controller in your component and add the extra checks there. This is similar to what we do now, but instead of one override per view, you build one and only one.
That wouldn't work if I only need the special code for one view, right? Since the tasks are shared for all views (unlike what we have currently).

Honestly the reason you don't see the benefit is because you still haven't grasped the principal concept here.

That may well be. But I wouldn't communicate it as such to someone else.

However the argument that we should stick to the way we've been doing it, because some people don't want to learn a better way, just isn't acceptable to me.

It's of course subjective if it's better or not.

\
Reply to this email directly or view it on GitHub.

avatar Mathewlenning
Mathewlenning - comment - 28 Apr 2014

Making progress! I won't be able to work on the example tomorrow, but I have 30th, 1st, and 2nd to finish up.

avatar Mathewlenning
Mathewlenning - comment - 2 May 2014

I've finished the project proposal for what I'm calling MVSC design. Its on google drive https://drive.google.com/file/d/0B7NSGTBMVDlFaEV4SG00Z3lLUTg/edit?usp=sharing

avatar Mathewlenning
Mathewlenning - comment - 5 May 2014

Does anyone know why Travis is throwing this error?

Fatal error: Using $this when not in object context in phar:///home/travis/.phpenv/versions/5.4.27/bin/phpunit/phpunit-mock-objects/Framework/MockObject/Generator.php(300) : eval()'d code on line 91

avatar dongilbert
dongilbert - comment - 5 May 2014

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.

avatar wilsonge
wilsonge - comment - 5 May 2014

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

avatar Mathewlenning
Mathewlenning - comment - 5 May 2014

@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.

avatar Mathewlenning
Mathewlenning - comment - 6 May 2014

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

avatar Mathewlenning
Mathewlenning - comment - 7 May 2014

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.
5. 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.

avatar HermanPeeren
HermanPeeren - comment - 7 May 2014

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.

avatar beat
beat - comment - 7 May 2014

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.

avatar Mathewlenning
Mathewlenning - comment - 8 May 2014

@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.

avatar Mathewlenning Mathewlenning - reference | - 9 May 14
avatar Mathewlenning Mathewlenning - reference | - 9 May 14
avatar phproberto
phproberto - comment - 4 Jun 2014

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!!

avatar HermanPeeren
HermanPeeren - comment - 5 Jun 2014

@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.

avatar HermanPeeren
HermanPeeren - comment - 5 Jun 2014

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?

avatar HermanPeeren
HermanPeeren - comment - 5 Jun 2014

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.

avatar phproberto
phproberto - comment - 5 Jun 2014

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

avatar HermanPeeren
HermanPeeren - comment - 5 Jun 2014

My Skype-name: herman.peeren

avatar Mathewlenning
Mathewlenning - comment - 5 Jun 2014

@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 notifications@github.com 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.

avatar Mathewlenning
Mathewlenning - comment - 5 Jun 2014

@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 notifications@github.com 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.

avatar Mathewlenning Mathewlenning - change - 2 Jul 2014
Status New Closed
Closed_Date 2014-04-26 11:59:35 2014-07-02 03:59:59
avatar Mathewlenning Mathewlenning - close - 2 Jul 2014
avatar Mathewlenning Mathewlenning - close - 2 Jul 2014
avatar Mathewlenning
Mathewlenning - comment - 2 Jul 2014

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

Add a Comment

Login with GitHub to post a comment