Menu fixes and enhacements

sub

Totally agree -- in my experience this is one menu constant that needs excessive massaging to get to work (still haven't had the patience to figure out exactly why my attempts sometimes go wrong...), and the default results are usually so insufficient that I've always ended up writing my own callback to process the menu tree anyway.

To put it another way, where have you see 'type' = MENU_ITEM_GROUPING outside of the drupal core? Can't think of any examples off the top of my head :-).

Given the importance of the menu system, and its current lack-of-intuitiveness, (and my personal observation that it is always a huge stumbling block for developers new to drupal) any change that simplifies it is a good thing in my book.*

*but don't make it too simple, of course.

My impressions:

Good:
1) The first menu item at every level is now appearing again. (My Account, Page, Site Configuration, etc..)
2) The aggregrator isn't dumping stuff in the root of its submenu anymore.

Questionable:
1)

+function _aggregator_has_categories() {
+  return user_access('access news feeds') && db_num_rows(db_query('SELECT 1 FROM {aggregator_category}', 0, 1));
+}

Can we have a db_table_is_empty() function? It took me a second to figure out what the intention was here, and it looks like a useful predicate (Yeah, I know, another issue..)

Bad:
Couldn't find anything bad, sorry ;)

Misc:
1) The local tasks weren't being highlighted at first. After enabling some modules, it started working again, so I assume it was a caching problem.

2) Could someone with a full contrib checkout or grep access do a tally for MENU_ITEM_GROUPING?

Overall:
Lookin' good.

And 5 seconds after posting:

The code I had marked as "Questionable" really is so. You meant db_query_range() there, not db_query(), correct?

Tested and approved. Adding, deleting, editing, and generally folding and spindling paths works nicely. Setting multiple aliases for the same path works, too. Hoorah.

I've removed a spurious menu_rebuild() removed from index.php, but other than that it's the same patch that chx posted. I'm setting RTBC.

• There was a big fuss over whether bubbling should be allowed. There are various interpretations of this: one is simply whether we should show accessible deep items in the navigation block at the top level or not. Another is whether it makes sense that you can access "admin/build/block" when you can't access "admin" or "admin/build".

  My impression was that the result of that discussion was that we should not show these links and not allow access either.

  However, this is not at all what the current patch implements. Old items that would bubble are not visible in the menu, unless you type their path directly. IMO this is unacceptable as pages that appear to be inaccessible are really not.

  If you go to such a 'deep' page, the link appears in the menu, but is indented, though at an incorrect level. Try giving e.g. anonymous users the "administer blocks" permission. I don't think this is intentional behaviour, looking at the code:

  /**
   * Returns a rendered menu tree.
   */
  function menu_tree() {
    if ($item = menu_get_item()) {
      list(, $menu) = _menu_tree(db_query('SELECT * FROM {menu} WHERE pid IN ('. $item->parents .') AND visible = 1 ORDER BY mleft'));
      return $menu;
    }
  }
  
  function _menu_tree($result = NULL, $depth = 0, $old_item = NULL) {
    // ...
    while ($item = db_fetch_object($result)) {
      list($access, , $path) = _menu_translate($item, $map, MENU_RENDER_LINK);
      if (!$access) {
        continue;
      }
      // ...
      if ($item->depth > $depth) {
        list($remnant, $menu) = _menu_tree($result, $item->depth, $item);
        // ...
      }
      elseif ($item->depth == $depth) {
        // ...
      }
      // it's the end of a submenu
      else {
        // ...
        break;
      }
    }
    // ...
  }
  

  menu_tree() fetches a flat list of all menu items, in the order they appear in the (fully expanded) tree. _menu_tree() then runs through it, recursing at the appropriate steps.

  If the user has access to the entire tree, the code will correctly recurse through and output the nested lists.

  However, if a user does not have access to a particular item with children, the while () iteration will be aborted with continue. However, all those db_fetch_object() calls for the children that would've been called in the recursive call to _menu_tree() have not been done. So, the next while () iteration will in fact fetch the first child of the disallowed menu item, check its access, and output it. And because the recursion was not performed, the code will stop prematurely at the end of the submenu and come one recursion short.

  Obviously we could address this bug by patching _menu_tree(), but that still wouldn't solve the problem that permissions appear to be hierarchical while really being flat. This could pose security issues.

  IMO we should move to complete hierarchical permissions and perform access checking up the tree for each page. It's the only consistent solution to the bubbling problem. It could allow us to simplify the access callback inheritance rules though.

• _menu_tree(). This function has dead code ($old_type), unclear variables ($old_item, $remnant) and a whopping one line of comments. I had to stare at it for 45 minutes to figure it out, which is ridiculous for such a small piece of code. I doubt anyone else would do the same.
• The call to theme_menu_tree() inside theme_menu_item() is inappropriate. Theme functions should receive as much ready-to-output material as possible. It should be moved up into _menu_tree().
• The mright value and column in the database appears to be unused... we calculate the value, but don't do anything with it. I don't think anyone will want to render the menu tree backwards either.

I'm also not 100% sure about dropping MENU_ITEM_GROUPING. It is a useful feature... access callbacks only work if the module that owns the item knows exactly what items are under it. This is true for e.g. create content, but not for the admin pages. Without such functionality, we'll be doomed to useless permissions such as "access administration pages", no?

While it is relatively easy to implement in the current model (just add a snippet in _menu_translate to do the right query and checks), the problem is that we'd have to recurse up the entire top-level admin tree (down to each admin/foo/bar) to determine the visibility, which would probably kill performance.

The patch looks great (much more understandable now), but I have one more thing to add... now that access lookups are performed up the tree, we no longer need to inherit access callbacks for the case where the arguments do not change. This should save some useless duplicate access checks. :)

Implemented the above. In the case where we would inherit an identical access check, we set the callback to TRUE instead.

Fixed some more problems:

• The patch swapped the classes 'leaf' and 'collapsed'.
• When rendering the menu, the triangle bullet should only be used when a menu item has visible children. Local tasks and callbacks should not be counted in this (e.g. 'my account' should have a 'leaf' icon).
• When visiting a local task, the page title should remain the page of the first non-local-task parent. (note: I have a patch to make the page title in the <head&tgt; tags more useful too, but it needs to be updated)
• Callbacks that are to true due to inheritance rules should not be inherited from themselves in menu_rebuild() (thanks chx)

Wouldn't it be cleaner to replace db_num_rows(db_query_range('SELECT 1 FROM {aggregator_category}', 0, 1) with
db_result(db_result('SELECT COUNT(cid) FROM {aggregator_category}')) or isn't that what this line does?

What is block_callback for? It does not appear to be used?

Some of the code comments (i.e. + // New alias.) add no value.

Some of the code comments are a little bit cryptic. What does 'inherited normally' means, for example? Why can't we inherit from certain menu items? The code comments explain what the code does, but often not _why_ it does that. While the code comments got better, they are far from great.

That said, great job Steven and chx. :)

The speed trade-off is database-dependent. In MySQL/MyISAM, if you just want to know "how many rows" there are (or "is there at least one row") then count(*) is already extra-indexed to be virtually instantaneous. I don't believe that's the case in MySQL/InnoDB or Postgresql, but I am not an expert in those databases.

Queue stock line: Benchmarks would be good to see which method is faster, when.

Committed to CVS HEAD. It's not perfect, but it's another great milestone. Further improvements and fixes can go in follow-up patches. Thanks!