[4.0] Making CategoryNode and MenuItem ImmutableNodes #23452
Conversation
Optimising CategoryNode, MenuItem Improving menu item discovery in routing
|
FYI: If we had running unittests, they would fail because of backwards incompatible changes here. They aren't major changes, but still the behavior is slightly different. |
# Conflicts: # libraries/src/Tree/NodeTrait.php
Fixing bug in NodeInterface/NodeTrait
|
I changed the backend menu to use the MenuItem class and re-ordered a lot of code as well. This makes about a 1000 lines of code obsolete... These changes had 3 goals:
I succeeded with 1. and as you can see reduced the code quite a bit, but it still isn't easy to understand. sigh The current implementation works in a way that is very strange to me... I don't even really know how to explain how it works. I felt uncomfortable working with first flat lists, then converting those into a pseudo-tree, then converting that again into a "real" tree, but being unable to access that without going through the Tree class first... The new implementation now works with nodes of type MenuItem right from the start and then modifies and iterates the whole tree afterwards. The code still is pretty complex, but I hope that I reduced the complexity by a degree. Maybe from a 10 to a 9? I also moved code from the Joomla\CMS\Menu\MenuHelper to the MenusHelper of com_menus, because we are already using code from that class in that whole system and I couldn't see a reason for having some code in the component and other code in the framework. Since this is highly application specific code, I decided to move this entirely to the components MenusHelper and to drop the framework class. This reduces the rendering of the backend menu from 10 files down to 3. One detail that I feel a bit uncomfortable with is removing the Joomla\CMS\Menu\Node* classes. The backend menu has special classes for each node type which contain some special handling and which I would consider a rather better implementation than what is in this PR. But I decided against those node-type-classes since it brings the backend menu closer to a copy of the frontend menu. At the same time, there is so much special logic in the code to render the menu, that a little bit more to acommodate for this wont change the overall impression of this noticeably. The part of creating your own backend menu has to be tested again, but shouldn't be far off from what we expect. @wilsonge I hope that raises the chance to merge this. If you want to know a different reason to make MenuItem implement the NodeInterface: With this change, you could dynamically extend the menu system during runtime. You could generate menu items from categories and insert them wherever you want by doing addChild(), etc. and it would automatically propagate through routing and menu displays without storing this in the DB. It would make the menu more flexible. ;-) |
|
|
||
| // Call preprocess for the menu items on plugins. | ||
| // Plugins should normally process the current level only unless their logic needs deep levels too. | ||
| $this->application->triggerEvent('onPreprocessMenuItems', array('com_menus.administrator.module', &$items, $this->params, $this->enabled)); | ||
| $this->application->triggerEvent('onPreprocessMenuItems', array('com_menus.administrator.module', $children, $this->params, $this->enabled)); |
There was a problem hiding this comment.
Is this going to cause a b/c break on existing plugins triggering on this? Just for the purposes of documenting it if it is
There was a problem hiding this comment.
yes and no. It principally is the same event with the same data, but the actual menu item objects are now different to the current ones. It depends on the plugin if it needs updates, since there is no submenu attribute anymore, but the methods to traverse the tree.
There was a problem hiding this comment.
OK. But it will need a few lines of docs to be safe. Honestly I have no clue if this is used but always better safe than sorry
There was a problem hiding this comment.
I hope that comment explains this enough?
|
Deprecation notices are added in #23842 |
|
I have tested this item ✅ successfully on cf51ece The amount of categories was not sufficient to say anything about performance. This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/23452. |
|
Thanks for being patient with this! |
In #23364 we added a node interface for object nodes and in this PR I'm making CategoryNode and MenuItem implement those interfaces. That should then make it possible to drop
\Joomla\CMS\Menu\Nodeand we can maybe replace some nasty stuff where we build menus or similar stuff with convoluted code with proper tree traversal instead.I added a trait for such node systems and changed CategoryNode and MenuItem to use those and a few fixes and improvements. I then also changed the menu item discovery of routing. This should improve the parseing quite a bit, since we don't have to run through all menu items.
I also fixed a few docblocks...
Looking forward to your feedback. Next thing would be to change mod_menu to use such a tree iterator to render the menu instead of what we have right now.
Testinstructions
The idea is, that this does not change anything visibly for the user. Take your testsite, preferably with some deeply nested menu items and lots of menu items. Check that the links all work and maybe how long it takes for Joomla to render a page. Apply this patch and see that the URLs stay the same and still all work and hopefully also see a slight performance improvement for page rendering by Joomla. The performance improvement should only be noticeable for really large sites with several thousand menu items.