Normal users should not see the create content link appear by default in a menu called "Management"

Wow, quite a patch :)

If we want to start down the road to a menu reorganization with something simpler, I'm attaching the screenshot I made in the other issue. The code to create this was only a few lines, although probably needs some cleanup since it was a little bit ugly and a little bit buggy.

The basic idea is that the "Create" menu would be seen by any user who has the ability to create content, but the "Manage" menu (which provides a separate window into content creation) would usually only be seen by higher-privileged site administrators.

Not sure about the Browse menu, however splitting the create and manage menus sound all right.

Well, "Browse" is just a renamed version of the existing Navigation menu -- renaming it to Browse was based on a throwaway suggestion in the other issue (I think by Dries).

We could equally well call it "Navigate" instead, or put them all back to nouns, etc.

Since the Management menu includes both the content creation and admin menus ATM, just disabling the management menu as in #511258: Do not enable the management menu by default could be problematic, since then there is no link to create content. So to avoid duplicating the admin menu in the toolbar and the sidebar, we should indeed break out the content creation. I don't think that a full blown menu reorg as presented by @sun is the way to go ATM, especially that many things changed in the menu structure since @sun's patch suggestion.

Ugh, scratch that, the toolbar already has a direct link to add content, so it cover all of the management menu main items.

Uhm, hum. I was wrong. We do need to solve this, since otherwise simple authenticated users really have no way to add content (via a link). /me misses his coffee :)

With the toolbar in core, the only block that we _really_ have to create is a 'Create (new content)' block for people that are not administrators. That would unblock #511258: Do not enable the management menu by default.

The other blocks could be left for contributed modules to provide. So maybe as a first step is just to separate out the 'Create' stuff.

It is horribly wrong to base the entire IA + UX of Drupal core on the sole assumption that the Toolbar will always be there. Unassigning from me - I'm not guilty.

D7 IA #fail.

@tha_sun Common, I still think relabling this menu is worth it - as it would probably cause confusion over what is "Management".

Right, as I recall, this was known to be a critical followup of that other patch from the moment it was committed. It's certainly true that it can be fixed on any individual site, but I don't want to see Drupal released with a configuration that makes no sense whatsoever for sites that allow community-created content.

Retitling this to focus more on what the exact problem is. We can certainly go with whatever the simplest solution is at this point, IMO.

In theory, this should be rather simple. Just remove the menu definition from node/add. However, that somehow makes the add content item appear at a "random place" (ie. as a child below another "random" item), not as a top level item in the menu. I've tried to debug this extensively, but could not find out how does it work/appear as a top level item in the management menu but not in the navigation menu.

I'd also suggest renaming the menu item to "Add content" for consistency with the management menu.

Anyway, this is obviously not as simple.

That last observation sounds heavily like this epic issue: #550254: Menu links are sometimes not properly re-parented

But on a second thought, 'menu_name' => 'navigation' possibly needs to be specified here, as 'menu_name' is derived from 'node' otherwise.

Ok, this one was tricky :)

The issue here is that the parent link 'node' *is* in the navigation menu, but as an invisible link (hidden = -1). So it found and used that as parent.

To test for this, I just added a condition so that we only look for parents that don't have -1 in the hidden field, maybe this needs to be made more complex (based on the current item, for example) but it does work correctly now for this case.

subscribe.

+++ includes/menu.inc
@@ -2665,6 +2665,8 @@ function menu_link_save(&$item) {
+    // Only visible links should be considered as possible parents.
+    $query->condition('hidden', -1, '<>');

1) The proper condition would have to be hidden = 0, but

2) This change breaks proper storage of depth/hierarchy of local tasks.

107 critical left. Go review some!

Ok :)

So what about this then:

Each menu link can only have parents with the same or higher visibility. I'm not really sure about the meaning of hidden = 1, but it seems to work for 0 and -1. At least, all menu router tests pass, but I don't know how good they are...

hidden = 0 means a menu link is visible, -1 means it's a MENU_SUGGESTED_ITEM in its initial disabled state, and 1 means it has been disabled via menu module's UI. Only sounds weird, pretty straightforward otherwise.

