User tests: Successful: Unsuccessful:
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.
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.
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.
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.
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.
Just tested and it installs great. It even seems faster, is that true?
Thanks Don!
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.
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)
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:
Helpers
folder to Helper
to be consistent with the rest of foldersredSHOP
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.
@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.
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
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)
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.
Status | New | ⇒ | Pending |
Build | ⇒ | . |
@dongilbert can you fix the merge conflicts? and sync/change this PR to staging?
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2015-03-14 00:37:44 |
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 theInstallDatabase_backup
task (same forInstallDatabase_remove
). Otherwise, I had originally gone with non-underscored names here.