Add custom menu API

Test added to patch.

If we explicitly only use title, description and menu_name, why not use menu_custom_save($menu_name, $title, $description = '') ?

10x Berdir, here's the patch according to your comments.

looks better now :)

:)

Fix typo.

The tests should use random string/name.

What's the advantage of a random string? Seems equivalent to me.

Test is now with random strings.

Looks great.

chx in IRC asked if we really need to have this function - maybe just a drupal_write_record() is ok. I think that maybe it's better to have this API function for DX. Any thoughts?

For me the function name doesn't sound right. menu_save_item, menu_set_item maybe?

We have menu_link_save() for custom menu links, this is for top level menus.

I'm pretty sure I've seen people ask how to make top level menus before, it'd be nice for them to find (or be directed) to an API function even if it's as light as this one.

chx asked for my opinion on this but my battery is dying, will check in later

(translation: "subscribe")

The last submitted patch failed testing.

re-roll.

The last submitted patch failed testing.

hmm, I still need to learn how to patch with git... I hope Testbot will like this one.

For API consistency, we should also have a menu_custom_delete() function. Also, this API function should be in menu.module, the module that allows custom menus, not in menu.inc.

This API would be a great addition, to be used in install profiles, ... etc., and for a better developer experience. Tagging accordingly.

I will take care of it.

Added the following changes to the patch at #17.

- menu_custom_save() moved from menu.inc to menu.module, the module that allows custom menus.
- added menu_custom_delete() to delete custom menu and menu links associated.
- menu_custom_save() should receive a $custom_menu object, for consistency with others API in core.
- Used menu_custom_save() in menu.install, and toolbar.install instead of manually executing queries.
- Used menu_custom_delete() in the menu delete form submit handlers in menu.admin.inc.

Looks great. Have been creating some menu and items. Didn't find any other referentes to menu_custom either.

1. I do not like when a function parameter type is not specified exactly. Would it be a problem to allow only array type $custom_menu? As I see it is used as array everywhere.
2. I do not see any return statement in this function.

@Pasqualle, thanks for the review.

1. I do not like when a function parameter type is not specified exactly. Would it be a problem to allow only array type $custom_menu? As I see it is used as array everywhere.

An object is being used for consistency with other module_*_save() API functions in core. IMO, it's PHP, we do not need strict type checking.

2. I do not see any return statement in this function.

No, no return statement , removed the comment.

