Posted by sun on April 1, 2009 at 4:59pm
9 followers
| Project: | Administration menu |
| Version: | 7.x-3.x-dev |
| Component: | Code |
| Category: | task |
| Priority: | major |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | D7 stable release blocker |
Issue Summary
Offshoot from #293768: Allow to enable/disable menu additions:
On-demand loading of sub-links and module objects/data.
- Allow modules to expose a list (sub-menu) of data items below a certain menu item.
- Load this data dynamically on-demand upon hovering.
- Cache these sub-menus in the same way like the entire menu. Make sure that user roles/permissions are taken into account.
- Allow modules to invalidate their sub-menu caches, when an item is created/renamed/altered/deleted.
- Allow modules to expose more than one sub-menu.
Comments
#1
We need to re-purpose this issue and start with the basic implementation. Functionality is similar to contextual links in D7 core, but we want more. ;)
smk-ka already did some awesome work on this, so we just need to get that reviewed and in shape. Will post a patch soon.
#2
You need to run update.php after applying this patch.
1) The structure of $expand_map looks awkward, and may very well be the cause for duplicated menu links for Field UI for content types.
2) Looks like we could skip MENU_DEFAULT_LOCAL_TASK with this, but OTOH, I originally envisioned that we want to stack dynamic links below the respective "List" task, so other tasks on the same level are not buried within a potentially large list of dynamic links.
3) Plenty of @todos.
#3
#631550: Stale + improper logic for MENU_VISIBLE_IN_TREE and MENU_VISIBLE_IN_BREADCRUMB eliminated the need to cater for local tasks and actions. We've got almost 100% clean code now, yay! :)
So this patch needs to be revamped to only merge in router items with dynamic path arguments.
#4
Started to streamline this code.
Text formats are currently not properly handled by (the wrongly named) function admin_menu_local_tasks(), because they are no local tasks.
I'm going to try to investigate whether we cannot totally streamline this code, since all dynamic router paths are properly stored as menu links now. This basically means that the following should get us a full-blown sub-tree for a certain dynamic path:
$path = 'admin/structure/types/manage/%';
$pids = db_select('menu_links', 'ml')
->fields('ml', array('p1', 'p2', 'p3', 'p4', 'p5', 'p6', 'p7', 'p8'))
->condition('router_path', $path)
->execute()->fetchAssoc();
$pids = array_filter($pids);
$query = db_select('menu_links', 'ml');
$query->join('menu_router', 'm', 'ml.router_path = m.path');
$query
->fields('ml')
->fields('m', array_diff(drupal_schema_fields_sql('menu_router'), drupal_schema_fields_sql('menu_links')));
foreach ($pids as $column => $pid) {
$query->condition($column, $pid);
}
$result = $query->execute()->fetchAll(PDO::FETCH_ASSOC);
dsm($result);
#5
Edited above code snippet: works :)
#6
hmm... while admin_menu_tree() gets a $menu_name argument, this is not passed to hook_admin_menu_map() / hook_admin_menu_map_alter(), so it seems that those that implement these hooks may do their job for an unexpected menu tree (in the case that admin_menu_tree() is invoked for several menu trees)? ...or maybe admin_menu_tree() does not really need the $menu_name argument?
#7
Actually, the $menu_name belongs to the things the still need to be removed. This dynamic tree expansion relies on router paths in the first place. In D7, also dynamic router paths are stored as menu links, so we just leverage that to retrieve the dynamic menu link tree, regardless of its menu_name.
Work in progress patch for the D7 approach attached... however, endless recursion in admin_menu_merge_tree(), since some expanded paths are not local tasks, but regular menu items, so they do not have a $parent_path, but the current code requires all registered dynamic paths to be keyed by their parent path.
#8
Resolved the recursion error, removed obsolete helper functions, added plenty of docs.
#9
Quick note: If we want to backport this to D6, then we need to backport #631550: Stale + improper logic for MENU_VISIBLE_IN_TREE and MENU_VISIBLE_IN_BREADCRUMB -- pwolanin, the menu system maintainer already agreed that it would be possible to do that.
#10
Cleaned up field_ui implementation.
#11
This patch requires #742318: Fields are editable regardless of whether an bundle instance exists, missing menu titles, etc. for Drupal core to make any sense.
With this patch, the only remaining problem is that
admin/structure/taxonomy/2/fields/field_image
gets also expanded below
admin/structure/taxonomy/1/fields
Patch contains some debugging code due to that. Still not sure where exactly it goes wrong.
#12
Still no luck. Recursion still entered for (translated) paths where it shouldn't happen.
#13
Agreed, creating the right expansion parameter set for each item and level is definitely... tricky :(
Fixed the remaining bits.
#14
YAY! Works for me :)
We still need to resolve the two related core issues tough:
#742318: Fields are editable regardless of whether an bundle instance exists, missing menu titles, etc.
#744258: admin/structure/taxonomy paths have to use machine_name, not vid
And if anyone is interested in this for D6, the following patch just needs to be backported:
#631550: Stale + improper logic for MENU_VISIBLE_IN_TREE and MENU_VISIBLE_IN_BREADCRUMB
Aside from those, let's also look at the remaining @todos in this patch:
+++ admin_menu.inc 16 Mar 2010 19:56:59 -0000@@ -7,6 +7,363 @@
+function admin_menu_tree($menu_name) {
...
+ foreach ($expand_map as $path => $data) {
+ // Convert named placeholders to anonymous placeholders, since the menu
+ // system stores paths using anonymous placeholders.
+ // @todo Why specify named placeholders in the first place then?
+ $replacements = array_fill_keys(array_keys($data['arguments'][0]), '%');
+ $data['parent'] = strtr($data['parent'], $replacements);
+ $new_map[strtr($path, $replacements)] = $data;
...
+ $expand_map = $new_map;
Unless I overlooked an important detail, the named placeholder arguments are immediately destroyed here, and rebuilt dynamically during the tree merging. If that is the case, I'd say we should use anonymous placeholders in the first place?
+++ admin_menu.inc 16 Mar 2010 19:56:59 -0000@@ -7,6 +7,363 @@
+ // Unlikely, but possible.
+ if (empty($plids)) {
+ return array();
+ }
This becomes more likely in combination with access permission optimizations (see below).
+++ admin_menu.inc 16 Mar 2010 19:56:59 -0000@@ -7,6 +7,363 @@
+ // The retrieved menu link trees have to be ordered by depth, so parents
+ // always come before their children for the storage logic below.
+ // @todo Order by 'fit' or 'depth' instead for parent path handling below?
+ foreach ($p_columns as $column) {
+ $query->orderBy($column, 'ASC');
+ }
This @todo is most likely obsolete - we need the links in their true hierarchical order.
+++ admin_menu.inc 16 Mar 2010 19:56:59 -0000@@ -7,6 +7,363 @@
+ // Set up path arguments map; depends on whether the item is dynamic
+ // (contains placeholders) or not.
+ if (strpos($link['path'], '%') === FALSE) {
+ // Build static item and subtree.
+ $map = explode('/', $link['path']);
+ $item = admin_menu_translate($link, $map);
+ admin_menu_merge_tree($item, $tree_dynamic, $current_expand_map, $hidden);
+ $tree[$key]['below'] += $item;
+ }
+ else {
Since all links we are merging are dynamic items containing placeholders, I can't see to which items this code should apply...?
+++ admin_menu.inc 16 Mar 2010 19:56:59 -0000@@ -7,6 +7,363 @@
+ $newpath = strtr($path_dynamic, $replacements);
+ // If any placeholder couldn't be replaced, skip this item.
+ if (strpos($newpath, '%') !== FALSE) {
+ continue;
+ }
+ $map = explode('/', $newpath);
+ $item = admin_menu_translate($link, $map);
+ // No access.
+ if (empty($item)) {
+ continue;
+ }
...
+ * @todo We have links. Consider forking menu_link_translate() instead.
+ */
+function admin_menu_translate($router_item, $map) {
Actually, _menu_link_translate() does very similar stuff like we do here. But it seems we need a mixture of both.
+++ admin_menu.inc 16 Mar 2010 19:56:59 -0000@@ -7,6 +7,363 @@
+ // Build subtree using current replacement arguments.
+ // @todo Avoid rebuilding this for each item.
+ $new_expand_map = array();
+ foreach ($replacements as $placeholder => $value) {
+ $new_expand_map[$placeholder] = array($value);
+ }
Tinkered about this for quite some time, but couldn't see a way to avoid it. Thus, let's just remove the @todo?
+++ admin_menu.inc 16 Mar 2010 19:56:59 -0000@@ -7,6 +7,363 @@
+ // @todo Strip potential HTML from titles?
+ $router_item['title'] = strip_tags($router_item['title']);
When going through _menu_item_localize(), then the resulting 'link_title' should be sanitized (and translated).
+++ admin_menu.map.inc 16 Mar 2010 19:58:56 -0000@@ -0,0 +1,141 @@
+ * @todo Replace all/most of those API functions with direct DB queries;
+ * we only need the menu arguments (keys), not fully loaded objects.
...
+function filter_admin_menu_map() {
+ $map['admin/config/content/formats/%filter_format'] = array(
+ 'parent' => 'admin/config/content/formats',
+ 'hide' => 'admin/config/content/formats/list',
+ 'arguments' => array(
+ array('%filter_format' => array_keys(filter_formats())),
+ ),
+ );
+ return $map;
+}
Most dynamic items are probably located below a parent that is using proper access restrictions - e.g. in this case, admin/config/content/formats is only accessible to users with "administer filters" permission. menu_tree_all_data() should already leave inaccessible items out.
Thus, we could use an early-out return for implementations like this, so we do not even try to merge in those dynamic items.
Actually, filter_formats() will only return those text formats the current user is able to access in D7 (the "administer filters" permission no longer works like a "bypass" permission). So we definitely want to query the available formats ourselves here.
The same probably applies to all or most other implementations - we just need a list of available items. Access is handled elsewhere. And to optimize performance, certain implementations may be skipped entirely by checking a user permission upfront.
Powered by Dreditor.
#15
The idea was to make it as easy as possible for third party module developers to add support for admin menu (DX). A typical implementation of hook_admin_menu_map looks like:
<?php$map['admin/structure/taxonomy/%taxonomy_vocabulary'] = array(
'parent' => 'admin/structure/taxonomy',
'hide' => 'admin/structure/taxonomy/list',
'arguments' => array(
array('%taxonomy_vocabulary' => array_keys(taxonomy_get_vocabularies())),
),
);
?>
It is immediately clear which argument (or arguments, in case there are multiple) the placeholder %taxonomy_vocabulary in the $map key (or 'parent' or 'hide') refers to. And it allows to copy'n'paste from hook_menu, FTW.
Yes, this should be obsolete.
Well, I asked myself the same :) I can only speculate that may intention back then was to also support additional static menu items, possibly to be able to support grouping of (dynamic) menu items.
At least this part:
+ // If any placeholder couldn't be replaced, skip this item.+ if (strpos($newpath, '%') !== FALSE) {
+ continue;
+ }
seems like duplication, as admin_menu_translate()/_menu_translate() also does this check and returns an empty array. Whether we should use _menu_link_translate() now, I don't know.
The only way I can think of would be to simplify the array passed to the function by directly using $replacements (i.e., the dynamic arguments of the current item) instead. The code is currently mimicking the (rather complicated) format of expand_map because it was merged with the menu item's $item[expand_map], which turned out to be not working. Not sure if this would be easily possible, though.
The title is passed through l() which causes all HTML tags to appear – see theme_admin_menu_links(). This todo has obviously been figured out long time ago and is now obsolete.
Simple user access checks upfront – yes please. For querying only the required data – well, I prefer API simplicity over speed in this case (or, at least as long as the overhead is justified).
#16
Since the existing patch already works, I decided to commit the patch as is, so it's easier to track any further changes (in this patch).
Resolves most @todos, but also introduces new ones.
#17
As of now, I assume that the cause for the not contained "Comment fields" and "Comment display" items is the following:
- Node bundle field edit paths are
admin/structure/types/manage/%node_type/fields- Comment bundle field edit paths are
admin/structure/types/manage/%comment_node_type/comment/fieldsNote the additional "comment/" argument in there.
admin/structure/types/manage/%/comment is not a registered path, so it is neither contained in the regular menu tree, but also not in our dynamic.
#18
We need to adjust the 'parent' attribute from comments to attach the items to a valid parent element, i.e. admin/structure/types/manage/%node_type/fields.
#19
Does this patch applies to my problem, too?
#20
I was trying to fix #884032: Add enabled views to the 'Views' menu when I noticed that vocabularies weren't being loaded either.
Taxonomy paths were changed with #744258: admin/structure/taxonomy paths have to use machine_name, not vid, so this patch mirrors those changes.
#21
Thanks, committed!
#17/#18 still needs work.
#22
I missed the other reference to taxonomy_vocabulary under field_ui. Sorry for clogging up the issue.
#23
Thanks! Committed #22 to HEAD.
We still need to resolve dynamic Field UI paths for comments on node types, so back to active.
#24
Re-rolled #16 against HEAD.
#25
Thanks for reporting, reviewing, and testing! Committed #24 to HEAD.
A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.
#26
The missing expansion of "Comment fields" and "Comment display" still remains.
Attached patch is what I have right now. Contains lots of debugging code, but essentially, %comment_node_type does not get expanded, since we seem to have a general problem of only allowing one dynamic argument per path.
#27
Stopping here. The cause for all this trouble seems to be http://api.drupal.org/api/drupal/modules--comment--comment.module/functi..., which pretends to be a loader function, but is a _to_arg function in reality. Thus, no node type loaded or returned... not sure whether we can support such horribly broken code.
Nevertheless it might be a good idea to commit the drupal_array_merge_deep() change.
#28
The same bug with 6-3.x
After uninstalling menu item still is there.
When trying to remove it, I've got error:
The menu name may only consist of lowercase letters, numbers, and hyphens.See duplicate as well: #588872: Module menu sctructure persists in Navigation menu after uninstalling
#29
#30
The last submitted patch, admin_menu.dynamic.27.patch, failed testing.
#31
#27: admin_menu.dynamic.27.patch queued for re-testing.
#32
The last submitted patch, admin_menu.dynamic.27.patch, failed testing.
#33
Not sure if this is a UX feature, or a performance work-around for #1905144: High load time for admin_menu upon user login/menu refresh.
Marked #1923260: og_menus uselessly included / high server load at log-in as a possible specific use case for this.