Posted by fokke on March 13, 2008 at 3:22pm
30 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | menu system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | DrupalWTF, Performance |
Issue Summary
When I visit a page that does not exist, I expect to still see the primary and secondary links as on any other page.
But what I get is no navigation links.
The bug is in menu.inc on line #1209 and #1210:
<?php
// Get the menu hierarchy for the current page.
$tree = menu_tree_page_data($menu_name);
?>I can't understand why the choice for this method is even documented! The method that should be used IMHO is:
<?php
$tree = menu_tree_all_data($menu_name);
?>
Comments
#1
Came up with a slightly better solution.
It checks first to see if the menu path exists, in which case it uses the page-based menu tree. If the menu path doesn't exist (ie, we are at a 404), then it loads the entire menu tree.
#2
The last submitted patch failed testing.
#3
Setting to review. Testbot was broken by an error in #361277: Remove the 'post settings' admin screen and relocate contents
#4
Please also follow the discussion at the following locations before committing anything:
http://drupal.org/node/292565: Regression: login destination is broken (was: Search from 404 page does not work)
http://drupal.org/node/116895: Show regions at 404 page
http://drupal.org/node/423696: Allow display of menu links on 404 error page
http://drupal.org/node/423992: Change show_blocks page variable to allow arbitrary regions to be hidden.
#5
The last submitted patch failed testing.
#6
setting back to 'needs review' - testbot was broken.
#7
The last submitted patch failed testing.
#8
Marked #249380: Primary links disappear in page not found and #423696: Allow display of menu links on 404 error page duplicate with this. This really is a UI bug. Arguably, the 404 page is the one on your site where you need your primary links (in D7, known as the "main menu", whee!), to show up the most... Otherwise, it's a total dead-end for your site, with no help on how to actually find what it was you were looking for...
#9
This issue is related to or a duplicate of #423992: Remove show_blocks page variable.
#10
This issue is not relevant anymore: Drupal 7 do not hide blocks on 404 pages anymore. We might want to restore this behavior at one point, but at this time this is already fixed ;)
#11
Damien: I'm talking about menu links, not blocks. The primary/main menu links are gone at the 404 page in both D6 and D7. That's a bug...
#12
This means, anyone not able to update to Drupal 7 will have to live with that bug? Please clarify. Thanks.
#13
This is still a usability bug in D7 and D6. I should point out there's a contrib module that fixes this problem for D6 if you are experiencing this problem (and the #423992 problem) now. 404 blocks.
Here's a completely untested patch that should attack the problem at its root. Namely menu_tree_page_data() only returns a tree if the current path has a menu item found in menu_get_item(), which 404 pages do not.
#14
This is definitely a usability prolem and should be fixed. It's ever-so-confusing for naive users to have this happen to them. They suddenly get dumped on a page with no navigation. Sure, Drupalistas know what to do. But that doesn't make it right.
#15
Looks good here
#16
I marked #780930: Shortcuts disappear on 404 pages as duplicate of this.
***
For patch reviewers, note that (I think) almost all of this patch is indentation changes. The functional change is limited to this:
+ // Paths that don't have a menu item are 404 Not Found.+ if (!$item) {
+ // Build a minimal menu item to facilitate building the tree.
+ $item = array(
+ 'href' => '@TODO:--we--need--non--existant--path--here',
+ 'access' => 0,
+ );
+ }
which, er, we probably shouldn't commit with a @TODO as part of the href :) Is it wrong to use NULL or an empty string here instead? I think that would work fine.
Also, we need more code comments here to explain why we are building up the mock menu item in this way - e.g., we should explain that 'access' is FALSE so as to avoid trying to expand any menu items in the tree, and the 'href' must be non-existent so we get a unique key to store this in the cache with.
#17
Sub.
#18
Yes, this looks really too ugly
I think the real bug is here:
http://api.drupal.org/api/function/drupal_deliver_html_page/7
we could define actual default paths for 404, 403, etc.
#19
I tried David Rothstein's suggestion and it works great. But Peter's suggestion works great too, so this patch alters drupal_deliver_html_page to use two new system-module-defined paths, system/403 and system/404; also adds tests.
#20
Fixed 404 tests.
#21
The last submitted patch, no-menu-links-on-404-233807-20.patch, failed testing.
#22
The testRecentNodeBlock test in node.test was flawed. It checked for block visibility on a node/add/article page, even though the admin user did not have permission to access that path. Since "Access denied" pages didn't have a proper menu item before this patch, the block still showed because it saw the "node/add/article" path. With this patch, "Access denied" has a proper menu item, so its menu item overrides the fubar "node/add/article, but can't access it" menu item as the active path. IMO, this is how it should be, but I suppose that is debatable.
Here's an updated patch that fixes the node test.
#23
+++ includes/common.inc
@@ -2448,8 +2448,9 @@ function drupal_deliver_html_page($page_callback_result) {
// Standard 404 handler.
- drupal_set_title(t('Page not found'));
- $return = t('The requested page could not be found.');
+ $path = 'system/404';
+ menu_set_active_item($path);
+ $return = menu_execute_active_handler($path, FALSE);
@@ -2477,8 +2478,9 @@ function drupal_deliver_html_page($page_callback_result) {
// Standard 403 handler.
- drupal_set_title(t('Access denied'));
- $return = t('You are not authorized to access this page.');
+ $path = 'system/403';
+ menu_set_active_item($path);
+ $return = menu_execute_active_handler($path, FALSE);
I think that the short-circuited 404 and 403 pages happen purposively for performance reasons.
If you want fully-fledged pages, then I guess you just need to enter custom paths into the system configuration.
Powered by Dreditor.
#24
Performance reasons on a 404 page??? ;-)
The most pressing issue that a site owner has when a user lands on a "not found" page is trying to get that user to a REAL page. Short-circuiting the display of ALL your navigational links on a 404 page is counter to that purpose.
Currently when a user is on a 404 page:
This is really a continuation of the discussion started in #423992: Remove show_blocks page variable. If 404 page performance is a concern, then surely it should be up to the performance-sensitive websites to solve and not place the burden on the 99% of users who don't realize "oh, I have to configure a custom 404 to fix this".
As an alternative to calling
menu_execute_active_handler($path, FALSE);with the new system/403 or system/404 paths, we could instead call the system_not_found() and system_access_denied() functions directly.@sun If you are commenting in an attempt to delay committing any menu-related patch until after #907690: Breadcrumbs don't work for dynamic paths & local tasks #2 lands, I'm totally +1 for that. :-D
#25
Yes, performance highly matters. You can pretty easily wreck an entire site and "take it offline" by requesting arbitrary URLs in a brute-force manner, so the standard 404/403 pages are kept minimal.
#76824: Drupal should not handle 404 for certain files seems to be related, too, as it further cuts down certain 404 pages to not even show a page.
I'm not saying that it shouldn't be possible to have fully-fledged pages as 404/403, but (1) it shouldn't be the default, and (2) I'm not sure whether they cannot already be achieved by configuring 404/403 pages to point to a node or something.
#26
While this would save me some work when I create new drupal sites, I understand the concern raised by sun (requesting arbitrary URLs in a brute-force manner.)
As it stands now, I create new nodes for 403 and 404, as the default often messes up how the page looks, when the navigation is removed, but not the other elements of the page (header, basically.)
#27
Sub
#28
In regards to #25, how is this any different from requesting a valid URL lots of times? What mechanism is preventing, say, the home page from being DoS'd, but not the 404 page? If it's a caching mechanism, why can't the 404 page be cached? If it's cached by URL, couldn't 404 be a special case that always pulls the 404 page cache? I'm not following the logic that 404 pages need to be easier to render than other pages.
#29
Create your own 404 page and point to it at admin/settings/error-reporting
Can't you just create your own 404 page and have 404's redirect by setting that respective path? Where I work, we have it set to display a "Didn't find what you were looking for?" form to the user that allows them to contact/notify us of the problem.
#30
#31
Tor Arne Thune's solution for D7 works for me.
menu.inc; line 979 (Drupal 7.2)
change: $tree = menu_tree_page_data($menu_name);
to: $tree = menu_tree_all_data($menu_name);
#32
Sub.
#33
I have similar poblem with my website. Please have a look at this link http://trekronerfestival.dk/qvik-kontakt , it will bring you to "Page not found" message. you will find that main menu links disappear.
I would like to create my own 404 error node. I could not find, where can create my own node for this error.
Please help.
#34
sub
#35
+1 to make this a regular page by default. A white screen of death has great performance too :) I'm not sure how much this short-circuits the regular page loading process but like #28 says caching could be used along with early detection to minimize the load.
#36
page cache should be enough for this, maybe even building it as anonymous so that even logged users get a cached result could be an option.
#37
I understand the logic of making the 404 page as fast as possible but ideally the page should have as much as possible the same content as any other standard page. I would say that the 404 page could look like a broken feature if you don't know the current implementation's background. (subscribing)
#38
Why not just generating the page as anon and caching it (like a 1-day cache)? Doesn't that solve the performance issue enough?
#39
@franz: Very good question and suggestion.
A full but cached 404 page is most likely even much faster than the current non-cached but cut-down 404 page.
In IRC, @catch only raised one possible conflict:
#40
@sun, what if we do serve an anon page to auth users too. *mean smile*?
#41
This patch was tested. I got JohnAlbin latest patch, re-rolled and added the cache. I've set granularity to role-based, should be enough to avoid any bugs for now. I'm not sure the 'keys' stuff is appropriate.
However, this obliterates another patch that had modified the 404 message to display the path searched (I removed that because it gets cached).
Some benchmarking would do great here.
#42
The last submitted patch, 233807-404-normal-page-cached.patch, failed testing.
#43
Ok, it was dumb to apply same logic to 403, I'm reverting this.
Also, I'm handling the bug with custom 404 (they're not displaying because the default 404 page is on cache).
#44
This should pass.
On Breadcrumbs, they now exist (as a single Home entry) on 403 - and I didn't change JohnAlbin patch on that, so I think that test was new. I changed it to assert for "Home" only. Not sure if this is really the expected, but since it is the expected for "admin/*" pages, why not?
#45
I've quickly tested and it seems to be nicely working. I've also did for the first time a little benchmark, here is the result:
404 test command: ab -c1 -n500 http://localhost/d8/xyz
403 test command: ab -c1 -n500 http://localhost/d8/admin
"with cache" mean that i checked "cache pages for anonymous users" and "cache blocks" in admin/config/development/performance
before patch
404 / without cache
Median processing time (ms): 198
403 / without cache
Median processing time (ms): 199
404 / with cache
Median processing time (ms): 41
403 / with cache
Median processing time (ms): 41
After patch
404 / without cache
Median processing time (ms): 191
403 / without cache
Median processing time (ms): 202
404 / with cache
Median processing time (ms): 32
403 / with cache
Median processing time (ms): 32
#46
Now, that's interesting, the cache I applied to 404 didn't improve much (~ 5%). Well, I guess the point wasn't improving in the first place. Could you care to benchmark the patch on #22 ?
#47
the patch in #22 doesn't apply anymore. can you reroll it?
#48
+++ b/includes/common.inc@@ -2494,12 +2494,21 @@ function drupal_deliver_html_page($page_callback_result) {
+ $page['#cache'] = array(
+ 'keys' => explode('/', $path),
+ 'granularity' => DRUPAL_CACHE_PER_ROLE,
+ 'expire' => CACHE_TEMPORARY,
+ 'bin' => 'cache_page',
+ );
Just remove these lines and benchmark.
5 days to next Drupal core point release.
#49
I've tried with the change in #48 but it didn't had any effect. The benchmark is using 500 times the same url, i think this modification would only have effect if the test would access a random url. (32ms, 404 with cache)
#50
I used siege on a lighttpd server (my local), with 500 different urls (all invalid). No page cache enabled.
Drupal 8.x current: 0.25 secs
Drupal 8.x with nice 404 pages but without cache (#22): 0.27 secs
Drupal 8.x with nice 404 pages with cache (#44): 0.20 secs
#51
Subscribing.
#52
Trying to remind myself why is this still on "needs work"
#53
For those looking for a solution for this issue in Drupal 7, see the 404 Navigation module.
#54
Here is a simple reroll of the patch in #44
#55
The last submitted patch, 404-normal-page-cached-233807-54.patch, failed testing.