Update libraries/joomla/application/menu.php #700
Conversation
|
Build triggered by changes to the base. Unit testing complete. There were 0 failures and 0 errors from 1971 tests and 11145 assertions. |
|
|
Current code forcefully include the file for JMenuSite class. It should check if class is already loaded or not, this way it provide greater flexibility to change the functionality without hacking the core code. Same work is done for all helper classes e.g. JModuleHelper. How can I see checkstyle issue ? Thanks |
| // Create a JPathway object | ||
| $classname = 'JMenu' . ucfirst($client); | ||
|
|
||
| //only load if not already loaded |
There was a problem hiding this comment.
Please change this to
// Only load if not already loaded.
Not taht this is not one f the check style errors.
|
Build triggered by changes to the head. Unit testing complete. There were 0 failures and 0 errors from 1971 tests and 11145 assertions. |
| // Create a JPathway object | ||
| $classname = 'JMenu' . ucfirst($client); | ||
|
|
There was a problem hiding this comment.
there are tabs at end of line
|
But why are you overloading the application's menu class? |
|
This patch makes it possible to override JMenuClient by adding a check in JMenu to first see if the application menu class is already loaded before including the core file. |
|
Build triggered by changes to the head. Unit testing complete. There were 0 failures and 0 errors from 1971 tests and 11146 assertions. |
|
So why are we overloading the application's menu class? |
|
Why? |
|
for : Adding access control OR extra functionality OR modified functionality, which does not necessarily relates to Joomla core. |
|
So like if you were creating a new application entirely? |
|
Or to add a real menu to the Administrator. There are lots of reasons developers override core classes. In fact, in Louis and Mark's new developer book, there is a whole chapter devoted to how to do so. Shyam - thanks for taking time to patch this. Sam - if you were creating a new application, then you would also be creating the child class for the menu and you wouldn't have to override it in that case. |
|
So where is the need or are we just doing it for kicks? |
|
Its for controlling menus for existing application. Core code should be flexible enough, so components/plugins can control application-wide menus. For more general sense of requirement of overriding, please refer article from @AmyStephen http://www.alltogetherasawhole.org/profiles/blogs/overriding-joomla-16-framework |
|
So you've got a replacement JMenuAdministrator or JMenuSite you can share? |
|
e.g. in one of our component we do it to check if user have access to particular menu as per his subscription. public function load()
{
// Initialise variables.
$db = JFactory::getDbo();
$app = JFactory::getApplication();
$query = $db->getQuery(true);
$query->select('m.id, m.menutype, m.title, m.alias, m.path AS route, m.link, m.type, m.level, m.language');
$query->select('m.browserNav, m.access, m.params, m.home, m.img, m.template_style_id, m.component_id, m.parent_id');
$query->select('e.element as component');
$query->from('#__menu AS m');
$query->leftJoin('#__extensions AS e ON m.component_id = e.extension_id');
$query->where('m.published = 1');
$query->where('m.parent_id > 0');
$query->where('m.client_id = 0');
$query->order('m.lft');
// Set the query
$db->setQuery($query);
if (!($this->_items = $db->loadObjectList('id'))) {
JError::raiseWarning(500, JText::sprintf('JERROR_LOADING_MENUS', $db->getErrorMsg()));
return false;
}
// ====== Code added PayPlans : START =====
$userType = XiFactory::getApplication();
if($userType->isSite())
{
//trigger event for controlling menus
$menus = $this->_items;
$args = array(&$menus);
PayplansHelperEvent::trigger('onPayplansMenusLoad', $args);
$this->_items = $menus;
}
// ====== Code added PayPlans : END =====
foreach($this->_items as &$item) {
// Get parent information.
$parent_tree = array();
if (isset($this->_items[$item->parent_id])) {
$parent_tree = $this->_items[$item->parent_id]->tree;
}
// Create tree.
$parent_tree[] = $item->id;
$item->tree = $parent_tree;
// Create the query array.
$url = str_replace('index.php?', '', $item->link);
$url = str_replace('&', '&', $url);
parse_str($url, $item->query);
}
} |
|
So this isn't something you feel could be added to JSiteMenu as a simple trigger for your event? Like JApplication::triggerEvent('menuLoad') ? |
|
Any trigger must be added in CMS, and when CMS community need it to be very much required. But as we are working on framework/platform, we must empower CMS or CMS-extensions to override framework-behavior in case required. And importantly when we say we are introducing true and full autoloading in J 2.5 , then we must consider that JPlatform is also autoloading. For autoloading we must not hardcode the inclusion of files and its path. I hope my comment make sense when we wish framework will empower other cms then joomla cms. |
|
The CMS doesn't need to override itself because it is itself. It doesn't make sense to provide the CMS a way to better override functionality it already provides. It doesn't necessarily make sense to provide a mechanism to override this when a better alternative could be arrived at using a different mehcanism. The example you provide is to replace an entire class to add an event trigger. Why not just add the event trigger? You're replacing a 140 line file to add 9 lines of code. Not to mention the unrelated functions that you don't touch. All of this just to trigger an event - so if you need such an event then you put it into the right place. Replacing entire classes isn't the only way to provide extensibility. I'd suggest in this particular case it's best to use our plugin system to build extensibility. As an aside it seems weird to override JSiteMenu and then check if JApplication::isSite is true. |
|
Sam - Why do you see this as a problem? The patch is backwards compatible and if it results in more flexibility. I'm really not sure why you see this as anything other than a bug fix. If it's not allowed, developers are still able to override JMenu, make this fix, and then override the application class. Isn't Joomla designed to be capable of overriding? Louis and Mark's book has a chapter devoted to it entitled "Using Plugins to Override Core Classes" - http://my.safaribooksonline.com/book/web-development/joomla/9780132780841/extending-joomla-with-plugins/157#X2ludGVybmFsX0ZsYXNoUmVhZGVyP3htbGlkPTk3ODAxMzI3ODA4NDEvMTcx I am not saying I am right on this, but I am confused by your response. Is there a standard for when a core class should be overrideable? Or, is the design intended to allow overriding anything? Is this include statement intentionally written to error out? It might help if you were able to share some of the criteria used to make these calls and the underlying thinking the project has on overriding core functionality, when it is to be allowed and when it should be discouraged. Thanks. |
|
Because it isn't a bug fix, it's a new feature. What I'm asking is if there a better way of doing this instead of replacing an entire class and for the example given it makes sense to add a plugin trigger instead which means more than one person can extend the functionality instead of whomever gets there first. To me it feels like multiple plugins could potentially wish to add, alter or remove menu items based. This seems to be the underlying feature being requested. The implementation of a plugin trigger provides even more flexibility than simply providing a mechanism to override the class. |
|
To phrase the situation slightly differently, the request is to add the ability to override a class so that the class can be overridden to provide the ability to add a plugin trigger. So why not just add the plugin trigger? |
|
Sam - A plugin trigger is not going to replace the static query results provided by the client menu class that are then used in the application class for ACL considerations and active menu decisions and within the menu module. It would not change the current menu used in the template, and so forth and so on. By preventing this very simple override, you are not stopping this, but forcing the override of additional core classes or preventing the flexibility a developer is hoping to introduce and making Joomla less viable for their use. Preventing this might be valid, I don't know. It depends on what the PLT is hoping to accomplish with the architecture. It would be helpful if the PLT adopted a policy on overrides, whether the ability is to do so is considered to be important to the value of the architecture, or in what cases doing so should be discouraged. Sharing that understanding of how the PLT sees this issue would help build a common understanding of what constitutes a bug fix (restoring preferred behavior) or a could pose a "threat" to the architecture (and, in those cases, would be deemed harmful.) There is no reason for me to respond further on this so I thank you for considering my comments. |
|
I haven't prevented anything, I'm just trying to understand things. Hence why I asked "why" after every time I got "what". Anyway, I'll wait for ssv445 to answer if a simple plugin trigger would indeed solve the problem that he's encountering. Also just realised that you can swap the include_once to an include if you're checking if the class is defined or not (presumably if the file is already included then the class would be defined). |
|
Sam - Changing the include to an include_once would still result in an error as it would not be including the same file, it is defining the same class. That's the resulting error and the proposed patch is the appropriate fix. |
|
I agree with the technical aspects of @pasamio comment, that we should add trigger, but triggers should responsibility of CMS rather then platform. Platform alternation should not be dependent on any trigger to be triggered in CMS. Can you share a link where I can see the vision of JPlatform, so that we can justify if the patch is inline with vision statement. |
|
@AmyStephen you misunderstand my post, I'm suggesting with the change we don't need the include_once as presumably if the class has already been defined either someone else did it or that file was loaded. @ssv445 I think the view is if you're using the platform directly then you're less likely to actually be doing the sort of things you're suggesting need to be done. But now you mention that you raise an interesting point: an argument could be made that the menu system is also CMS specific and should actually be shifted to the Joomla! CMS folder. That's actually not a bad idea that you raise there. |
|
I'm sorry, Sam, do you mean because of the autoloading the entire statement can be removed? I'm still not certain how the autoloading works, so that certainly could be possible. Agree that the menu belongs to the CMS. Thanks for hearing me. |
|
No I mean that it doesn't need to be an include_once any more, just an include because presumably if the file is loaded the class is defined and if the class isn't defined then presumably the file isn't loaded so we can eliminate the extra check that include_once will do. |
|
So, are you saying if this pull request is allowed - and the IF statement is then put in place - that the include_once could be changed into an include? If so, yes, fine, okay, makes sense. |
|
@pasamio as per your suggestion - removed include_once Yes, menu are integral part of CMS, but can we implement routing in Platform without menus ?
|
|
Menus as they are defined now are an integral part of the Joomla! CMS, I would suggest that if you were planning on building a new Joomla! Platform powered CMS that the last mistake you would make was the re-use of the existing menu system (and maybe not the routing system either). If you have a look at JRoute/JRouter you will notice that they themselves do not call a dependency upon the menu themselves. JRouteSite is in fact the beast that invokes the menu system into action from the application. Using the existing menu system would encourage the proliferation of the Itemid which I feel has been universally agreed to be one of the weakest aspects of our SEF implementation as it easily provides for the ability to get to a given content item multiple different ways causing us issues when considering SEO and Google which now penalises for duplicate content (at one point the ability to create duplicate content in different parts of the site was viewed a feature to enable end users to be able to discover a given piece of content in multiple pathways). So to answer the question, the menu is really a construct of how the CMS works at the moment and I'd almost suggest should be hidden lest we forget the mistakes we made and redo Itemid all over again. |
|
Build triggered by changes to the head. Unit testing complete. There were 0 failures and 0 errors from 1971 tests and 11146 assertions. |
|
While I agree with @pasamio 's points, the problem is that it is completely off-topic and, while I am certain it wasn't Sam's intention to derail an issue that was important to a community developer, unfortunately, that seems to be what happened. @ssv445 - I agree with your bug fix and would have included it, right away. But, you do have other options without hacking core. You can override JMenu, put in your bug fix, and then load your own JSiteMenu. It's not ideal, but it will work. |
|
@AmyStephen yes, currently we do override JMenu in joomla 1.7. Thanks for supporting the issue. |
Update libraries/joomla/application/menu.php
|
I agree with Sam, it's not worth worrying about, and I agree with Amy, it's not worth worrying about. The file should move to the CMS anyway, so I'm fine with it not being my problem anymore :) |
This will allow developers to override the menu class.