In one of my modules I replaced $item['type'] = MENU_NORMAL_ITEM with $item['type'] = MENU_VISIBLE_IN_TREE | MENU_VISIBLE_IN_BREADCRUMB, i.e. I removed the MENU_MODIFIABLE_BY_ADMIN property, so that the item should not be modifiable by the menu administrator anymore. However I can still edit the item. Not sure I missed a point here, but it seems to me that the menu system does not respect this property at all.

CommentFileSizeAuthor
#33 modif.module.txt2.5 KBpwolanin
#32 modif.zip_.jpg1.07 KBprofix898
#27 menumod.patch3.32 KBprofix898
#17 modified_out.patch11.75 KBchx
#11 menu_modifiable.patch2.75 KBcatch
#10 menu.patch2.75 KBchx
#8 menu.patch1.3 KBchx
#7 menu.patch0 byteschx
#3 modif.patch1.02 KBprofix898
#2 modif.1.01 KBchx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

profix898’s picture

profix898 CreditAttribution: profix898 commented
Title: menu administration does respect MENU_MODIFIABLE_BY_ADMIN » menu system does not respect MENU_MODIFIABLE_BY_ADMIN

upps. fixed title ;)

chx’s picture

chx CreditAttribution: chx commented
Status: Active » Needs review
FileSize
modif.1.01 KB
profix898’s picture

profix898 CreditAttribution: profix898 commented
FileSize
modif.patch1.02 KB

