Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
While working on an hook_update_N() I needed to pragmatically add a new custom menu. This patch allows doing it using an API.
Comment | File | Size | Author |
---|---|---|---|
#113 | drupal.menu-custom-dbmerge.patch | 1.89 KB | sun |
#87 | menu_delete.patch | 687 bytes | salvis |
#101 | menu_delete.473082.100.patch | 882 bytes | salvis |
#100 | theme_username.565792.44.patch | 1.97 KB | salvis |
#94 | menu_delete.patch | 687 bytes | salvis |
Comments
Comment #1
amitaibuTest added to patch.
Comment #2
BerdirIf we explicitly only use title, description and menu_name, why not use menu_custom_save($menu_name, $title, $description = '') ?
Comment #3
amitaibu10x Berdir, here's the patch according to your comments.
Comment #4
Berdirlooks better now :)
:)
Comment #5
amitaibuFix typo.
Comment #6
chx CreditAttribution: chx commentedThe tests should use random string/name.
Comment #7
moshe weitzman CreditAttribution: moshe weitzman commentedWhat's the advantage of a random string? Seems equivalent to me.
Comment #8
amitaibuTest is now with random strings.
Comment #9
catchLooks great.
Comment #10
amitaibuchx 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?
Comment #11
tic2000 CreditAttribution: tic2000 commentedFor me the function name doesn't sound right. menu_save_item, menu_set_item maybe?
Comment #12
catchWe 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.
Comment #13
bjaspan CreditAttribution: bjaspan commentedchx asked for my opinion on this but my battery is dying, will check in later
(translation: "subscribe")
Comment #15
amitaibure-roll.
Comment #17
amitaibuhmm, I still need to learn how to patch with git... I hope Testbot will like this one.
Comment #18
dropcube CreditAttribution: dropcube commentedFor 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.
Comment #19
dropcube CreditAttribution: dropcube commentedI will take care of it.
Comment #20
dropcube CreditAttribution: dropcube commentedAdded 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
, andtoolbar.install
instead of manually executing queries.- Used menu_custom_delete() in the menu delete form submit handlers in menu.admin.inc.
Comment #21
davyvdb CreditAttribution: davyvdb commentedLooks great. Have been creating some menu and items. Didn't find any other referentes to menu_custom either.
Comment #22
Pasqualle1. 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.
Comment #23
dropcube CreditAttribution: dropcube commented@Pasqualle, thanks for the review.
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.
No, no return statement , removed the comment.
Comment #24
PasqualleThat'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..
Comment #25
Pasquallethe file_save() issue #561520: $file is an object
Comment #26
dropcube CreditAttribution: dropcube commentedWell, 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!!!
Comment #27
PasqualleI 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.
Comment #28
amitaibuIn #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...)
Comment #29
Pasqualle@Amitaibu: only one variable should be used for the item to be inserted into db. that is the common format of *_save functions..
Comment #30
dropcube CreditAttribution: dropcube commentedFor 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.
Comment #31
Pasquallechanged 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.
Comment #32
dropcube CreditAttribution: dropcube commentedCustom 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.
Comment #33
Pasqualleyou 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..
Comment #34
dropcube CreditAttribution: dropcube commentedNo. 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.
Comment #35
Pasqualleas I see #31 is the closest to RTBC, I will just move the functions back to menu.module..
Comment #36
PasqualleComment #37
dropcube CreditAttribution: dropcube commentedI 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.
Comment #38
amitaibuI've added a test to the custom_menu_delete()
Comment #39
dropcube CreditAttribution: dropcube commentedOk, all test passed. I think it is RTBC.
Comment #40
amitaibuhmm, it didn't upload my patch.
Comment #41
amitaibu* Test from #40 was already covered.
* Fixed a query placeholder (menu_name >> :menu_name)
* Fixed a typo in the toolbar.install (Admininstration >> Administration).
Comment #42
dropcube CreditAttribution: dropcube commentedComment #43
pwolanin CreditAttribution: pwolanin commentedThis 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.
Comment #45
lilou CreditAttribution: lilou commentedHEAD is broken.
Comment #46
webchickIf 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?
Comment #47
amitaibuPatch 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).
Comment #48
pwolanin CreditAttribution: pwolanin commentedShould we not call the functions menu_custom_menu_save()? Generally we put the verb last like, node_save(), menu_link_save(), etc.
Comment #49
pwolanin CreditAttribution: pwolanin commentedsee above - let's be more consistent in terms of funciton naming. Otherwise this is essentially done
Comment #50
sun@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.
Comment #51
pwolanin CreditAttribution: pwolanin commented@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.
Comment #52
sunComment #53
sunAlright, so our [object] is 'menu_custom'. ++consistency
Here we go.
Comment #54
dropcube CreditAttribution: dropcube commentedI 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 ?
Comment #55
sunaha! 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? ;)
Comment #56
sunDoesn'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.
Comment #57
pwolanin CreditAttribution: pwolanin commentedDid 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.
Comment #58
sunSince 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.
Comment #60
sunDisabled slave #44, since it returned strange results for a couple of patches.
Comment #61
sunComment #62
sunAnyone up for approving this patch?
Comment #63
pwolanin CreditAttribution: pwolanin commentedDoes it make sense to use the API function during the install hook?
Comment #64
sunOf 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.
Comment #65
pwolanin CreditAttribution: pwolanin commentedThe _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.
Comment #66
sunClarified 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... :)
Comment #68
pwolanin CreditAttribution: pwolanin commentedWell, 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.
Comment #69
sunFixing the tests as well. :)
Comment #70
Dave ReidSubscribing. This is so very handy for some contrib modules (devel, admin_menu, etc).
Comment #71
Dave ReidPretty sure we don't document @return if there is no return value.
This review is powered by Dreditor.
Comment #72
Dave ReidComment #73
sunRemoved.
For whom is concerned: #516254: Remove @return PHPDocs where nothing is returned
Comment #74
Dave ReidLooks good, bot likes and lots of concern has gone into this.
Comment #75
webchicknode_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.
Comment #76
sunFirst of all, straight re-roll due to conflicts.
Comment #77
sunI 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.
Comment #78
sunchx 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.
Comment #79
RobLoachApplied 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.
Comment #80
sunchx 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.
Comment #81
Dave ReidIf 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?
Comment #82
sun@DaveReid:
What's unclear here?
Comment #83
Dave Reid@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!
Comment #84
webchickGood stuff. Committed to HEAD!
This needs to be documented in the upgrade guide.
Comment #85
amitaibuIt 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.
Comment #87
salvis[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.
Comment #88
Dave Reid@salvis: That's not going to work because _menu_delete_item() expects an array.
Comment #89
salvis@Dave Reid: Look at the code of _menu_delete_item(). I did run it...
Comment #90
Dave ReidWell we could avoid the additional array -> object processing if we just fetch objects. For large menus maybe it would make a difference.
Comment #91
salvisYes, 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.
Comment #92
sunThere you go.
Now we need tests.
Comment #94
salvis@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.
Comment #95
salvisComment #96
Berdir@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.
Comment #97
sunBy 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. :)
Comment #98
salvis@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?
Comment #99
pwolanin CreditAttribution: pwolanin commentedmenu 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.
Comment #100
salvisOk, if that's how we want to have it...
Comment #101
salvisOops, sorry, wrong file...
Comment #102
pwolanin CreditAttribution: pwolanin commentedlooks fine
Comment #103
webchickLet's get a test for this.
Comment #105
salvisCommitting #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?
Comment #106
amitaibuI either need to get rights to add the docs + code example from #85, or someone with those rights should do it.
Comment #107
sunComment #108
amitaibuCan anyone grant me access to add the docs from #85?
Comment #109
webchickI just fixed the permissions again on http://drupal.org/update/modules/6/7, you should be good to go. Thanks, Amitaibu!
Comment #110
amitaibuAdded docs -- http://drupal.org/update/modules/6/7#custom_menu_api
Added coder issue -- http://drupal.org/node/654364
Comment #111
catchComment #113
sunJust noticed that Damien included a fix for the introduced workaround here in #576290: Breadcrumbs don't work for dynamic paths & local tasks
Let's rather fix it here.
Comment #114
PasqualleI guess in (menu.admin.inc) menu_edit_menu() the $form['old_name'] should be removed also..
Comment #115
sunI considered that but didn't remove those, because I wasn't sure whether they might be helpful for hook implementations. Not sure. For now, I'd prefer to go without removing them.
Comment #116
Pasqualleok, the patch is good
I do not understand why the return value of db_merge is not documented
http://api.drupal.org/api/function/db_merge/7
Comment #117
sun.
Comment #118
pwolanin CreditAttribution: pwolanin commented#113: drupal.menu-custom-dbmerge.patch queued for re-testing.
Comment #119
webchickCommitted to HEAD. Thanks!