That's right, we do not need strict type checking, as it is PHP, but it would be nice to have. So when I see a variable named $custom_menu then I would know what type should I expect.
As I see (http://api.drupal.org/api/search/7/_save) there is only 1 *_save() function (file_save) where different variable types are allowed, and I would rather fix that also than creating a new one..

the file_save() issue #561520: $file is an object

Well, I suggest to leave it as an object, and do the typecasting so that we don't have to change everywhere an array is used. Then, we can convert the other structures to stdClass if you prefer.
4 days to code freeze!!!

I do not know which issue are you referencing to. The $custom_menu is an array everywhere, so the function parameter should be an array. The $file is an object everywhere so the function parameter should be an object. I do not understand what else do you want to change..
I am happy with array and object variables as function parameters, I just want the type to be specified, as it is specified (mostly?) everywhere in Drupal.

In #2 Berdir asked
> why not use menu_custom_save($menu_name, $title, $description = '') ?

Which in terms of DX I think is better in this situation.

Nit pick - Two dots(..)

Also, how about adding a test for the menu delete? (I don't have a dev enviorement on this computer so can;t re-roll myself...)

@Amitaibu: only one variable should be used for the item to be inserted into db. that is the common format of *_save functions..

For consistency with other *_save() API functions in core, we should use an object as the parameter to that function.
It's easy to read, can be modified inside the function and the changes persists without requiring to be passed by reference, etc...

- Removed the trailing dot in the docs.

changed the menu_custom_save() function to accept array type parameter only. $menu and $custom_menu is used as array, so the function parameter should be an array. There are many *_save() functions with array parameters, so the function is consistent with the rest of core..

moved menu_custom_save() and menu_custom_delete() into menu.inc as these functions should be accessible even the menu module is disabled.

Custom menus are handled by menu.module, which in fact, creates the table {menu_custom}. So, those API functions should live in menu.module, not in menu.inc. If the menu.module is not installed, you probably don't need to use those functions.

Note, for example, that toolbar.module has menu.module as a dependency.

About the parameter as array or as object, I do not think that is something worth further discussion, whatever the decision, for me is fine. The most important is to get this API in before the freeze.

you are right, the table is created with menu.module therefore the functions should be in menu module.

but we should decide which form should be used:

vs

I think 99% of Drupal sites have the menu module enabled, so moving the creation of menu_custom table to system.install is still an option, to make the code cleaner.. If it is too much here, then a new issue should be created, and menu_save() and menu_delete() should be moved back to menu.module for this patch..

I think 99% of Drupal sites have the menu module enabled, so moving the creation of menu_custom table to system.install is still an option, to make the code cleaner..

No. On the contrary, each module should be responsible of installing it's own tables and stuff, handling it's updates, etc... I don't want that table created if I don't 'want' menu.module enabled in my application/site/install profile, etc... Well, that's not the point of that issue.

@Pasqualle, can we mark one the patchs in this issue as RTBC ? Code freeze is here ;). It's a valuable addition, and it's is a really simple patch. So, let's go on.

as I see #31 is the closest to RTBC, I will just move the functions back to menu.module..

I think it's ready to go.

- Updated the tests to check that all menu links associated to a menu were removed from database when then menu is deleted.

I've added a test to the custom_menu_delete()

Ok, all test passed. I think it is RTBC.

hmm, it didn't upload my patch.

* Test from #40 was already covered.
* Fixed a query placeholder (menu_name >> :menu_name)
* Fixed a typo in the toolbar.install (Admininstration >> Administration).

This looks sensible - it should indeed be in menu module, and an API for this is one of those things that we just didn't have time for in D6.

The last submitted patch failed testing.

HEAD is broken.

If those are the only keys, I think I'd prefer to enumerate the parameters ($name, $title, $description). That'd be more user-friendly for people using IDEs.

Also, I'd prefer names like menu_save_custom_menu() and menu_delete_custom_menu() since they're more descriptive.

That function name is confusing; it makes it sound like you're adding your own custom menu API. :P Maybe add a Via or With or something in there to make it more clear?

Why 16 and not a constant like you have with MENU_MAX_MENU_NAME_LENGTH_UI?

I'm on crack. Are you, too?

Patch addresses #46 comments.

> Why 16 and not a constant like you have with MENU_MAX_MENU_NAME_LENGTH_UI?

I kept it as is, to follow addCustomMenu() which uses '16'. I went over quickly on other tests and saw the use of explicit numbers (testPagePreviewWithRevisions() in node.test for example).

Should we not call the functions menu_custom_menu_save()? Generally we put the verb last like, node_save(), menu_link_save(), etc.

see above - let's be more consistent in terms of funciton naming. Otherwise this is essentially done

@webchick: That's inconsistent.

I agree with pwolanin in #48. We always use the pattern [object]_[action]() for CRUD functions in core.

We also use single arguments for CRUD functions throughout core. Only the loader function usually takes an ID instead of an object.

And we already have http://api.drupal.org/api/search/7/menu_load --

Hence, I don't see why these couldn't be simply named menu_save() and menu_delete() ? Only the last one needs a special validation to not delete system menus.

Anything else would be a DrupalWTF.

@sun - the table is {menu_custom} and there MUST be a significant distinction made between what these functions are operating on and hook_menu, hook_menu_alter, which are operating on the router.

so, I'd suggest going back to menu_custom_save() or menu_custom_menu_save(), or something like that.

Alright, so our [object] is 'menu_custom'. ++consistency

Here we go.

I think we should use API function menu_link_delete here, mainly because it invokes hook_menu_link_delete(), but this function also clear caches, update parental status for menu links, etc... so that may be an expensive operation. Thoughts ?

aha! I think that's the same mess that's happening in http://api.drupal.org/api/function/menu_delete_menu_confirm_submit/7 before the menu is deleted? ;)

Doesn't seem so. However, we could load all items before deleting them and invoke hook_menu_link_delete() manually.

We should additionally clear the caches in the API function to make it consistent with other CRUD functions.

Interesting: http://api.drupal.org/api/function/menu_delete_menu_confirm_submit/7 and http://api.drupal.org/api/function/_menu_delete_item/7 invoke different functions to clear the caches.

Did Rickard's hook get in to act each time we delete a menu link?

If the cache is already clear, additional calls to clear it shoudl be relatively fast right? Deleting should be a very rare operation.

Since we need to fetch the list of items anyway, we could simply set $item['plid'] = 0 and $item['has_children'] = FALSE before invoking _menu_delete_item(), so we skip the expensive and senseless operations here:

- Re-parenting of children

- _menu_update_parental_status() [which somehow looks very similar to _menu_delete_item()'s re-parenting, just acting on a different column... but anyway]

I also see that both cache clearing functions invoked by _menu_delete_item() take special care for multiple invocations:

http://api.drupal.org/api/function/menu_cache_clear/7
http://api.drupal.org/api/function/_menu_clear_page_cache/7

Which means that we actually do not have to clear the cache on our own, since _menu_delete_item() will clear all caches in case the menu contained some links.

The last submitted patch failed testing.

Disabled slave #44, since it returned strange results for a couple of patches.

Anyone up for approving this patch?

Does it make sense to use the API function during the install hook?

Of course! It's the main reason why we want to do this after all: Stop writing raw stuff into the database and use proper API functions instead.

Instead of questioning this, we should move forward, and provide and use more API functions in installers/uninstallers and installation profiles. Text formats, filters, user roles, user permissions, and with this patch also custom menus can finally all be handled with proper functions instead of raw database queries.

The _delete function doxygen is incomplete - it should make very clear that it both deletes the custom menu AND all menu links associated with that custom menu.

Clarified PHPDoc - although we aren't so verbose in other API functions that delete user data...

Needed re-roll due to #508570: Restore URL consistency for node types and menus anyway... :)

The last submitted patch failed testing.

Well, we don't necessarily delete lots of other data do we? e.g. if I delete a user, I think their content is may be retained.

Fixing the tests as well. :)

Subscribing. This is so very handy for some contrib modules (devel, admin_menu, etc).

Pretty sure we don't document @return if there is no return value.

This review is powered by Dreditor.

Removed.

For whom is concerned: #516254: Remove @return PHPDocs where nothing is return

Looks good, bot likes and lots of concern has gone into this.

node_save()
user_save()
comment_save()
menu_custom_save()

"One of these things is not like the other...."

I don't like "custom" being in the function name:

1) These are not for just "custom" menus, but also system-defined menus such as Navigation, Management, etc.
2) As a developer, the function I'm going to search api.drupal.org for is "menu_save", for compatibility with our other CRUD API function names. When I don't find it, I'm going to resort to db_insert() and db_delete() like our .profile file currently does.
3) Our functions map to the data concept, not to the table name. It's not users_save() even though the table is called users. It's not node_revisions_save(). Etc.

