Consider a menu item 'something/%foo'. This indicates that the router will rely on foo_load to handle requests that match. foo_load is not guaranteed to exist, however. The current behavior when foo_load does not exist is for the {menu_router}:path to be set as 'something/%foo'. This will never match, however, because menu_get_ancestors only returns paths in their simplified form, ie something/%. Thus we are recording unmatchable paths into the {menu_router}. The correct behavior when foo_load is missing is to not build that item and not record it to {menu_router} because it can't be served, due to the missing load (or _too_arg) function. Here's the patch:

diff -u -p -r1.255.2.16 menu.inc
--- includes/menu.inc	25 Jun 2008 08:49:25 -0000	1.255.2.16
+++ includes/menu.inc	2 Jul 2008 19:36:35 -0000
@@ -2218,6 +2218,7 @@ function _menu_router_build($callbacks) 
             $match = TRUE;
           }
         }
+        if (!$match) { continue 2; }
       }
       if ($match) {
         $parts[$k] = '%';
CommentFileSizeAuthor
#7 menutest.patch3.32 KBrobertdouglass
#5 menutest.patch2.91 KBrobertdouglass
#3 menutest.patch2.91 KBrobertdouglass
#2 menutest.patch2.91 KBrobertdouglass
menu.patch524 bytesrobertdouglass

Comments

pwolanin’s picture

pwolanin commented

needs a code comment - and maybe a test :)

robertdouglass’s picture

robertdouglass commented
Version: 6.x-dev » 7.x-dev
StatusFileSize
new menutest.patch2.91 KB

I've confirmed the bug exists in HEAD as well, so here's a commented patch and a test.

robertdouglass’s picture

robertdouglass commented
StatusFileSize
new menutest.patch2.91 KB

Missed a couple of details.

pwolanin’s picture

pwolanin commented

This looks like the queries are not correct?

 $count = db_result(db_query("SELECT count(*) FROM {menu_router} WHERE path = 'something/%/else'"));

should maybe be:

 $count = db_result(db_query("SELECT count(*) FROM {menu_router} WHERE path = 'something/%%/else'"));

note: %%

robertdouglass’s picture

robertdouglass commented
StatusFileSize
new menutest.patch2.91 KB

Yes, that's right. Thank you.

robertdouglass’s picture

robertdouglass commented
Status: Needs review » Needs work

Feedback from chx:

- code style (newlines after and before {} )
- move continue one line up so that foo/%/bar doesn't get mangled
- add foo/%/bar to the test.

robertdouglass’s picture

robertdouglass commented
Status: Needs work » Needs review
StatusFileSize
new menutest.patch3.32 KB

While it wasn't true that the 'foo/%/bar' path breaks with the proposed change I moved the !$match test up a line anyway. This makes it a more targeted case that only cares whether a _to_arg or _load function is missing. Code style corrected and 'foo/%/bar' test added.

pwolanin’s picture

pwolanin commented

code looks fine, test works. Not an urgent bug, but useful to fix for developers.

robertdouglass’s picture

robertdouglass commented

So, RTBC?

dries’s picture

dries commented

I think this looks like a good fix. I'm wondering though, whether it is in our interest to 'fail silently'. Wouldn't it be better to generate a warning or error message and to correct the developer?

damien tournoud’s picture

damien tournoud commented

Does that mean that we don't care to support path elements that start with a '%'?

Is it possible in the current implementation?

robertdouglass’s picture

robertdouglass commented
Status: Needs review » Needs work

@Damien Tournoud: Nothing in the API is changing with this patch. '%' paths are still supported as before. We were 'failing silently' while polluting the database before. Now we're failing silently without polluting the database. I'll roll another patch that adds a watchdog message.

robertdouglass’s picture

robertdouglass commented

Question with regards to #10: if I put a watchdog message in menu.inc, is there ever a bootstrap situation where menu.inc gets loaded but watchdog isn't available? I'll look into this, too. Are there any other reasons for or against failing silently?

damien tournoud’s picture

damien tournoud commented

@robertDouglass: sorry, I wasn't clear enough. I was talking about paths that would actually have a "%" in them. For example: mymodule/%C3%A0-la-claire-fontaine (that's the encoding of "à la claire fontaine"). Do we support them today?

pwolanin’s picture

pwolanin commented

@Damien - no, those are not supported today - at least not as actual paths. The arguments (trailing parts) can have any characters.

pwolanin’s picture

pwolanin commented
Status: Needs work » Needs review

I don't think we need a watchdog message - you would really only have these failures while developing a module.

I think the existing patch is fine - if we really want to give developers more info, perhaps we could use a flag like is done in _db_query:

  if (variable_get('dev_query', 0)) {

Though per Barry's DX post, we should think about having a define that we could perhaps use a few places:

  if (defined(DEV_DEBUGGING)) {
robertdouglass’s picture

robertdouglass commented

Honestly, I'm fine without any failure warning. There are probably hundreds of places where we could insert if (DEBUG) something in all of Drupal. It seems arbitrary to start here.

chx’s picture

chx commented
Status: Needs review » Closed (won't fix)

Opsie. Isn't this the usual "we do not babysit broken code"? This will be in the memory of every Drupal site out there and fire never unless you have written broken code but then you deserve a broken site.