Two minor yet important corrections:

  • It must be $item['type'] instead of $menu['type'] in the first case
  • Expression if (!($item['type'] & MENU_MODIFIABLE_BY_ADMIN)) {... was missing a required parenthesis.

Otherwise looks good. Thanks.

@chx: Sorry, that I'm the one to find the hidden imperfections of the new amazing menu system ;)

chx’s picture

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

Yup!

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

I've committed a slightly modified version of this patch. Thanks.

Timotheos’s picture

Timotheos CreditAttribution: Timotheos commented
Status: Fixed » Needs work

Now when I'm logged in as admin I get an access denied when trying to add a menu item anywhere. If I remove this patch then it works.

chx’s picture

chx CreditAttribution: chx commented
Status: Needs work » Needs review
FileSize
menu.patch0 bytes
chx’s picture

chx CreditAttribution: chx commented
FileSize
menu.patch1.3 KB

I can't work on core with the bzr mirror broken not because I am picky but because more or less all my ports are filtered from here. We shall see what happens.

chx’s picture

chx CreditAttribution: chx commented

I wanted to say won't fix but I won't. customized will get a -1 value for these and it will work... why people can't get these before the freeze? :(

chx’s picture

chx CreditAttribution: chx commented
FileSize
menu.patch2.75 KB

D'oh!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Reviewed & tested by the community
FileSize
menu_modifiable.patch2.75 KB

This works great. Did a minor re-roll with some comments via chx in irc, so should be RTBC.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Priority: Normal » Critical

it's also critical with those 403s.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

hmmm, Is there even a reason to support non-modifiable links? This seems to be a relic of the old menu system.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Reviewed & tested by the community » Needs work

I think this is a bad time to be re-writing the menu system - let's roll back #5 and just eliminate MENU_MODIFIABLE_BY_ADMIN from MENU_NORMAL_ITEM - as above my expectation when coding this was that all all links are editable. Is there a compelling use case for having some that are not editable?

profix898’s picture

profix898 CreditAttribution: profix898 commented

We had MENU_MODIFIABLE_BY_ADMIN for a long time (at least in 4.6, 4.7 and 5), removing it now is regression. And that just because of the inconvenient timing when this bug was discovered ...

chx’s picture

chx CreditAttribution: chx commented
Status: Needs work » Closed (won't fix)

Well yes, the menu does not support that flag. Goba and pwolanin supports that invidual flags are not to be used in menu types and then we are good. Most other flags btw are supported. If you are so inclined then you can use form_alter on the overview and the menu item edit form -- both are forms now. Goba rolled back the patch Dries committed.

In other words: sorry, this is a feature request, late by six months. We would either need to change DB schema or hack. Neither is welcome this late.

chx’s picture

chx CreditAttribution: chx commented
Status: Closed (won't fix) » Reviewed & tested by the community
FileSize
modified_out.patch11.75 KB

Here is the reverse patch. Note that the 'node' item is consistent: it did not appear in the menu anyways as MENU_MODIFIABLE_BY_ADMIN was not handled as the issue says. menu_link_build never checked this flag:

  if ($item['type'] == MENU_CALLBACK) {
    $item['hidden'] = -1;
  }
  elseif ($item['type'] == MENU_SUGGESTED_ITEM) {
    $item['hidden'] = 1;
  }

So as long as MENU_CALLBACK and MENU_SUGGESTED_ITEM are not the same numerically, we are fine. And they are not because I re-added the 0x0010 to MENU_SUGGESTED_ITEM definition with a small comment.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Reviewed & tested by the community » Fixed

profix898: this is not really a regression, as the flag in question provided control on the UI provided for the menu item in question. Now that all the UI is form based, there is much finer control over it with hook_form_alter(). Also using the flags independently was not "expected behavior", although the flags were admittedly not documented as internal. To avoid hackless core solutions for reintroducing this feature in the menu API (as an alternative to the now supported forms API solution, to keep compatibility), would require breaking APIs (seriously chx's patch to solve this was a hack with other flags). Database changes are not allowed now, hacks are ideally not accepted, so all is left is to suggest the more fine grained solution for developers which works well.

profix898’s picture

profix898 CreditAttribution: profix898 commented

I'm really sorry to read this ... Never mind I'm long enough with Drupal to manage this with hook_form_alter() but it actually multiplies the code required (because you need to maintain a table of all items to hide/disable manually).

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

@profix898 -can you link to a post in the forum and explain your use case there? there may be other possible ways to achieve what you want.

profix898’s picture

profix898 CreditAttribution: profix898 commented
Priority: Critical » Normal
Status: Fixed » Active

Sorry for reopening the issue, but I dont think the problem is really solved as you cant easily use hook_form_alter() ... Why? ... Read on ...
It should be noted that you cant simply unset the items to hide them in the menu_overview_form! If you do and hit the Save button the items are messed up (they loose parents, etc.). That said it appears to be very difficult (if not impossible) to achieve a similar result with hook_form_alter() as it was possible with the flag. As a workaround I tried to empty the 'operations' subarray to hide the 'edit' and 'reset' links, so that the items are still visible but not editable. However you can still use the new DnD functionality to change the items. I couldnt find any way to alter the form without breaking the items more or less. I guess the submit handler for this form should check which items are actually in the form before saving the changes, otherwise using hook_form_alter() on the menu_overview_form breaks the menu. I seems that hook_form_alter'ing the form is not a good idea in general :/

Edit: @pwolanin: Sorry, I missed your post while writing this. I think my explanation above sheds a different light on the solution, not? Otherwise using hook_form_alter is not as difficult as I thought (i.e. I dont need a table etc.).

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

@profix898 - what is the use case? Your module could easily make a separate block of links that would not be editable with menu module.

chx’s picture

chx CreditAttribution: chx commented

Also, have you used the #access property?

profix898’s picture

profix898 CreditAttribution: profix898 commented

Taking the point that MENU_MODIFIABLE_BY_ADMIN was always meant for internal use only (Gabor #18), it doesnt make much difference for <= D5, because we had the chance to dynamically add items to the menu in the !$may_cache phase of hook_menu. We could also flag items with MENU_DYNAMIC_ITEM and they didnt appear in the menu administration (from the api docs of D5: "Dynamic menu items change frequently, and so should not be stored in the database for administrative customization.").
In D6 the situation is different. Now all items are stored in the db and all items are available for customization. However the MENU_MODIFIABLE_BY_ADMIN flag was still available and an easy way to prevent items from being altered manually. But now (after this patch) there is no way to have any sort of dynamic and unmodifiable item anymore at all and thats the big thing here!
(That doesnt diminish the new menu system at all, we are talking only about the new menu administration form.)

Use cases are actually all modules with frequently changing items or items you want to preserve a certain order for, esp. items where the order is based on an external data source (as in my case). Even if I would allow people to rename items or change description, I definetly dont want them to re-parent these items.

In the end there are two problems here:
1. There is no way to protect menu item from being altered manually (both possibilities from <=D5 with MENU_MODIFIABLE_BY_ADMIN and MENU_DYNAMIC_ITEM are no longer available)
2. Using hook_form_alter() on the menu_overview_form, which was said to be THE way to customize menu administration, breaks the menu more or less.

The normal/other forms in Drupal usually only store the items submitted by the form preserving everything else in {variable}. However in menu administration the 'after-hitting-save' state completely depends on form being transmitted, that why items get messed if you remove a row using hook_form_alter(). The easiest way to fix this is probably to merge the original items with the new values submitted by the form. This way all items removed by hook_form_alter() would still being saved (and therefore not break) while the other items are stored as dictated by the user. (Didnt have time to look into details, so I hope I didnt miss something here.)

@chx: What #access property? I cant remember that mentioned anywhere in the docs!? I dont think you are talking about the 'access callback' and 'access arguments' properties, do you?

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

profix898: a usual way to hide stuff from users but keep them in the form for submission is to set them to '#type' => 'value', which makes them appear in the submission but not on the user interface. That's pretty much required in some forms, where the value is expected to be submitted (for example properties in node forms).

profix898’s picture

profix898 CreditAttribution: profix898 commented

@Gabor: I already tried that as well (changed title, hidden, expanded, etc to be of type 'value' and emptied operations), but all I got was an empty row with only the DnD icon ...

profix898’s picture

profix898 CreditAttribution: profix898 commented
Status: Active » Needs review
FileSize
menumod.patch3.32 KB

Actually I cant see what was so bad about the original patch (except that it protected items too much). Here is a patch that reintroduced the MENU_MODIFIABLE_BY_ADMIN flag and fixes the issue reported in #6. Can anyone please review and tell me if there is any chance to get the flexibility to protect menu items back into D6!?

chx’s picture

chx CreditAttribution: chx commented
Status: Needs review » Closed (won't fix)

Again, form_alter , if not the whole item, then the operations column (and the edit item page). The patch is totally wrong because type is a menu_router column not a menu_links one. So if you create a link to a menu callback, that will not be modifiable imo. And finally, we have not heard any valid use case for this.

profix898’s picture

profix898 CreditAttribution: profix898 commented
Status: Closed (won't fix) » Needs review

@chx: I did write about the use cases above and as I said before as well using hook_form_alter() on this form is everything but trivial. Also the patch works ... I guess you didnt even try the patch nor did you try to form_alter the form, right? I'm fine to close this issue if anyone could tell me how to manage this. Basically I'm asking for something that was available for years ... if this is not fixable OK, but you could at least take 2 mins to think about and try ...

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs review » Needs work

here's my suggestion - the D6 menu API is radically changed, so I think what you want to do is for you module to have its own menu and block of links. Book module and menu module use the same API - you'll note that book module's menus are not editable at all via the menu module interface. However, all the book module's links are in the {menu_links} table as well.

So, you can basically make up a menu name - book module uses "book-toc-X" where X is a nid, menu module uses "menu-X" where X is user input. So I suggest you follow this pattern with something like "modulename-X" as the menu name. Then you can just start using menu_link_save() to added items to this menu, and menu_tree('modulename-X') or similar code to display the links in a block.

Without knowing more about how your module works, I can't help much more.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Category: bug » support

see also menu_enable() for code where some links are generated automatically.

profix898’s picture

profix898 CreditAttribution: profix898 commented
FileSize
modif.zip_.jpg1.07 KB

@pwolanin: Thanks. Attached to this post is a simple module to demonstrate what I'm trying to achieve. Actually I have two modules (a custom module I discovered this problem with and the gallery module package), where a similar problem exists in porting them to D6. Basically the modules read data from an external source (simply an array in the demo module) and generate menu items based on that data. Apparently it doesnt make sense to allow users/admins to edit the items, especially not to change the hierarchy. Only the base/toplevel item should be editable and movable. In D5 I simply added the child items with the MENU_DYNAMIC_ITEM flag in !$may_cache (or persistently in $may_cache with the MENU_MODIFIABLE_BY_ADMIN flag removed).
In principle porting the modules to D6 was straight forward. The behavior for all items was exactly as expected, until I accidently discovered that the items show up in the menu administration. I posted the bug report and now we are here ;) IMO using a separate menu is not a solution: 1. it would only contain one visible item in menu administration (namely the toplevel item) and 2. the items wouldnt show in the main 'navigation' menu (but thats what these modules were designed for ...). It also multiplies the effort required to implement such a simple module.

Edit: the demo module also implements hook_form_formid_alter() to hide the 'operations' link for the child items. Just in case anyone wants to play with that.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
modif.module.txt2.5 KB

attached is a re-written sample module - it basically works to do what you want, but showing the links in a menu block. Note that in this sample module the links are re-created on every page load. Also, there is no check to see whether they are valid - you'd need to define an access callback to do that.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

see also in menu_link_save(): drupal_alter('menu_link', $item, $menu);

you thus have the alternative of putting all the link's values back if the admin tries to alter them.

Otherwise, I stand by my suggestion above that you make your own block for 6.x.

Your module code needs a lot of work - not that hook_menu is ONLY called on menu rebuild, not on each page view. You also seem to be confusing links with router paths.

profix898’s picture

profix898 CreditAttribution: profix898 commented
Status: Needs work » Closed (fixed)

attached is a re-written sample module ...

Wow. Thanks a lot :) I just spent the last 2 1/2 hours playing with the code. Indeed I learned many details about the new menu system I was previously not aware of. I will think about your solution for my custom module as it provides a new level of flexibility to have the items separate.

But as mentioned above I'd like to have the links in the main menu (at least for gallery). The gallery_menu module was developed for the only purpose to add these items to the 'navigation' menu (we have an optional block with links already).

Although stated above that hook_form_alter() is the way to go and we could do mostly all this (and much more) with it, especially what was possible with the modifiable flag, its seems impossible to implement this way. At least nobody comes up with a solution here (and my attempts failed).
That means the module is basically NOT PORTABLE as is to Drupal 6 (now that the modifiable flag was removed). Its actually the first time in over 2 years with Drupal that I'm deeply disappointed with a decision to see this kind of regression happen ...

see also in menu_link_save(): drupal_alter('menu_link', $item, $menu);
you thus have the alternative of putting all the link's values back if the admin tries to alter them.

As I cant hide/remove the links in menu administration, that's definitely an alternative I havent thought of yet. Although it seems a bit hackish it is the only useful solution. Thx, I will try to implement it this way.

Your module code needs a lot of work - not that hook_menu is ONLY called on menu rebuild, not on each page view

Yes, I know. The real module checks for updates in hook_init() and triggers a rebuild if required.

You also seem to be confusing links with router paths.

If you add a MENU_NORMAL_ITEM in hook_menu it automatically generates both the router item and link item, not? I thought that was the easiest way to implement it, but I agree that it would far be more efficient to have as few router items as possible and add link items only. Its also much easier to maintain the links in the module. Good point.

I'm going to close this issue as all important details have been argued and there seem no chance to get non-customizable links (or a similar feature) back in :( Thanks again to pwolanin, I really appreciate your help.