For example, if you move the administration menu to, say, the primary links menu, and then enable a new module with a new administration page, the administration page is still placed in the navigation menu, although its parent has moved. This is a usability issue: think of all the tedious time you'd spend moving all new administration items to the correct spot on the primary links menu.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

theborg’s picture

Status: Active » Needs review
FileSize
1.03 KB

Maybe is because then new menu cannot find his parent when looking for it.

chx’s picture

Status: Needs review » Reviewed & tested by the community

yeah, while writing my new dropzone module i was contemplating the same. line 1841 db_query("UPDATE {menu_links} SET menu_name = '%s', plid = %d, link_path = '%s', so we do update the menu name so this is indeed a bug and the solution is fine.

theborg’s picture

FileSize
1.42 KB

Pointed by chx, remove the comment about the menu parents.

chx’s picture

Status: Reviewed & tested by the community » Needs work

uh, link_path is not unique, pwolanin is right, let me write a better patch.

pwolanin’s picture

we can have many parents with the same path - the previous code was a little faulty in this regard, but this is worse.

Maybe we need to prioritize parents with module == 'system'?

chx’s picture

Status: Needs work » Needs review
FileSize
1.15 KB
pwolanin’s picture

Status: Needs review » Needs work

I don't think that will work

pwolanin’s picture

or- it will work, but doesn't fix the bug

chx’s picture

Status: Needs work » Needs review
FileSize
1.85 KB

Opsie! My patch is correct but it fixes a different bug :D

pwolanin’s picture

Status: Needs review » Needs work

I'll give that a firm "maybe" - looks reasonable but I'll have to test. Seems to fix the fundamental flaw in the logic which assumes there is only one possible parent.

pwolanin’s picture

Status: Needs work » Needs review

didn't meant to set to CNW.

However, I'll still a little suspicious of the first part of the pacth. If you are re-parenting a link, you should know the new menu as well as the new plid, right?

chx’s picture

Yeah I surely know but if your UI provides a parent choosing mechanism why should the caller retrieve the menu name of the parent when the plid is the primary key anyways? Waste of time / effort.

pwolanin’s picture

@chx - yes, it's certainly true that you should not need both when plid maps to an mlid which must be unique.

pwolanin’s picture

Status: Needs review » Needs work

part or all of this change seems to have been included in the other patch. chx?

theborg’s picture

Status: Needs work » Needs review
FileSize
1.7 KB

Retested chx #9 and is still valid.

This is the same patch, redone after the others menu patches are applied.

David_Rothstein’s picture

Status: Needs review » Needs work

I tested this patch, and it looks like the behavior is on its way to being right, but not there completely.

What I did:
Moved the Administration menu to Primary Links, then enabled a module that creates its own menu items (Statistics).

What happened:
The new menu items don't appear in the Navigation block any more, which is a good development, but they don't appear where they're supposed to be either -- if you go the main admin page they don't show up. Instead, if you go to the menu administration page (admin/build/menu) and open the Navigation menu, there they are by themselves, without their parent (even though they don't appear in the Navigation block). Hm...

pwolanin’s picture

Status: Needs work » Needs review
FileSize
2.17 KB

How about the attached? Use GROUP BY to avoid one PHP loop, and behaves differently for links derives from router items vs. user-generated links.

pwolanin’s picture

Ah, #17 is not working, but I finally found the bug. I think the attached patch works as desired.

In chx code (and my derived code in #17), he sets

  if ($parent !== FALSE) {
    $menu_name = $parent['menu_name'];
  }

but, $menu_name is not used except to check if the item has moved menu. hence we need:

  if ($parent !== FALSE) {
    $item['menu_name'] = $parent['menu_name'];
  }
  $menu_name = $item['menu_name'];
pwolanin’s picture

hmm, chx chastized me for the above for being an ignorant MySQL user, and writing nasty non-standard GROUP BY SQL.

see attached better patch.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Tested, works fine, RTBC.

Gábor Hojtsy’s picture

Version: 6.x-dev » 7.x-dev
FileSize
2.46 KB

I went ahead and fixed a code comment in the code as well. Committed the attached patch. Also RTBC for 7.x.

STyL3’s picture

i'm in need of this patch, but am rather new to patching. i have cygwin installed and all, i just don't know which file needs this patch. any info is greatly appreciated.

pwolanin’s picture

http://drupal.org/patch

or just wait until later tonight, and the change will be in the new tarball: http://drupal.org/node/97368

STyL3’s picture

i've actually read all that. been awhile but i've read it. i downloaded the patch file and know how to use cygwin, but at the command line i have to tell it what file the patch is being applied to. this is what i was curious about. does a patch to core work differently than it does for a module?

David_Rothstein’s picture

If you do it correctly, you should never need to tell it what file to patch.

Just use patch -p0 < menu_reparent_211979-20.patch.txt and it will find the files to patch for you. (For patches to Drupal core, you need to run this command from the top-level directory of your Drupal installation, not any subdirectory -- maybe that was causing your problem?)

If you ever do need to figure out in advance what file(s) will be patched, you can just look inside the patch file and it is relatively easy to tell. For example, in this case the file "includes/menu.inc" is the one being patched.

Good luck... hope this helps.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD.

Anonymous’s picture

Status: Fixed » Closed (fixed)

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