If your module supports an arbitrary number of arguments and you want to verify that such a path is valid then you can't really do that because load functions get one argument. Now, you can't just add arguments to load because node_load will break. Instead we make this new functionality possible:

function gallery2_menu () {
  $items['gallery/%gallery2'] = array(... );
}

function gallery2_menu_map(&$args) {
  // For gallery/album1/album/pic1 $args is array('gallery', 'album1', 'album', 'pic1')
  // Check whether this is a valid gallery2 path and if not set $args to FALSE.
}

Currently you need to do evil hacks and even then you will be dependent on $_GET['q'] which makes it impossible to add a menu link to such a path. The patch has a zero performance impact -- it adds very few lines of code (11) and to the normal execution path we have added one single if() command, nothing else. Even the condition that the if runs is already there.

If this gets in I will update the documentation -- but not the doxygen. It's very rarely used (but when you need it, you really do) and would just confuse people.

Core has no such thing, only search supports arbitrary number of arguments and it does not need to validate them.

CommentFileSizeAuthor
#28 menu_map-177488-28.patch3.84 KBchx
#15 menu-map-177488-15.patch2.94 KBchx
#8 menu_load-177488-8.patch2.21 KBchx
#7 multi-args-177488-7.patch6.36 KBpwolanin
#2 menu_map-177488-2.patch2.91 KBchx
#1 menu_map-177488-1.patch5.32 KBchx
menu-map.patch2.84 KBchx

Comments

chx’s picture

chx commented
StatusFileSize
new menu_map-177488-1.patch5.32 KB

Actually, core has such a thing: profile categories. http://drupal.org/node/111481 I have fixed that issue depending on this one. And also, search can be slightly small if this gets in. The attached patch fixes a notice in the previous one and adds a comment in the code.

chx’s picture

chx commented
StatusFileSize
new menu_map-177488-2.patch2.91 KB

I splitted the patch between here and the linked issue.

profix898’s picture

profix898 commented

@chx: I didnt have time to join irc, so I hope you got my last mail about gallery_menu module. Looks like your session on this was quite productive. I will take time to review this patch shortly. Thanks a lot.

pwolanin’s picture

pwolanin commented

@chx - could you please explain the logic of the code in more detail and/or add more code comments?

While I get the gist of what you're doing, this seems less than ideal. There seem to be other ways around this.

For example - if a single load function needs several args, you can put additional % elements in the path, and then get the values in the load function using arg().

Also, there seems to be a bug somewhere else in the code initially saving the load functions -
for example, in my DB in {menu_router} the path book/export/%/% has the load_function value of a:2:{i:2;N;i:3;N;}

chx’s picture

chx commented

book/export/%/% indeed reads as array(2 => NULL, 3 => NULL) regardless of my patch:

      if (preg_match('/^%([a-z_]*)$/', $part, $matches)) {
        if (empty($matches[1])) {
          $match = TRUE;
          $load_functions[$k] = NULL;
        }

so, a bare percent sign will create a NULL.

This might not be needed any more, actually, but that's another issue. Regarding comments,

If this gets in I will update the documentation -- but not the doxygen. It's very rarely used (but when you need it, you really do) and would just confuse people.

Finally, you must never use $_GET['q'] and friends in a load function because when you try to add a menu link to such a path then menu_get_item will run and you will get a nice 'path does not exist' error.

pwolanin’s picture

pwolanin commented
Status: Needs review » Needs work

Yes - sorry to confuse the issue - the book/export/%/% type issue is not a conequence of the patch, just something I noticed and probably needs a separate issue.

So, I don't really like this patch because it introduces a rather hackish flag of whether or no that column is serialized, and so on.

I see that you're right that a load function should never depend on arg(). So what's a clean alternative? One possibility - the prototype for all load functions could be something like:

function myobject_load($id, $map) {

}

this would mean we need to wrap node_load() in another function, but would be a cleaner general solution, I think.

pwolanin’s picture

pwolanin commented
Status: Needs work » Needs review
StatusFileSize
new multi-args-177488-7.patch6.36 KB

thinking a little more - we need the $index as well as the $map

chx’s picture

chx commented
StatusFileSize
new menu_load-177488-8.patch2.21 KB

Let's use an optional _menu_load function instead. %node_nid is not intuitive.

pwolanin’s picture

pwolanin commented
Status: Needs review » Reviewed & tested by the community

ok - I like this one - it lets up keep %node while minimizing the changes to the menu code and still enabling a fix for the profile module bug.

Code tested and works fine.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Needs review

Now this seems like an API break for those, who already started to post modules and depend on some load function not taking only one argument, right?

pwolanin’s picture

pwolanin commented

@Gabor - a load function that only take a single argument will still work fine (e.g. user_load()) - it's only the rare cases like node_load() that take additional, different arguments that would have any problem. This patch provides a simple way around for them - see the node_menu_load() example.

chx’s picture

chx commented
Status: Needs review » Reviewed & tested by the community

yes it's an api break indeed, sorry -- i can not really help it. the menu_map in #2 provides a non api break solution but it's far less elegant.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Needs work

We need more code comments in this patch.

Also, the API is not 100% to my liking. The difference between _menu() and _menu_load() is not clear. Can we think of better names? Can we think of a better API?

pwolanin’s picture

pwolanin commented

@Dries - so here are what I see as the 3 broad goals:

1) provide the full path and index information to the load function
2) have a fairly simple/obvious system of function naming for the load functions
3) be able to reuse as many existing load function as possible (this may be dispensible)

