No navigation links on 404 pages

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.

Setting to review. Testbot was broken by an error in #361277: Remove the 'post settings' admin screen and relocate contents

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.

setting back to 'needs review' - testbot was broken.

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...

This issue is related to or a duplicate of #423992: Remove show_blocks page variable.

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 ;)

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...

This means, anyone not able to update to Drupal 7 will have to live with that bug? Please clarify. Thanks.

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.

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.

Looks good here

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.

Sub.

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.

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.

Fixed 404 tests.

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.

+++ 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.

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:

• no breadcrumbs are displayed
• no menu tree blocks are displayed
• the theme's main and secondary links are not displayed
• if you have access to the shortcuts bar, none of its links are displayed even thought the grey bar appears.

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

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.

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.)

Sub

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.

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.

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);

Sub.

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.

sub

+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.

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.

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)

Why not just generating the page as anon and caching it (like a 1-day cache)? Doesn't that solve the performance issue enough?

@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:

except to serve that page for auth users you'd still need full bootstrap but even with that [likely] an improvement.

@sun, what if we do serve an anon page to auth users too. *mean smile*?

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.

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).

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?

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

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 ?

the patch in #22 doesn't apply anymore. can you reroll it?

+++ 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.

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)

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

Subscribing.

Trying to remind myself why is this still on "needs work"

For those looking for a solution for this issue in Drupal 7, see the 404 Navigation module.

Here is a simple reroll of the patch in #44

is there an easy quick-fix for this issue in drupal7?

@dbinoj: There is the already mentioned http://drupal.org/project/navigation404 module

The easy quick-fix I just did was to create a page node and hook it up as the 404 page in the system information.

Rerolled #44 for Drupal 7.

Back to Drupal 8.

Would anyone be adversed to calling this a release blocker so that 8.0 might be the first Drupal release since 5.0 that didn't have problems with 404 pages?

Tagging novice for a re-roll.

I am looking into this issue - now that menu system and routing has been completely reworked in d8, should the same approach be used as in the old patch? I.e. should I create menu paths for 403 and 404 pages and redirect to them when necessary using menu_set_active_item?

I'm a first-timer in the mentored core sprint and will take a shot at this re-roll.

The re-roll failed, so I'm moving on to another issue.

For Drupal 8 I think this may no longer be a bug. At least, I'm not seeing it with this patch: #2256521: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module

So I'd suggest working on D7 mostly here and postponing for D8 until that issue is resolved and it can be verified this is still a bug.

The direction to go here is not clear now/yet, and the issue summary does not identify which tasks are good for new contributors. So removing Novice. per https://drupal.org/core-mentoring/novice-tasks

Also referenced here:
http://drupal.stackexchange.com/questions/53619/no-navigation-links-on-4...

Would be nice if there was a fix for #59.

@DamienMcKenna I'm not willing to call it a release blocker, but ya.. Lets at least call this major.

The patch in #59 had segments that were already included, so just eliminated them:
- includes/common.inc
- modules/node/node.test
- modules/simpletest/tests/menu.test
- modules/system/system.module
- modules/system/system.test

Just want this in D7 for testing purposes.

And back to D8.

This appears to be fixed in Drupal 8:

Moving to 7.

Reroll of the D7 patch in #70

Another reroll of #75 after #1003788: PostgreSQL: PDOException:Invalid text representation when attempting to load an entity with a string or non-scalar ID was committed.

Try https://www.drupal.org/project/navigation404

Ok, so the latest patch for this issue is "about a year ago". Can I ask why it hasn't been committed to the current version of D7?

@ExTexan I'm assuming it has not been committed since my last patch failed testing

Ahh, got it. Makes sense. Thanks for the quick reply.

This module https://www.drupal.org/project/navigation404 does not help.
User menu does not show up.
Pls advice.
Thank you!

Reroll of #77.

aerozeppelin, is it to me?
If so, how and what should I do with it?
Thank you!

@trk_ I re-rolled the patch from #77. If you feel like working on this patch, you can start by fixing the failing test cases. :D

Tried to fix failing tests from #77. The updates are in no way complete. Hoping someone will take this forward.

I can not. I do not understand in PHP programming.

I checked the last failing test. When the ?destination= is a system/403 or system/404 the page request should not be forwarded to these paths.

Patch #92 works great! Thank you! RTBC

+1

Am using panels everywhere with a custom site template and #92 works perfectly for me, includes all the items that were missing from the page not found screens.

I have reviewed the patch mentioned in comment #92. Looks good.

1. +        $page['#cache'] = array(
   +          'keys' => explode('/', $path),
   +          'granularity' => DRUPAL_CACHE_PER_ROLE,
   +          'expire' => CACHE_TEMPORARY,
   +          'bin' => 'cache_page',
   +        );
            print drupal_render_page($page);
   

   I do not understand what is going on here. Why are things being put in the page cache regardless of whether page caching is enabled on the site? And why are we assuming the page is cacheable by role? This looks like it will cache for authenticated users too... and there's no reason to assume that this page can't have user-specific content on it.

2. -    // If the destination is an external URL, remove it.
   -    if (isset($_GET['destination']) && url_is_external($_GET['destination'])) {
   -      unset($_GET['destination']);
   -      unset($_REQUEST['destination']);
   +    if (isset($_GET['destination'])) {
   +      // If the destination is an external URL or a 403/404 page, remove it.
   +      if (url_is_external($_GET['destination']) || in_array($_GET['destination'], array('system/403', 'system/404'))) {
   +        unset($_GET['destination']);
   +        unset($_REQUEST['destination']);
   +      }
   

   ....

   --- a/modules/shortcut/shortcut.module
   +++ b/modules/shortcut/shortcut.module
   @@ -647,7 +647,7 @@ function shortcut_preprocess_page(&$variables) {
      // shortcuts and if the page's actual content is being shown (for example,
      // we do not want to display it on "access denied" or "page not found"
      // pages).
   -  if (shortcut_set_edit_access() && ($item = menu_get_item()) && $item['access']) {
   +  if (current_path() != 'system/404' && current_path() != 'system/403' && shortcut_set_edit_access() && ($item = menu_get_item()) && $item['access']) {
   

   If we have to make all these random places in core aware of these new paths, that's a no-go and it suggests something else is wrong. The fix for this issue should be self-contained.

Overall this approach looks like it's turning out to be a bit complicated and I'm wondering if the earlier approach (to just address this within the menu system) would be better as a more targeted way to fix the bug.

Re-roll