Drupal's menu module is currently doing rather poorly, as far as separation between UI and API logic goes. Throughout the module, there are examples of database queries, form handling, and presentational display all being tangled together in a single function.

Some examples:

Example 1: menu_overview_tree() - the logic for finding the top-level menus (i.e. the 'menus' as distinct from the 'menu items') is inseparable from the presentation logic for outputting the table of menus and menu items. Is currently:

foreach ($menu['items'][0]['children'] as $mid) {

Should be something like this:

foreach (menu_get_root_menus($menu) as $mid) {

This way, other modules can work out what the 'root menus' are without duplicating the code.

Example 2: menu_edit_item_save() - database queries are plonked right in with the parsing of form values, and the outputting of user feedback messages. The queries should be moved to a separate function, e.g. menu_save_menu_item(), so that they can be run without necessarily being fed form values. User feedback messages should also be moved to a separate function, as not all modules that play with the menu system may wish to display them.

Example 3: menu_edit_item() - the query for selecting a particular menu item's details is mixed right in with the form handling system for the menu item editing interface. Is currently:

$item = db_fetch_object(db_query('SELECT * FROM {menu} WHERE mid = %d', $mid));

Should be something like this:

$item = menu_get_menu_item($mid);

This way, other modules can grab a menu item straight from the database.

I personally need this cleaning-up to happen for menu.module, as it will provide an improved menu API that I can call in my new category module, and in one of its extension modules, category_menu. The menu API available through the menu.inc functions and through hook_menu() is powerful, but does not extend to allowing other modules to directly manage menu items in the database. My module will need to do this, as only the menu table (in the DB) allows you to specify an arbitrary parent for a particular menu item - hook_menu() simply calculates the parent-child hierarchy based on the URLs.

I think that when the menu module functions are separated into API and UI code, the API code should remain in menu.module, rather than moving to menu.inc. This is because menu.inc functions (such as _menu_build()) are designed to completely ignore the menu table if menu.module is disabled. Therefore, any functions that deal with the menu table should be in menu.module itself. This will allow menu.inc to remain focused on hook_menu() operations, and to keep all DB-related menu functionality separate (this is how it's currently designed, and I think this design is very good).

I'm going to start coding this soon, but in the meantime, thoughts and suggestions are most welcome.

CommentFileSizeAuthor
#13 menu_db_api_5.patch22.09 KBRichard Archer
#10 menu_db_api_4.patch21.42 KBJaza
#9 menu_db_api_3.patch20.84 KBJaza
#7 menu_db_api_2.patch20.78 KBJaza
#6 menu_db_api_1.patch20.79 KBJaza
#4 menu_db_api_0.patch17.68 KBJaza
#2 menu_db_api.patch17.64 KBJaza

Comments

Richard Archer’s picture

Richard Archer commented

Watch out when you are making changes to menu.inc. The menu system is a major resource hog and any changes which exacerbate this problem will have a hard time being accepted.

I wonder what menu_edit_item is doing making database queries anyway? Why not pull that info out of $menu. Probably because there's no good mechanism in the API to retrieve a single menu item :)

The changes you have in mind sound good. I am keen to maintain back-compatibility as much as possible, so try to keep the existing API in place and just factor out the logical chunks from each function as needed.

I look forward to seeing some patches!

Jaza’s picture

Jaza commented
Status: Active » Needs review
StatusFileSize
new menu_db_api.patch17.64 KB

OK, the patch is ready. This patch introduces 3 new functions to the menu API:

1. menu_get_item() - Retrieves the menu item specified by $mid, or by $path if $mid is not given.
2. menu_save_item() - Save a menu item to the database.
3. menu_delete_item() - Delete a menu item from the database.

Note: the existing menu_delete_item() function (which is also a menu callback) has been renamed to menu_edit_item_delete(). I didn't see anything that calls this function, apart from the callback in hook_menu(), which has been updated. The new name is more consistent with the naming of menu_edit_item_save(), and allows the name menu_delete_item() to be available for the new function that is focused solely on this task.

The menu_get_item() function has been introduced into menu.inc, while menu_save_item() and menu_delete_item() have been put into menu.module. This is because the former deals with the menu in memory - as accessed through menu_get_menu() - while the latter deals directly with the database, through SQL queries.

I have tried to replace code with menu_get_item(), wherever menu_get_menu() is being called, and only one menu item (or sometimes two) is actually being accessed from it. So in many places, something like this:

$menu = menu_get_menu();
... $menu['items'][$mid]['pid'] ...

Has become:

$item = menu_get_item($mid);
... $item['pid'] ...

This makes the code more readable, and hides the full $menu tree from many parts of the code.

menu_save_item() has replaced all INSERT and UPDATE queries on the menu table, and menu_delete_item() has replaced most DELETE queries on the menu table.

These functions were originally written for the category_menu module (part of the category module package - http://drupal.org/node/39683), and can currently be found in category_menu.module with the following equivalent names:

category_menu_api_get_item()
category_menu_api_save_item()
category_menu_api_delete_item()

They have been designed to work with category_menu, with menu.module and menu.inc, and hopefully with any other module that needs to directly access menu items and perform queries on the menu table. If and when this patch gets committed, the functions in category_menu will be removed, and category_menu will instead call the 'official' menu API functions.

Richard Archer’s picture

Richard Archer commented

Nice work!

Here are a few things I picked up on reading through the patch:

  • I don't much like the while ($mid && ($item = menu_get_item($mid)) && ($item['type'] & MENU_IS_LOCAL_TASK)) {. I prefer code to be easier to understand than that, even at the expense of a couple of extra lines.
  • There is chunk of code removed from menu_rebuild which checks for $new_mid == 1. Are you sure it's a good idea to remove this?
  • Should menu_save_item check to see if the item already exists? It might be best to centralise this test rather than having every calling function do it. That said, menu_edit_item_form doesn't seem to check for duplicates.
Jaza’s picture

Jaza commented
StatusFileSize
new menu_db_api_0.patch17.68 KB

New patch fixes up the following things:

1. Ugly boolean logic in 

while ($mid && ($item = menu_get_item($mid)) && ($item['type'] & MENU_IS_LOCAL_TASK)) {

has been cleaned up. The multi-faceted while statement has been reduced, and the extra conditions are now inside the loops as if statements, which will break if the condition is not met. Personally, I prefer long while statements to using the break mechanism, but I couldn't think of any other alternative, and I guess the latter is more readable.

2. menu_save_item() now checks - using menu_get_item() - to see if the item specified by $item['mid'] already exists. If it does, then a new menu ID is generated, and the INSERT query is run instead of the UPDATE query. I can't imagine that this check will fail very often, if ever. But it's there now, just in case (perhaps admin x loads a menu item, then admin y deletes that item, then admin x tries to save it? this would be a valid use case, and one that the new check accommodates very well).

As for the "chunk of code removed from menu_rebuild which checks for $new_mid == 1", this code is redundant and not needed. In fact, it wasn't needed before this patch either. The includes/database.[mysql/pgsql] scripts insert a value of 2 (my) or 3 (pg) into the sequences table, meaning that 'navigation' (mid 1) and 'primary links' (mid 2) will never be troubled with an override attempt.

Richard Archer’s picture

Richard Archer commented

I have back-tracked the $new_mid == 1 code and as far as I can see the problem it addresses still exists. If Drupal is installed with a prefix for database table names the database setup script inserts the wrong table name into the sequence table. See: http://drupal.org/node/11449

But after the primary links patch the sequence is now initialized to 2 instead of 1, so this work-around was broken anyway.

I think for now this test should remain, but in a slightly different format:

       // Check explicitly for mid 1. If the database was improperly prefixed,
       // this would cause a nasty infinite loop.
       // TODO: have automatic prefixing through an installer to prevent this.
       do {
         $new_mid = db_next_id('{menu}_mid');
       }
       while ($new_mid <= 2)

I'll re-open the previous issue and try to come up with a better solution.

Jaza’s picture

Jaza commented
StatusFileSize
new menu_db_api_1.patch20.79 KB

Old patch no longer applies - this new one has been updated to work with latest HEAD.

Also, I've added a new function to the API, called menu_get_root_menus(), which returns an array with the mid and the title of all 'root menus'. If you look at the top of this thread, you'll see that this was actually the first example I gave of a function that could improve the menu system API - but I forgot to actually put it in until now! ;-)

In menu.module, all spots where the code iterates through $menu['items'][0]['children'] have been changed to use menu_get_root_menus(), which significantly improves readability and code re-use in these spots, and perhaps also improves performance (just a guess, not benchmarked).

As for the test that checks for an mid of 1 or 2, it's not really possible to keep this test in its old spot, as this patch relies on menu_save_item() to generate a new mid, and that call happens further down, by which time the test becomes redundant. The only practical possibility would be to put the test within menu_save_item() itself, which personally I am loath to do (in English: I think it would be a bad idea). Really, IMO, the problem lies with the database prefixing script. Database prefixing is a feature for advanced users anyway, and so they should be able to manually fix their install script before running it; that is, until the script is fixed. I say let's remove this cruft from menu.inc, and focus on where the problem really is.

Jaza’s picture

Jaza commented
StatusFileSize
new menu_db_api_2.patch20.78 KB

Once again updated for latest HEAD.

dries’s picture

dries commented

I'm not opposed to this patch but I leave it up to Richard to decide/review/test.

Jaza’s picture

Jaza commented
StatusFileSize
new menu_db_api_3.patch20.84 KB

Updated for latest HEAD. More to follow...

Jaza’s picture

Jaza commented
StatusFileSize
new menu_db_api_4.patch21.42 KB

This new patch significantly improves performance, by implementing a static variable cache of the menu tree within menu_get_item(). I was hoping that this wouldn't be needed, as menu_get_menu() maintains its own cache in the form of the global $_menu variable. But Richard ran some benchmarks (the results of which he sent to me), which clearly show that without the static cache that it has now, menu_get_item() increases the execution time of some dependent functions by almost double.

Richard's benchmark results for menu_get_active_nontask_item (called 100 times):

Original method: 0.0145 seconds
New method (without cache): 0.0245 seconds
New method (with cache): 0.0175 seconds

And for menu_item_link (also called 100 times):

Original method: 0.066 seconds
New method (without cache): 0.061 seconds
New method (with cache): 0.057 seconds

This patch also (at Richard's request) re-implements the check for an mid of 1 or 2 in menu_save_item(). This part of the patch is based on Richard's patch at http://drupal.org/node/41811.

Jaza’s picture

Jaza commented

Should also mention that the new static variable cache in menu_get_item() gets reset whenever menu_rebuild() is called. The optional $reset parameter in menu_get_item() is used by menu_rebuild(), and shouldn't need to be used by any other calling function.

dries’s picture

dries commented

I'm OK with these improvements but leave it up to Richard to change the status field to 'ready to be committed'.
Glad to see you're putting some effort in keeping an eye on the performance implications. Rock on guys!

Richard Archer’s picture

Richard Archer commented
StatusFileSize
new menu_db_api_5.patch22.09 KB

I really like this patch. It vastly improves the menu system API without actually creating back-compatibility problems. I think this patch creates a much better framework for further development of the menu system.

I benchmarked a couple of areas where the new patch might have an impact on performance, and found the differences to be quite minor. In some places the added overhead from the menu_get_item calls slow things down by 5% -- and there are a few places where menu_get_menu is still called to fetch the whole tree because menu_get_item would need to be called too many times. In other places the reduced overhead of duplicating $_menu results in a performance increase. Overall I would expect this patch to be performance-neutral.

I have thoroughly reviewed the patch and I can't see any problems. I expect that in a patch of this size there will be something wrong, but it all looks quite robust to me. I would like to get this into HEAD ASAP so at least a few people have a bash at it before beta3 is released.

I have made a couple of little changes in this version, nothing major. Jaza, could you have a look at this current version and mark this 'ready to be comitted' if it looks OK.

Jaza’s picture

Jaza commented
Status: Needs review » Reviewed & tested by the community

Thanks for the quick review and update, Richard! I just had a look at your patch - the changes are indeed fairly minor. I applied the patch to latest CVS, and tested it: all menu administration functions seem to work, and other menu functionality (e.g. active items, breadcrumbs) is all working fine.

I think this patch is ready to go!

dries’s picture

dries commented

Great job. Seems to work for me as well. Committed to HEAD. Thanks guys! Looking forward to future improvements. :)

Richard Archer’s picture

Richard Archer commented
Status: Reviewed & tested by the community » Fixed

Thanks for your excellent work on this patch Jaza. It's a big improvement!

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)