so, unfortunately #1 and #3 conflict for node_load() because it takes additional arguments. i think it might make the most sense in terms of a clean API to actually require a distinct name for all load functions, and not try to re-use existing functions - i.e. ditch #3. In the patch there is "X_menu_load". I'm not sure what would be more obvious? "X_callback_load", "X_menupath_load", "X_pathmap_load", "X_menutree_load".

So, in several cases these would just be simple wrappers around an existing load function (as with node_load in the patch). If you look at the linked issue for profile module, I've already had to introduce more logic than is handled by user_load(), so I used user_profile_load().

chx’s picture

chx commented
Status: Needs work » Needs review
StatusFileSize
new menu-map-177488-15.patch2.94 KB

This patch is another way. It needs commenting and the user part removed to the other issue but it serves as an example.

pwolanin’s picture

pwolanin commented

hmm, this could work, but I still prefer something like #8 where we just pass all the needed info to the load function.

pwolanin’s picture

pwolanin commented

so, just to clarify, it seems there are at least 4 separate approaches proposed to fix this bug. So, more feedback fmor Dries/Gabor is needed.

a quick summary:

  1. the approach in #1/#2 above - swtich on whether the field is serialized, etc.
  2. the approch in #8 - 2 possible naming schemes for the load function
  3. suggested in #14 (no actual patch) - variant of #8 where we rename all load function so we have only a single naming scheme.
  4. the approach in #15 where we have a mechanism for the load function to pull in the current map

I don't like the 1st at all. The 4th is a bit weak too since then the load function doesn't know it's index and thus cannot be re-used for different menu paths where the load function may appear at different positions in the path.

So, my preference among the above is 3,2,4,1

chx’s picture

chx commented

The approach in #15 definitely knows about the index it returns array($index, $map). I really like the reusability of the load functions so I would say my order of preference is the exact opposite of pwolanin's: 1, 4, 2, 3.

pwolanin’s picture

pwolanin commented

well in that case, my preference is really "any of these except #1"

chx’s picture

chx commented

Well, then our choice should be #4 because Dries did not like #2 and I much do not like #3.

so, please review the latest patch :)

chx’s picture

chx commented
Title: Regression: arbitrary number of arguments in router path are not supported » Regression: arguments containing the the argument delimiter are not supported

I told eaton that in user/1/edit/this/is/a/problem , this/is/a/problem is one argument and that's the problem and then he came up with this title.

pwolanin’s picture

pwolanin commented

was this supported in D5? I'm trying it now, and adding slashes to the profile name breaks in D5.

chx’s picture

chx commented

that may be but the other use of this patch is gallery2 which is more or less unportable at this moment. also, with !$may_cache you had ways to deal with this bug, it's a whole another issue that this was a bug in D5 but in D5 you *could* deal with it.

profix898’s picture

profix898 commented

@chx: I wrote you an email about 2-3 days ago that I managed to port gallery_menu module. I found a simple way to 'hack' G2 (plain query against the db instead of using G2 API), so that the module can be ported to D6 using 'normal' static menu items and callbacks. However it requires a call to menu_rebuild() if an album changed in G2. It would be great to have an additional parameter for menu_rebuild to rebuild the menu items of a certain module only (reduces overhead of calling menu_rebuild()), e.g. menu_rebuild('gallery') refreshes the menu items of 'gallery' module only. Otherwise it is working without this patch now.

chx’s picture

chx commented
Title: Regression: arguments containing the the argument delimiter are not supported » Regression: arguments containing the argument delimiter are not supported

Gallery might work but it's an ugly quirk to mandate menu_rebuild -- but user categories still do not work... the latest patch is really not evasive API wise.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Needs work

Well, I looked at the patch, but there is nothing to help me understand how menu_get_object_map() and menu_set_object_map() are useful / should be used. There are no docs. user_category_load() should also be sprinkled with docs on why this is done, what is the task, why is this the solution?

I think it is a good idea to keep the current naming scheme, and distance ourselfs from thinking about introducing new APIs or changing existing ones.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

chx asked for clarification. I tried to explain that the latest patch looks reasonable, because it does not modify any forward facing APIs. The object map code it adds and uses however is quite obscure without documentation, so let's see some explanation (phpdoc on all functions added) in the code to see the picture better. The user_category_load() is also in need of some inline comments to make it clear what it does and why.

chx’s picture

chx commented
Status: Needs work » Needs review
StatusFileSize
new menu_map-177488-28.patch3.84 KB

I mentioned above that if this patch will be chosen, I will add comments so there. The user part is still in, I guess we can fix http://drupal.org/node/111481 in one commit otherwise this patch makes no sense.

chx’s picture

chx commented
Status: Needs review » Fixed

http://drupal.org/node/184022 fixed this sorry mess.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)