If anything, I'd rename the menu_custom table to menu if we really care about the discrepancy. But the number of people with innate knowledge of Drupal's DB internals vs. those searching through api.drupal.org is probably a 1:10000000 ratio, so I'm not that fussed about it.

First of all, straight re-roll due to conflicts.

I agree with webchick, and I already wondered when I saw the naming request above (which didn't contain any reasoning).

I double-checked that these function names do not clash with other functions.

chx additionally asked in IRC, why we don't follow the usual hook_*_insert() and hook_*_update() pattern here. I replied that we couldn't, because we don't know.

But sleeping over that question helped.

Given this patch, we might even remove that man-made limitation in a follow-up issue.

Applied the patch, played around in the menu administration, added some menu items, deleted others. Looked at the code and really liked how clean it made the code. The thing I like most about it is how it moves the Block Menu handling code from the Menu module into the Block module. I'm inclined to set this to RTBC, but chx said he wanted to discuss some stuff. So, in the mean time, I'll just subscribe.

chx requested to squeeze a @todo into menu_save() to remind us to replace that 'old_name' condition with the proper db_merge() result, which should tell us whether the query inserted or updated something, which is currently not the case.

So this patch should be ready to fly.

If we're renaming an existing menu, won't this create a new record instead of updating the old one? Shouldn't the merge be on $menu['old_name']? Do we test when this happens?

I'm on crack. Are you, too?

@DaveReid:

What's unclear here?

@tha_sun: Listen up kids, don't drink and review!

My only concern is taken care of, code is good, love the APIs, test bot likes...RTBC'd!

Good stuff. Committed to HEAD!

This needs to be documented in the upgrade guide.

It seems I can't edit the convert 6 to 7 page, so here's it is. Someone with better English should go over it :)

