? ? Error

User tests: Successful: Unsuccessful:

avatar dongilbert
dongilbert
8 Oct 2013

This is something I've been wanting to do, to show as an example how to properly use namespaces within a Joomla! application. I figured the installer would be a good candidate, since it's never used by the end user except for the one time, but developers could still inspect the code closely, since it ships with every download of the CMS.

A big thanks goes out to @mbabker for this as well. Earlier this year (?) he converted the installer to the new MVC, which made the transition to the namespaces that much easier.

avatar dongilbert dongilbert - open - 8 Oct 2013
avatar mbabker
mbabker - comment - 8 Oct 2013

I was asked to keep the task names as is because of the language keys in the install app. INSTL_INSTALLING_DATABASE_BACKUP is the key for the InstallDatabase_backup task (same for InstallDatabase_remove). Otherwise, I had originally gone with non-underscored names here.

avatar dongilbert
dongilbert - comment - 8 Oct 2013

I don't see how the language key names effect the JS task names, unless of course we're generating messages based on the task name. Do you know if that's the case? If so, we can easily create an array mapping the task name to the language key name.

IMO, language keys shouldn't effect software architecture, especially when said architecture is meant to be a top-notch example of code quality and usage.

avatar mbabker
mbabker - comment - 8 Oct 2013

It was a request from JM to keep the translation teams from having to redo keys just for that request. I found it easier to just rename the classes than to try and work around mapping it separately and made PHPCS ignore PEAR Valid Class Name check for those classes. So, architecturally, renaming them is the right thing to do, just wasn't a task I wanted to take up at the time or a battle worth having.

avatar dongilbert
dongilbert - comment - 8 Oct 2013

I don't see how that's a valid argument though. The keys ought to be underscore separated tasks, breaking at the CameCase of the class name. He doesn't have to redo the translation for that.

Worse comes to worse, I'll put in the extra effort, because this is supposed to be a good reference example.

avatar mbabker
mbabker - comment - 8 Oct 2013

Never said it was valid, just what was the case. The language key is built based on how the task is defined in that stage of the installer (in reality, I count 3 different places that task is actually used; a CSS ID, the language key, and the task that's sent in the AJAX request). Changing it for one means you change a few lines in different places.

avatar betweenbrain
betweenbrain - comment - 10 Oct 2013

Just tested and it installs great. It even seems faster, is that true?

Thanks Don!

avatar dongilbert
dongilbert - comment - 11 Oct 2013

I've done some updates, and cleaned up the code a bit more, reducing code duplication between the Installation\Application\WebApplication and the JApplicationCms classes. This should speed up the application a bit, since it's having to do less work all around.

avatar wilsonge
wilsonge - comment - 12 Oct 2013

Simple install works fine. Got to get my file/folder permissions on linux fixed before I can test the extra languages step of the install process - but hopefully will do that tomorrow :) (worst case will do it on windows)

avatar phproberto
phproberto - comment - 12 Oct 2013

Great job @dongilbert !

When trying to install languages I get the tinyMCE error and after it a Layout Path Not Found. Not sure if that's unrelated. In the standard Joomla! the language gets installed even with the error.

A fast look at the code makes me love it but:

  • I would rename Helpers folder to Helper to be consistent with the rest of folders
  • I still don't like the 1 task controllers. A small application like the installer already has 20 controllers. Imagine what would happen for something like redSHOP that has around 100 multitask controllers. Do we really want to have 300 - 400 controllers?

Here for example we have (easilly identifically and groupable by the Install prefix):

  • InstallConfigController
  • InstallDatabaseBackupController
  • InstallDatabaseController
  • InstallDatabaseRemoveController
  • InstallEmailController
  • InstallLanguagesController
  • InstallSampleController

One single Install controller will save us files and avoid that Install******* prefix.

This is a fast view. I haven't played enough with it so correct me if I'm wrong.

avatar elinw
elinw - comment - 13 Oct 2013

@phproberto As discussed before one task controllers are not (unless you want them to be) one method controllers they are one request controllers, they are really a great way to go especially in a context like this. What makes sense is to consider what is really a complete task i.e. database handling does not need to be broken up since there is no reason to make separate requests for each. Just set the options and let the application deal with it. No reason at all why they can't be completely parameterized.

avatar elinw
elinw - comment - 13 Oct 2013

I believe that cms controllers sometimes have a protected controller prefix property that is used as part of the key for messages. Perhaps something like that is the thing to do here? https://github.com/joomla/joomla-cms/blob/master/administrator/components/com_banners/controllers/banner.php#L25

avatar dongilbert
dongilbert - comment - 13 Oct 2013

I architecturally despise the one task controllers, but it's what we have currently. If there's support for it, I'll refactor to a more traditional approach, since the point of this exercise is to show best practice and example. 


Sent from Mailbox for iPhone

On Sun, Oct 13, 2013 at 9:12 AM, elinw notifications@github.com wrote:

I believe that cms controllers sometimes have a protected controller prefix property that is used as part of the key for messages. Perhaps something like that is the thing to do here? https://github.com/joomla/joomla-cms/blob/master/administrator/components/com_banners/controllers/banner.php#L25

Reply to this email directly or view it on GitHub:
#2170 (comment)

avatar dongilbert
dongilbert - comment - 14 Oct 2013

After looking at it a bit, it's hard to not do the one-task-per-controller architecture with the current routing and JController interface, because the interface enforces the paradigm. Following Elin's advice though, it would certainly be possible to have multiple-method controllers to handle the actual tasks, but at that point, your getting outside of the purpose of the previous architecture decisions that formed the JController interface. So, we're stuck between a rock and a hard place. If we stick with the current JController then we need to stick with one task per controller. There is, however, a benefit of all those controllers, and it is that it reinforces the "Single Responsibility Principle" - each class is enforced to have only a single responsibility.

avatar brianteeman brianteeman - change - 21 Aug 2014
Status New Pending
Build .
avatar zero-24
zero-24 - comment - 27 Dec 2014

@dongilbert can you fix the merge conflicts? and sync/change this PR to staging?

avatar wilsonge wilsonge - change - 14 Mar 2015
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2015-03-14 00:37:44
avatar wilsonge wilsonge - close - 14 Mar 2015

Add a Comment

Login with GitHub to post a comment