However, the 'hidden' column is a wrong condition for the (re-)parenting process. This problem is one of the built-in limitations of the current menu system, in which menu links are derived from menu router items. That design won't change soon and definitely not prior to D8, so we need to find a proper solution here.

1) We want to explicitly define 'menu_name' => 'navigation' for node/add, no matter what we do, so as to ensure that this sub-tree of menu links initially ends up where it's supposed to be.

2) We want a menu router sub-item to appear on the top-level. I wonder whether we can specify 'plid' => 0 for node/add. Technically, the menu router information is merged with menu link defaults in the process chain. Therefore, 'menu_name' => 'navigation' and 'plid' => 0 should make node/add appear on the top-level of the Navigation menu.

3) If we can't make 2) work, then we could also investigate whether we can assign a non-existing fake 'menu_name' to for the parent 'node' path. However, 2) should be possible to do, even if it may require changes to the menu router to menu link conversion.

2) We want a menu router sub-item to appear on the top-level. I wonder whether we can specify 'plid' => 0 for node/add. Technically, the menu router information is merged with menu link defaults in the process chain. Therefore, 'menu_name' => 'navigation' and 'plid' => 0 should make node/add appear on the top-level of the Navigation menu.

Yes, this should be possible. See #508458-48: Config and modules page for a patch from a long time ago that did that, and #596010-8: Move "Administration" link into Account menu and make Toolbar output first level of 'admin' menu for a more recent one; it should be possible to cobble together something from there if necessary.

Whether this all is a good idea or not is another question :) In some ways it would be cleaner to create a dedicated menu.

Ok. I agree that 2) should work and I'm obviously fine with not having the hidden condition, especially if it isn't a regression from D6 (can someone confirm that?). I'm just trying to bring some critical issues forward :)

Not sure about 3) but I can try it.

@David: My revised + much simplified patch over in #596010: Move "Administration" link into Account menu and make Toolbar output first level of 'admin' menu should work just fine.

@Berdir: We've spent quite some time on the menu system to finally make it store a proper hierarchy for local tasks over in #631550: Stale + improper logic for MENU_VISIBLE_IN_TREE and MENU_VISIBLE_IN_BREADCRUMB. The 'hidden' condition of this patch would revert and break that hierarchy. Since your second patch passed, our tests do not seem to fully cover all suggested/disabled/enabled scenarios yet. They will likely be advanced via #550254: Menu links are sometimes not properly re-parented

Let's see whether 2) works.

Of course, it doesn't. This one, a variant of 3), works though. Need to remember to open a new bug report for this weird auto-parenting, but that misbehavior is possibly related to #550254: Menu links are sometimes not properly re-parented

@sun, #26 would work if you also included the menu.inc changes. However, the first linked to patch (which was simpler) unfortunately might not work anymore - I think there has been some overloading of the meaning of 'plid' = 0 vs 'plid' = NULL in the intervening months unfortunately, which makes this more complex.

#27: drupal.node-add.27.patch queued for re-testing.

@pwolanin: Could you direct us which approach to take? #27 works, but we can also make #26 work. Either way leaves the feeling of a "hack".

Note that both patches passed the tests, but writing proper tests for this menu system functionality belongs into #550254: Menu links are sometimes not properly re-parented

Gábor explained the bug of #26 in #16. To make the approach of #26 work, we'd have to rethink and redo David's patch (would've to happen in a separate issue first though).

+++ modules/node/node.module	12 Apr 2010 23:26:02 -0000
@@ -1844,14 +1844,15 @@ function node_menu() {
+    // Required to make 'node/add' appear on the top-level of 'navigation' menu.
+    'menu_name' => '',
     'type' => MENU_CALLBACK,
   );
   $items['node/add'] = array(

This will most likely also fix one of the remaining issues in #576290: Breadcrumbs don't work for dynamic paths & local tasks

Powered by Dreditor.

@sun re #23 no:

hidden = -1 means it's a MENU_CALLBACK - never visible. Those are in the tree as a trick to make breadcrubs, etc work (sort of, better than not at all).

hidden = 1 is a MENU_SUGGESTED_ITEM in its initial disabled state, OR means it has been disabled via menu module's UI.

This needs a summary of what work needs to be done by someone who understands the history and can explain the current status. (needs a pre-review description of what tests need to be done by a person wanting to do the review)

Or... seems like sun and David gave a review, and since they didnt mark it rtbc, even though I dont understand exactly what they said, it must need work. :)

