Tabs Episode 2: Attack of the Clones
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | menu system |
| Category: | task |
| Priority: | normal |
| Assigned: | chx |
| Status: | closed |
To understand what's going on here let's start with search. When you search for 'foo' you want to see tabs that point to 'search/node/foo' and 'search/user/foo'. This requires some facility that rewrites the links when they are rendered. Instead of using a 'rewrite callback/arguments' we are using 'search/node/[search_get_keys]'. There are similar challenges when rendering the link for 'my account', 'my blog', 'my recent posts' in the menu or as a tab -- they need to change the wildcard in the link to the current user id.
So, we have node edit defined as 'node/[node_from_arg]/edit' and we need to determine whether we want to display this tab or not. To do that, we need to load the node specified by the arg and node_access it. This is what map callback did until now, but I decided it to be simpler if I roll this activity inside node_from_arg -- this way, you need to specify one function and that handles both operations.
Actually, three, because for the function user_from_arg , we needed to distinct between links in tabs (user/[user_from_arg]/edit) and the menu (my account, blog). Otherwise, you either would nto be able to view other user than the current or you won't have tabs for the current user.
The issue title comes from the fact that the various _from_arg functions are very similar.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| menu_41.patch | 40.51 KB | Ignored | None | None |
#1
Doh. The new schema.
#2
This is a re-roll with a missing comma in the install file and missing < in tracker.module.
asimmonds and I were testing and found a few different issues:
- No "my recent posts" tab in tracker
- node/#/edit doesn't work
- admin/content/taxonomy/edit/vocabulary/vid is a 404.
#3
% joins map callback in the graveyard per Dries' request -- we now have one unified syntax, if you want a generic wildcard, then you write something like
book/export/[]/[]. And, a few more fixes.#4
This patch includes asimmond's fix for the forum admin pages, as well as some documentation for the _menu_access function that chx and I worked on.
#5
Does that fix all problems or are there still some open issues? Will this require a second tabs patch?
I'll test this patch ASAP.
One small thing, looking at the patch, the syntax {foo} might be slightly more intuitive than [foo]. No need to change that now, but probably something we'll want to gather some feedback about.
#6
This adds back all primary tabs. N-levels of tabs is the final touch -- I can roll that into this if you want. It's not hard, requires no API change.
#7
N levels of tabs. There is no point in implementing just two. The difference to the previous patches is very small, I just added a loop to the tabs.
#8
Ugh. That was uploaded from my old CVS checkout. Here is the real one, from my bzr working tree.
#9
Now, the N-levels of tabs seem to work -- with the previous patch, if you were on a secondary tab , the primary did not appear. Now it does. We inspect the all levels above and below where we stand now. So, for user/1/edit/test we inspect tabs user/1/edit/test/* and then user/1/edit/* and then for user/1/* and that's it
#10
Erm, my index.php change sneaked in, otherwise same.
#11
Now we only two operations. The menu builder now runs three passes but the actual work is not more, just arranged in a different fashion. The first pass rips out the callback from paths so in pass 2 & 3 we do not have a separate
user/[user_current]anduser/[user_from_arg]but just one pathuser/[]the one used for matching anyways. Note that pass 1 is almost exclusively router stuff while pass 2 only deals with menu stuff. The split in pass 3 is also very visible. The pieces are coming together.#12
More comments.
#13
This patch is the result of the sheer brilliance of Steven and my attempts to realize his genius thoughts. Now you have paths like
node/%node/editwithout any helper functions. It's magic :) actually, it utilizes node_load. I tested with user, node and search tabs.#14
System.install
#15
According to forngren this is RTBC he tested all the tabs aside from the outline and the category tab but those I checked . Also, we have a Selenium test in the making -- stay tuned.
#16
Yes, patched tested and approved.
(subscribing)
#17
Theme fix.
#18
This seems like special-case code that needs to be removed. I'd prefer to have a clean and well-definied API rather than one that exploits certain accidentical patterns in the code. Maybe the _load should not be removed, but then we'd still need to unify _from_arg and _load. Having two ways to accomplish something does more harm than good.
<?php+ if (function_exists($matches[1] .'_load')) {
+ $from_arg_functions[$k] = array('function' => $matches[1] .'_load', 'is_numeric' => TRUE);
+ $match = TRUE;
+ }
?>
#19
the _from_arg and the _load are two different things. The best example is, I believe forum_from_arg , which is quite different from forum_load . However the order was wrong , now _from_arg overrides _load as it should be.
#20
I don't understand. Can you explain this more/better?
#21
No. Please read forum_load and forum_from_arg. You will see two functions that are absolutely different.
#22
I rather give in than debate over this, from_arg is gone for good. As this was a search-and-replace operation retesting is not necessary. The following files are changed from the previous patch: menu.inc , forum.module, filter.module, user.module
#23
This let me simplify the menu_translate function. Yay.
#24
With the anonymous user, I get:
* warning: Invalid argument supplied for foreach() in .\modules\user\user.module on line 57.
* user warning: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '' at line 1 query: SELECT * FROM users u WHERE in .\includes\database.mysql.inc on line 172.
For the anonymous user, user_current_to_arg() returns a 0, which is not placed into $path_map in _menu_translate() so user_current_load() is then called with a empty string.
Wrapping a isset() around this $return in _menu_translate() seems to fix it for this.
<?php$return = $to_arg_function(!empty($map[$index]) ? $map[$index] : '');
if (!empty($map[$index]) || isset($return)) {
$path_map[$index] = $return;
}
?>
#25
I can confirm asimmonds's findings. Here's a patch with the isset() added.
The only remaining bug I could find was that profile categories don't seem to show up. To reproduce, enable profile module, add some field, and go to user/1/edit. The tab to edit the field isn't there.
#26
Btw, I tested this pretty thoroughly to thank chx for all the time he spent on the elusive login issue; I enabled all core modules (except menu), hit all URLs with tabs on them, including previous troublespots (tracker, search, forum), so I'm pretty sure once this is fixed, this sucker's ready to go.
#27
Here's above patch without settings.php. :P
Did some more digging... looks like menu_rebuild() isn't being called when profile fields are created, since after putting that in index.php it worked fine. Still investigating.
#28
OK I think this does it. I added a menu_rebuild() right after the cache_clear_all() in profile_field_form_submit.
Marking back for review.
#29
Committah-ed! :)
#30