I am trying to define my own wildcard loader function and noticed that if I use a number as part of the wildcard loader name then it does not get called.

This below doesn't work. My function s3_object_load() does not get called, and nor does s3_object_page(). I end up with a 'page not found':

// in hook_menu:
$items['amazon/%s3_object/view'] = array(
    'title' => 'View S3 object',
    'page callback' => 's3_object_page',
    'page arguments' => array(1),
    'access callback' => 'user_access',
    'access arguments' => array('administer s3'), 
  );
}
// end of hook_menu snippet

// Loader function I want to use, but never gets called
function s3_object_load() {
  
}

The code below does work (the loader function gets called, and so does s3_object_page()):

// in hook_menu:
$items['amazon/%s_three_object/view'] = array(
    'title' => 'View S3 object',
    'page callback' => 's3_object_page',
    'page arguments' => array(1),
    'access callback' => 'user_access',
    'access arguments' => array('administer s3'), 
  );
}
// end of hook_menu snippet

// Loader function that works
function s_three_object_load() {
  
}

Note that the only change made was renaming the loader function from 's3_object' to 's_three_object'. I suspect there is a problem somewhere in the path parsing caused by the inclusion of digits in loader function names.

Any ideas?

CommentFileSizeAuthor
#13 empty-or-valid-callback-285165-11.patch923 bytespwolanin
#12 285165-wildcard-fix.patch879 bytesdamien tournoud
#6 menu-inc-wildcard-loader-pattern.patch966 bytesgábor hojtsy
#1 menu_object_loader.patch735 bytesheine

Comments

heine’s picture

heine commented
Status: Active » Needs review
StatusFileSize
new menu_object_loader.patch735 bytes

You are correct. The replacement regular expression follows http://www.php.net/functions

damien tournoud’s picture

damien tournoud commented
Version: 6.3 » 7.x-dev
Status: Needs review » Reviewed & tested by the community

Looks quite reasonable, but should go to 7.x first. The same patch should apply to 7.x (Heine, can you confirm?).

This is quite a small fix, so submitting "as is" for Dries review.

lilou’s picture

lilou commented

Patch applied successfully to CVS HEAD.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs work

Ha, that's a fun bug.

Can we please have a comment or two to explain what the hell that scary regex does? :)

damien tournoud’s picture

damien tournoud commented

The comment should cite http://php.net/manual/en/language.functions.php, which document that regexp.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new menu-inc-wildcard-loader-pattern.patch966 bytes

My issue on #321040: QUICKFIX: menu system does not allow numbers and uppercase in wildcard names was marked as a clear duplicate of this one. So let's get this fixed :) The attached patch includes comments and works for me with wildcards such as %l10n_community_language and %l10n_community_project.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks for the documentation.

gábor hojtsy’s picture

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

Note that Dries also committed it to 6.x.

pwolanin’s picture

pwolanin commented
Priority: Normal » Critical
Status: Fixed » Active

this regex is wrong - it just broke a large fraction of all existing menu paths

that only matches if the % is followed by at least one char

pwolanin’s picture

pwolanin commented

here's one possible fix:

 if (preg_match('/^%$|^%([a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*)$/', $part, $matches)) {
chx’s picture

chx commented

%([a-zA-Z_][a-zA-Z_0-9]*)? do not overcomplicate. this plus _load is a PHP identifier. Starts with a letter or an underscore, continues the same plus numbers.

damien tournoud’s picture

damien tournoud commented
Status: Active » Needs review
StatusFileSize
new 285165-wildcard-fix.patch879 bytes

Oups, here is a sane fix.

pwolanin’s picture

pwolanin commented
StatusFileSize
new empty-or-valid-callback-285165-11.patch923 bytes

patch against D6, applies w/ offset to D7.

chx’s picture

chx commented
Status: Needs review » Needs work

Why so complicated, sweet heavens. %$ is not needed, it's going to cause you notice issue. Use the ^%([whatever][whatever0-9]*)?$ notation.

chx’s picture

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

Oh Damien's is a different! That's the good one. #12.

damien tournoud’s picture

damien tournoud commented

Full test suite:

6983 passes, 0 fails, 0 exceptions

pwolanin’s picture

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

by way of explanation, chx liked Damien's better since $matches[1] will be set still (though empty) for a plain %

pwolanin’s picture

pwolanin commented
Status: Needs review » Reviewed & tested by the community
dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

gábor hojtsy’s picture

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

Since Dries did not commit the followup in #12 to Drupal 6 this time, I did. So this is also closed for Drupal 6.

Anonymous’s picture

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

Automatically closed -- issue fixed for two weeks with no activity.