When I try to create a link: www.mysite.com/404 in a hook_menu() implementation, I get the not found page (which is another 404 page, not my custom). This also happens with any paths I try, which only contain digits.

CommentFileSizeAuthor
#16 2456193-16.patch2.7 KBpoker10
#16 2456193-16_test-only.patch1.11 KBpoker10
#15 patch_apply.png44.64 KBvikashsoni
#13 numeric-menu-item-paths-2456193-13.patch1.55 KBpsha
#7 numeric-menu-item-paths-2456193-7.patch1.54 KBpsha
#1 numeric-menu-item-paths-2456193-1.patch790 bytespsha

Comments

psha’s picture

psha commented
Status: Active » Needs review
StatusFileSize
new numeric-menu-item-paths-2456193-1.patch790 bytes

This patch fixes the issue by changing the menu.inc core file.

Status: Needs review » Needs work

The last submitted patch, 1: numeric-menu-item-paths-2456193-1.patch, failed testing.

kalinchernev’s picture

kalinchernev commented

Problem reproduced, patch working correctly.

kalinchernev’s picture

kalinchernev commented
Status: Needs work » Reviewed & tested by the community
dcam’s picture

dcam commented
Version: 7.35 » 7.x-dev
Status: Reviewed & tested by the community » Needs work

No. A patch with 388 test failures and 37 exceptions is not working correctly.

@psha
Thank you for your contribution! In the future, please set the issue version to the *-dev version to ensure the patch is tested against the latest code.

dawehner’s picture

dawehner
Primary language German
commented

As a workaround you can probably use a path alias for this particular usecase.

psha’s picture

psha commented
StatusFileSize
new numeric-menu-item-paths-2456193-7.patch1.54 KB

Thanks for the response. I also noticed multiple practical bugs. I hope this one works.

psha’s picture

psha commented
Status: Needs work » Needs review
kalinchernev’s picture

kalinchernev commented

Works well for me, and all tests pass, good job! :)

kalinchernev’s picture

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

changing the status, because the tests are passing and the code changes seem safe enough for me

David_Rothstein’s picture

David_Rothstein commented
Status: Reviewed & tested by the community » Needs review 
-      $callbacks = array_merge($callbacks, $router_items);
+      $callbacks += $router_items;

Hm, I'm not sure we want to do this. It means that previously the new items took precedence, but now the existing ones do. In general people probably shouldn't be defining the same menu item in more than one module, but I bet it happens and we could introduce some weird bugs this way.

Can't we do it in a way that doesn't change the precedence?

David_Rothstein’s picture

David_Rothstein commented

Also, has anyone checked whether this issue affects Drupal 8 too?

psha’s picture

psha commented
StatusFileSize
new numeric-menu-item-paths-2456193-13.patch1.55 KB

This change produces the same array values as array_merge, except for the order of the elements, but the array will get sorted anyways.

kalinchernev’s picture

kalinchernev commented

@David_Rothstein I don't think the issue is relevant to Drupal 8. The routing definitions are key:value pairs of strings

vikashsoni’s picture

vikashsoni commented
StatusFileSize
new patch_apply.png44.64 KB

Thanks for the patch it's working fine for me

poker10’s picture

poker10 commented
Issue tags: +Needs performance review
StatusFileSize
new 2456193-16_test-only.patch1.11 KB
new 2456193-16.patch2.7 KB

The patch from #13 still applies correctly to D7 HEAD and tests are passing. I think this needs a test so the issue can be considered by maintainers. I am adding a simple test that verifies if a numeric link is accessible. Patch itself is unchanged (except that I tweaked the comment a little bit).

And to address these concerns about the performance: #1177656-5: Support for top-level, numeric menu paths, I will try to test the patch on some sites with several thousands menu items to see if the performance differs.

The last submitted patch, 16: 2456193-16_test-only.patch, failed testing. View results

poker10’s picture

poker10 commented
Issue tags: +Needs change record

Status: Needs review » Closed (outdated)

Automatically closed because Drupal 7 security and bugfix support has ended as of 5 January 2025. If the issue verifiably applies to later versions, please reopen with details and update the version.