Hook_menu_alter doesn't work if menu item changed
If I do anything to a menu item, including changing its position (weight) with menu administration, then hook_menu_alter is unable to change its title. This seems to be contrary to the way the hook should work; essentially the hook is all but worthless in this case. Does anyone know why this is this way? Yes, I can update the menu_links table and change it there just fine, but then Im doing a query to change what a query should be about to do anyway. Nancy E. Wichmann, PMP Injustice anywhere is a threat to justice everywhere. - Martin L. King, Jr. No virus found in this outgoing message. Checked by AVG - http://www.avg.com Version: 8.0.176 / Virus Database: 270.12.4/2079 - Release Date: 4/24/2009 7:04 PM
When generating navigation links from router items, core honors customized menu links. You customized your link so we do not overwrite it. I feel this is correct behaviour.
I understand your reasoning. However, there is "customized" and there is "customized." Dragging a menu item to a new position in the menu is not really "customized" in most people's opinion. This behavior forces me to do an UPDATE query on the menu_links table because hook_menu_alter doesn't work; is that really what you want? Nancy E. Wichmann, PMP Injustice anywhere is a threat to justice everywhere. - Martin L. King, Jr. No virus found in this outgoing message. Checked by AVG - http://www.avg.com Version: 8.0.176 / Virus Database: 270.12.4/2079 - Release Date: 4/24/2009 7:04 PM
No, I want you to use menu_link_save.If there are various customizations then please file a patch. On Sat, Apr 25, 2009 at 12:58 PM, Nancy Wichmann <nan_wich@bellsouth.net> wrote:
I understand your reasoning. However, there is "customized" and there is "customized." Dragging a menu item to a new position in the menu is not really "customized" in most people's opinion.
This behavior forces me to do an UPDATE query on the menu_links table because hook_menu_alter doesn't work; is that really what you want?
Nancy E. Wichmann, PMP Injustice anywhere is a threat to justice everywhere. - Martin L. King, Jr.
No virus found in this outgoing message. Checked by AVG - http://www.avg.com Version: 8.0.176 / Virus Database: 270.12.4/2079 - Release Date: 4/24/2009 7:04 PM
<div><br></div>
This is wacky! In 5.x to change the title of a content type, all it took was a variable_get and a simple bit of code in hook_menu and hook_node_info. But in 6.x I have to jump through all kinds of hoops changing the menu? I still haven't succeeded in getting it to update the description in the rendered menu. Is there an example somewhere? I see several, but I don't have the "mlid"; if I have to do a query to get that, I might as well go ahead and keep updating the tables directly. Nancy E. Wichmann, PMP Injustice anywhere is a threat to justice everywhere. - Martin L. King, Jr. No virus found in this outgoing message. Checked by AVG - http://www.avg.com Version: 8.0.176 / Virus Database: 270.12.4/2079 - Release Date: 4/24/2009 7:04 PM
No. Only if you customized your menu link then you need to. I am sorry, but this is how it is. If you consider this a bug, then please file an issue. The bugfix would be changing f (!$existing_item || !$existing_item['customized']) in menu.inc to something a lot more sophisticated. I am happy to review such a patch while I am not exactly eager to write it. On Sat, Apr 25, 2009 at 2:37 PM, Nancy Wichmann <nan_wich@bellsouth.net> wrote:
This is wacky! In 5.x to change the title of a content type, all it took was a variable_get and a simple bit of code in hook_menu and hook_node_info. But in 6.x I have to jump through all kinds of hoops changing the menu? I still haven't succeeded in getting it to update the description in the rendered menu. Is there an example somewhere? I see several, but I don't have the "mlid"; if I have to do a query to get that, I might as well go ahead and keep updating the tables directly.
Nancy E. Wichmann, PMP Injustice anywhere is a threat to justice everywhere. - Martin L. King, Jr.
No virus found in this outgoing message. Checked by AVG - http://www.avg.com Version: 8.0.176 / Virus Database: 270.12.4/2079 - Release Date: 4/24/2009 7:04 PM
<div><br></div>
Quoting Nancy Wichmann <nan_wich@bellsouth.net>:
This is wacky! In 5.x to change the title of a content type, all it took was a variable_get and a simple bit of code in hook_menu and hook_node_info. But in 6.x I have to jump through all kinds of hoops changing the menu? I still haven't succeeded in getting it to update the description in the rendered menu. Is there an example somewhere? I see several, but I don't have the "mlid"; if I have to do a query to get that, I might as well go ahead and keep updating the tables directly.
The menu is cached and is only rebuilt in certain circumstances. This doesn't provide for changing a static menu that is predefined unless you rebuild the cache for all menu items. This is a costly effect. -- Earnie -- http://r-feed.com/ -- http://for-my-kids.com/ -- http://www.4offer.biz/ -- http://give-me-an-offer.com/
I am aware of the overhead. I am using menu_rebuild() but it is not working because just re-weighting a menu item marks it as "customized." Nancy E. Wichmann, PMP Injustice anywhere is a threat to justice everywhere. - Martin L. King, Jr. No virus found in this outgoing message. Checked by AVG - http://www.avg.com Version: 8.0.176 / Virus Database: 270.12.4/2080 - Release Date: 4/25/2009 8:29 AM
I have to agree with Nancy. Re-weighting a menu item does not qualify as customized, IMO. Otherwise, I agree it works as designed. ..chris On Sun, Apr 26, 2009 at 3:00 PM, Nancy Wichmann <nan_wich@bellsouth.net> wrote:
I am aware of the overhead. I am using menu_rebuild() but it is not working because just re-weighting a menu item marks it as "customized."
Chris Johnson wrote:
I have to agree with Nancy. Re-weighting a menu item does not qualify as customized, IMO. Otherwise, I agree it works as designed.
The thing is, the system can only tell that there's an entry in the table, not what part of the item has been modified. I ran into this in some completely unrelated piece of code, where I ended up storing weights separately so that moving an item up or down didn't constitute modifying it, and it is a lot of extra code just to do that.
Quoting Earl Miles <merlin@logrus.com>:
Chris Johnson wrote:
I have to agree with Nancy. Re-weighting a menu item does not qualify as customized, IMO. Otherwise, I agree it works as designed.
The thing is, the system can only tell that there's an entry in the table, not what part of the item has been modified. I ran into this in some completely unrelated piece of code, where I ended up storing weights separately so that moving an item up or down didn't constitute modifying it, and it is a lot of extra code just to do that.
So the menu item becomes partially disconnected from the module. This isn't a good thing. Thanks, Earl, I think you explained something I noticed with a custom module I'm creating. Drove me nuts until I directly deleted a menu item that shouldn't exist after the module was uninstalled. I couldn't delete it via the menu UI at all. The menu item caused unpredictable actions when the module was reactivated or the menu cache was rebuilt and I was going bonkers trying to fix a problem caused by something as simple as changing the item's weight via the UI. -- Earnie -- http://r-feed.com/ -- http://for-my-kids.com/ -- http://www.4offer.biz/ -- http://give-me-an-offer.com/
Curiosity (and recognizing I'm about to run into the same problem Nancy had) made me look. Each of the menu item editing forms has an ['#item'] element that gets the original menu item. So instead of a more sophisticated test in _menu_navigation_links_rebuild(), maybe a more sophisticated test assigning a value to $item['customized'] just prior to calling menu_link_save() (though there's 17 occurrences of menu_link_save() in core...)? On Mon, Apr 27, 2009 at 12:36 PM, Earl Miles <merlin@logrus.com> wrote:
Chris Johnson wrote:
I have to agree with Nancy. Re-weighting a menu item does not qualify as customized, IMO. Otherwise, I agree it works as designed.
The thing is, the system can only tell that there's an entry in the table, not what part of the item has been modified. I ran into this in some completely unrelated piece of code, where I ended up storing weights separately so that moving an item up or down didn't constitute modifying it, and it is a lot of extra code just to do that.
I am not the menu expert here, so I am sure that there is something wrong with my little change: function menu_overview_form_submit($form, &$form_state) { // When dealing with saving menu items, the order in which these items are // saved is critical. If a changed child item is saved before its parent, // the child item could be saved with an invalid path past its immediate // parent. To prevent this, save items in the form in the same order they // are sent by $_POST, ensuring parents are saved first, then their children. // See http://drupal.org/node/181126#comment-632270 $order = array_flip(array_keys($form['#post'])); // Get the $_POST order. $form = array_merge($order, $form); // Update our original form with the new order. $updated_items = array(); $fields = array('expanded', 'weight', 'plid'); foreach (element_children($form) as $mlid) { if (isset($form[$mlid]['#item'])) { $element = $form[$mlid]; // Update any fields that have changed in this menu item. foreach ($fields as $field) { if ($element[$field]['#value'] != $element[$field]['#default_value']) { $element['#item'][$field] = $element[$field]['#value']; $updated_items[$mlid] = $element['#item']; $updated_items[$mlid]['customized'] = $field == 'plid'; // <-- only plid is really customized. } } // Hidden is a special case, the value needs to be reversed. if ($element['hidden']['#value'] != $element['hidden']['#default_value']) { $element['#item']['hidden'] = !$element['hidden']['#value']; $updated_items[$mlid] = $element['#item']; } } } // Save all our changed items to the database. foreach ($updated_items as $item) { // $item['customized'] = 1; menu_link_save($item); } } Frankly, I dont think expanded is really customized either. I tested this on my dev site and got the results I want. So now Karoly or Earl will tell this ditzy blonde why this is not the solution. Nancy E. Wichmann, PMP Injustice anywhere is a threat to justice everywhere. - Martin L. King, Jr. No virus found in this outgoing message. Checked by AVG - http://www.avg.com Version: 8.0.176 / Virus Database: 270.12.4/2082 - Release Date: 4/27/2009 6:19 AM
Nancy Wichmann wrote:
I tested this on my dev site and got the results I want. So now Karoly or Earl will tell this ditzy blonde why this is not the solution.
I don't know enough about the whole menu system to say whether or not this is the solution. It might be, though, or it might be on the track to a solution.
I would like to gently remind you that the place to get feedback on core changes is in the issue queue, where everyone can participate.
Thing is, it's not a bug, whether folks want it to work that way or not. It is literally *by design*. So it would have to go under Drupal 7, no? On Mon, Apr 27, 2009 at 4:20 PM, Karoly Negyesi <karoly@negyesi.net> wrote:
I would like to gently remind you that the place to get feedback on core changes is in the issue queue, where everyone can participate.
I wanted to get laughed at here first... Nancy E. Wichmann, PMP Injustice anywhere is a threat to justice everywhere. - Martin L. King, Jr. No virus found in this outgoing message. Checked by AVG - http://www.avg.com Version: 8.0.176 / Virus Database: 270.12.4/2082 - Release Date: 4/27/2009 6:19 AM
You are not being laughed at but this belongs to the issue queue,. It can be a bug....
http://drupal.org/node/446656 Nancy E. Wichmann, PMP Injustice anywhere is a threat to justice everywhere. - Martin L. King, Jr. No virus found in this outgoing message. Checked by AVG - http://www.avg.com Version: 8.0.176 / Virus Database: 270.12.4/2082 - Release Date: 4/27/2009 6:19 AM
Reviewed as promised.
BTW, during my testing, I dragged the top menu item to after the next one. This marked BOTH items as customized, even though I only changed one. Is that also the intended behavior? Nancy E. Wichmann, PMP Injustice anywhere is a threat to justice everywhere. - Martin L. King, Jr. No virus found in this outgoing message. Checked by AVG - http://www.avg.com Version: 8.0.176 / Virus Database: 270.12.4/2082 - Release Date: 4/27/2009 6:19 AM
Nancy Wichmann wrote:
BTW, during my testing, I dragged the top menu item to after the next one. This marked BOTH items as customized, even though I only changed one. Is that also the intended behavior?
Yes, tabledrag re-orders everything between an item's new position and old position. Here's an example of why: Original weights: Item A: 1 Item B: 2 Item C: 3 Item D: 4 Item E: 5 Move item D to the 2nd position. New weights: Item A: 1 Item D: 2 [changed] Item B: 3 [changed] Item C: 4 [changed] Item E: 5
participants (6)
-
Chris Johnson -
Earl Dunovant -
Earl Miles -
Earnie Boyd -
Karoly Negyesi -
Nancy Wichmann