@pwolanin: We still need a decision on which direction to take (see #31). One involves to use a menu link property in a router item definition, the other one involves a menu_name trick to make the auto-parenting process for menu links forget about the /node path. (Note that the latter may also help us to fix broken dynamic breadcrumbs in the other issue.) Both achieve the same goal: /node/add (Add content) appears as top-level link in the Navigation menu, instead of the Management menu.

#27: drupal.node-add.27.patch queued for re-testing.

Maybe not critical, but certainly "major".

The patch is definitely an improvement over the current situation, but overall still seems like a regression from Drupal 6. Adding new content does not have much to do with "navigation", yet that is the menu title everyone will see it under with this patch (see attached screenshots of what it looks like for a regular content creator and an administrator). Drupal 6 made more sense here, IMO :(

Changing to major as per tag.

Tag update

Sub

#27: drupal.node-add.27.patch queued for re-testing.

Although badly needed, this is D8 material according to the rules (I had to learn today). It may be backported at a later point in time (though that's unlikely).

OK, I get that you are upset about other issues, but seriously:

The current UI sucks; it makes a mockery of one of biggest strengths of Drupal (end-user created content), and the patch, although not perfect, at least improves things and gets rid of the strange implication that adding content to a site is a "Management" activity, and does so with a very small change.

If it still works, it should be committed to D7.

Subscribing for now, and tagging as a beta blocker. If we move this link, it has tremendous impact on book authors, documentation team, etc. Need to read up above and figure out why people feel this is so important.

Ok. Apologies for going all "meta" for a moment, but got the chance to talk to Bojhan in IRC tonight about this and #511258: Do not enable the management menu by default.

Given where we're at in the cycle, we can only really make very minor, incremental tweaks to the UI at this point, and should avoid it whenever possible lest we face wrath from book authors, documentation team members, and others affected. However, given the current morass of confusing navigation we confront users with out-of-the-box, I think this represents a critical enough problem to warrant a second look.

These two patches together would fix the following problems:

- Removes the confusing concept of adding content being a "Management" task, which it's not for all but basically brochure-ware sites.
- Helps reduce the sea of navigation referenced above to merely a Great Lake: http://img.skitch.com/20101020-dkywhen89n7kx6n9s79kakhy6c.png.
- Displays the Navigation menu immediately upon installation, thus training new users that stuff can go in there, so it's not so shocking when they enable something like Forum module and suddenly this weird block pops up from nowhere.

I asked Bojhan about the "Navigation" confusion, since this isn't about navigating at all, really, and creating content is more of a user-focused task than anything. He acknowledged that it's somewhat of a trade off, but there's no other clear solution atm.

I agree, and think that overall this patch puts us ahead of where we are in D7 currently, while being the smallest possible UI change that could possibly help.

Therefore, Committed to HEAD. Thanks a lot for the thoughtful discussion here. I'll see you over at #511258: Do not enable the management menu by default. (and #550254: Menu links are sometimes not properly re-parented if I can ever find 4 spare hours to understand that issue :P)

I realize we're still waiting for a direction from pwolanin on the exact approach here. My hope is that this can be a non-critical follow-up now that the UI is doing what we want.

After having spent weeks with the menu system's current re-parenting logic, and seeing this patch in the commit stream, I wonder why we need that hack here at all. Reading through the issue follow-ups, it looks like we didn't try the most simple solution.

Hm... Trying it out quickly it didn't seem to break anything, and the code certainly looks like it makes sense too.

I'm going to RTBC this patch, because it removes a needless hack, and all links for node* paths are expected to appear in the Navigation menu anyway.

But, what does it do?

I was thinking this was changing a block title but maybe not?

No, there is no visible change.

no UI change = good UI change! :P

Committed to CVS HEAD. Thanks.