Tabs Episode 2: Attack of the Clones

chx - February 4, 2007 - 02:51
Project:Drupal
Version:6.x-dev
Component:menu system
Category:task
Priority:normal
Assigned:chx
Status:closed
Description

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.

AttachmentSizeStatusTest resultOperations
menu_41.patch40.51 KBIgnoredNoneNone

#1

chx - February 4, 2007 - 02:57

Doh. The new schema.

AttachmentSizeStatusTest resultOperations
menu_42.patch44.29 KBIgnoredNoneNone

#2

webchick - February 4, 2007 - 03:30
Status:needs review» needs work

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.

AttachmentSizeStatusTest resultOperations
menu_43.patch45.28 KBIgnoredNoneNone

#3

chx - February 4, 2007 - 03:48
Status:needs work» needs review

% 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.

AttachmentSizeStatusTest resultOperations
menu_44.patch48.43 KBIgnoredNoneNone

#4

webchick - February 4, 2007 - 04:50

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.

AttachmentSizeStatusTest resultOperations
menu_45.patch49.74 KBIgnoredNoneNone

#5

Dries - February 4, 2007 - 08:24

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

chx - February 4, 2007 - 10:10

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

chx - February 4, 2007 - 14:58

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.

AttachmentSizeStatusTest resultOperations
menu_46.patch48.43 KBIgnoredNoneNone

#8

chx - February 4, 2007 - 15:01

Ugh. That was uploaded from my old CVS checkout. Here is the real one, from my bzr working tree.

AttachmentSizeStatusTest resultOperations
menu_47.patch47.96 KBIgnoredNoneNone

#9

chx - February 4, 2007 - 16:51

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

AttachmentSizeStatusTest resultOperations
menu_48.patch49 KBIgnoredNoneNone

#10

chx - February 4, 2007 - 21:41

Erm, my index.php change sneaked in, otherwise same.

AttachmentSizeStatusTest resultOperations
menu_49.patch48.78 KBIgnoredNoneNone

#11

chx - February 5, 2007 - 23:48

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] and user/[user_from_arg] but just one path user/[] 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.

AttachmentSizeStatusTest resultOperations
menu_50.patch49.18 KBIgnoredNoneNone

#12

chx - February 6, 2007 - 09:46

More comments.

AttachmentSizeStatusTest resultOperations
menu_51.patch50.35 KBIgnoredNoneNone

#13

chx - February 8, 2007 - 13:27

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/edit without any helper functions. It's magic :) actually, it utilizes node_load. I tested with user, node and search tabs.

AttachmentSizeStatusTest resultOperations
menu_52.patch50.57 KBIgnoredNoneNone

#14

chx - February 8, 2007 - 13:37

System.install

AttachmentSizeStatusTest resultOperations
menu_53.patch50.7 KBIgnoredNoneNone

#15

chx - February 8, 2007 - 16:47
Status:needs review» reviewed & tested by the community

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

forngren - February 8, 2007 - 22:00

Yes, patched tested and approved.

(subscribing)

#17

chx - February 9, 2007 - 07:45

Theme fix.

AttachmentSizeStatusTest resultOperations
menu_54.patch51.1 KBIgnoredNoneNone

#18

Dries - February 10, 2007 - 13:20

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

chx - February 10, 2007 - 13:52

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.

AttachmentSizeStatusTest resultOperations
menu_55.patch51.1 KBIgnoredNoneNone

#20

Dries - February 10, 2007 - 15:31

I don't understand. Can you explain this more/better?

#21

chx - February 10, 2007 - 17:46

No. Please read forum_load and forum_from_arg. You will see two functions that are absolutely different.

#22

chx - February 10, 2007 - 17:59

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

AttachmentSizeStatusTest resultOperations
menu_56.patch53.74 KBIgnoredNoneNone

#23

chx - February 10, 2007 - 20:33

This let me simplify the menu_translate function. Yay.

AttachmentSizeStatusTest resultOperations
menu_57.patch53.5 KBIgnoredNoneNone

#24

asimmonds - February 11, 2007 - 00:04

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

webchick - February 11, 2007 - 02:08
Status:reviewed & tested by the community» needs work

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.

AttachmentSizeStatusTest resultOperations
menu_58.patch54.11 KBIgnoredNoneNone

#26

webchick - February 11, 2007 - 02:10

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

webchick - February 11, 2007 - 02:24

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.

AttachmentSizeStatusTest resultOperations
menu_59.patch53.5 KBIgnoredNoneNone

#28

webchick - February 11, 2007 - 02:29
Status:needs work» needs review

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.

AttachmentSizeStatusTest resultOperations
menu_60.patch54.71 KBIgnoredNoneNone

#29

Dries - February 11, 2007 - 09:39
Status:needs review» fixed

Committah-ed! :)

#30

Anonymous - February 25, 2007 - 09:47
Status:fixed» closed
 
 

Drupal is a registered trademark of Dries Buytaert.