---

In Drupal 6 in order to create a custom menu (i.e. a menu you would create via admin/build/menu/add) you had to query the {menu_custom} table yourself.

In D7 several API functions to create, update and delete custom menus and related hooks were added.

The last submitted patch failed testing.

[EDIT: This has become obsolete — skip to #473082-105: Add custom menu API]

Has anyone actually run this?

This crashes because $link is an object. The attached patch fixes it.

@salvis: That's not going to work because _menu_delete_item() expects an array.

@Dave Reid: Look at the code of _menu_delete_item(). I did run it...

Well we could avoid the additional array -> object processing if we just fetch objects. For large menus maybe it would make a difference.

Yes, for those sites that keep deleting menus day in and day out over and over again. ;-)

I actually tried PDO::FETCH_ASSOC, but I prefer nice and clean code.

There you go.

Now we need tests.

The last submitted patch failed testing.

@sun:

What you meant was

but as I said, I prefer #87. It's easier to get right, easier to read, it builds on the chosen default behavior of db_query() and thus helps to reinforce the fact that we get back objects by default.

Adding obscure flags in obscure places keeps us from developing a natural code flow, as you've demonstrated very nicely.

Setting back to needs review for #87.

@salvis

While your code might look cleaner, there was a decision to use arrays inside menu.inc/menu.module. Not very concistent with the rest of core, I know, but now is probably not the right time to change that.

By all means, PDO fetch modes are not obscure at all. You are just not used to them - but you definitely will in D7. We use them in many more places throughout core.

Whether ->fetch() or using third argument to db_query() doesn't matter for me. Maybe Berdir can clarify about which of both "should rather" be used. :)

@sun: You didn't get it to work in two attempts. That tells me it's too difficult to get right. Can't you see that?

menu links and router items should always be arrays - this was a D6 core design decision. you can argue if it's wrong, but then you need to change all of the rest of core to match.

Ok, if that's how we want to have it...

Oops, sorry, wrong file...

looks fine

Let's get a test for this.

The last submitted patch failed testing.

Committing #511286: D7UX: Implement customizable shortcuts fixed the bug I reported in #87 above — thanks.

I'm resetting the flags to what they were in #87 (#473082-85: Add custom menu API) above, except NR would fall back to NW right away. That's where we stand now.

@Amitaibu: Can you pick it up again, please?

I either need to get rights to add the docs + code example from #85, or someone with those rights should do it.

Can anyone grant me access to add the docs from #85?

I just fixed the permissions again on http://drupal.org/update/modules/6/7, you should be good to go. Thanks, Amitaibu!

Added docs -- http://drupal.org/update/modules/6/7#custom_menu_api

Added coder issue -- http://drupal.org/node/654364

Automatically closed -- issue fixed for 2 weeks with no activity.