Hi all!
I'm proud to present this patch, it does some simple but yet very useful changes to the error reporting system. Basicly this is a version of Search404 (http://drupal.org/node/26010) on steroids, with some enchantments. All features from Search 404 are kept. It has been rewritten to be more stable, secure and fast than before. New features has been added:

  • The error message is now configurable
  • Three special paths has been added;
    • <blank> - It's the classic Drupal 404 page, just an error message
    • <front> - Invokes the frontpage - with an error message set to $message
    • <search> - This is what Search 404 accomplishes and is also the standard 404 page if nothing else is specified. It performs a search on the words from the requested URL. The options to configure Search 404 remains:
      • Display a link instead of the search results directly. This feature is particular useful when many bots is hitting the site.
      • Jump directly to the search result when there is only one result.
      • Use OR between keywords when searching.
      • A list of words that should be ignored (php, html etc)
    • Of course any Drupal path can still be specified
  • There's now an option to send "404 Not found" header or not
  • Brand new 403-handling:
    • Can use <blank> and <front>, but also <login> which will display a login form.
    • Configurable error message and option to send "403 Access denied" header

Care has been taken to not create loops, such as the user don't have access to the 404 page, and the 403 page itself isn't found.

One reason why this should be in core, is that error-handling surely is someting the CMS itself should take care of.

How to test:
1a) Visit: http://forngren.com/fantastic40x/
1b) Make a fresh Drupal HEAD install, and make sure that everything seems ok. Apply the patch attched
2) Fool around with the site
3) Report results back in this issue

Links
http://drupal.org/node/26010 Search 404

Posted by geokker - November 7, 2004 - 11:56 http://drupal.org/node/12668
I can't believe how many sites miss this trick. I was looking for a radio programme 'Loose Ends' on the BBC website. I tried www.bbc.co.uk/radio/looseends and got a 404.

Why? How difficult would it be to put the non-matching part of the URL into a search and bring up 'Sorry, we couldn't find an exact match - did you mean...' then list trip_search results or something.

If anyone can figure this out, it should go into core Drupal functionality.

A good CMS has an excellent search facility.

A good site has this search facility on every page.

When I'm looking for something on a site, as soon as it renders I instantly judge it. If I can't quickly see a search box - I hate it. If there is a ton of content on the front page - I'm not a fan. If the search is slow, or doesn't yield coherent results - I'm dissapointed. If it's Flash - I'm not impressed.

http://drupal.org/node/7570 Diskussion about "Custom 404 Error Page"

Sideeffect
There is a tiny sideeffect, though. After using this for a few days on a test site with 6 nodes, I stopped using the navigation and just typed what I was looking for in the URL. I think that Bert Boerland can confirm this.

Final words, contact info and more
First of all, none of this would be possible if it wasn't for the work done by Lars Sehested Geisler/larssg, the maintainer of Search 404, and Bert Boerland, which has been a great support during the process of writing, testing and documenting this patch. Since I've summerholidays I can usually fix bugs/errors within a few hours (awake between 10-24 GTM +2). If you want to discuss this patch, please, do so. I'm in #drupal when I'm awake. You can also use my contact form (http://drupal.org/user/40238/contact) or skype me (forngren).

Cheers!

Comments

Status:Active» Needs review

Btw, some errors appears:

    * user warning: Table 'temp_search_sids' already exists query: CREATE TEMPORARY TABLE temp_search_sids SELECT i.type, i.sid, SUM(i.score * t.count) AS relevance, COUNT(*) AS matches FROM search_index i INNER JOIN search_total t ON i.word = t.word INNER JOIN node n ON n.nid = i.sid INNER JOIN users u ON n.uid = u.uid WHERE n.status = 1 AND (i.word = 'stoomboot') AND i.type = 'node' GROUP BY i.type, i.sid HAVING COUNT(*) >= 1 in /home/forngre/public_html/fantastic40x/includes/database.mysql.inc on line 128.
    * user warning: Table 'temp_search_results' already exists query: CREATE TEMPORARY TABLE temp_search_results SELECT i.type, i.sid, 5 * (2.34496700469 * i.relevance) + 5 * POW(2, (GREATEST(n.created, n.changed, c.last_comment_timestamp) - 1154980734) * 6.43e-8) + 5 * (2.0 - 2.0 / (1.0 + c.comment_count * 1)) AS score FROM temp_search_sids i INNER JOIN search_dataset d ON i.sid = d.sid AND i.type = d.type INNER JOIN node n ON n.nid = i.sid LEFT JOIN node_comment_statistics c ON c.nid = i.sid WHERE (d.data LIKE '% stoomboot %') ORDER BY score DESC in /home/forngre/public_html/fantastic40x/includes/database.mysql.inc on line 128.

I'm not sure if those is because of the patch or Drupal itself, probably the first.

Title:Better error-handlingBetter 404 and 403 handling

Component:system.module» base system
StatusFileSize
new9.24 KB

Due some recent discussions (e.g. http://drupal.org/node/76824), I've decided to split this patch in three smaller ones; the one attached, a 403 patch and 404 patch.

This patch does some improvements to the error reporting settings:

  • Features added
    • Changed help texts
    • Added configurable error messages
    • Added option to send messages or not
    • Added option to send http 403/404 headers or not
  • Bugs squezed
    • Try to create a "error page loop", i.e. set the 403 page to a path that doesn't exist or the 404 page to a path that user don't have access to. You'll get "2" or "3" as content

I think this code is pretty robust, so it's mainly spelling and coding standards that needs to be checked.

Cheers Johan Forngren

Status:Needs review» Needs work

1. if (empty($return) || $return == MENU_NOT_FOUND || $return = MENU_ACCESS_DENIED) {

Should that be '=='?

2. Incorrect use of t().

3. +    $return = '&nbsp;'; is ugly.

4. Why the various extra variable_set()s? I vote against these.

StatusFileSize
new9.24 KB

1. if (empty($return) || $return == MENU_NOT_FOUND || $return = MENU_ACCESS_DENIED) {
Should that be '=='?

Fixed

2. Incorrect use of t().

Not quite sure what you mean.

3. + $return = ' '; is ugly.

Agreed, but I didn't wanna leave it blank/" " since I think that will breake some templates, how about setting the message as $return?

4. Why the various extra variable_set()s? I vote against these.

Not quite sure here either, do you mean the settings I added? In that case, I'm not sure howto remake them, or should some of them be removed?

(your opinons/review are greatly appreciated)

about the use of t():
you have hard coded html within your strings, and what's worse they are not compliant with xhtml standards (<i> is deprecated, you don't have any <p> around your paragraphs, etc.).
Check http://api.drupal.org/api/HEAD/function/t.
I am not sure what the complete solution would look like, but this gives you a small clue to know where to start.
Can someone more competent complete what I am saying?

Status:Needs work» Needs review
StatusFileSize
new9.26 KB

Finally, here's the updated patch. I still use some (x)html within the code;

<?php
'#description' => t('These settings apply when the requested document could not be found, i.e. a <em>404 error</em>.'),
?>

But I think that's cleaner than:

<?php
'#description' => t('These settings apply when the requested document could not be found, i.e. a %404_error.', array('%404_error' => t('404 error'))),
?>

3. + $return = '& nbsp;'; is ugly.
4. Why the various extra variable_set()s? I vote against these.

Still not fixed

...and what's worse they are not compliant with xhtml standards ( is deprecated, you don't have any

around your paragraphs, etc.)...

Switched to <em>, see above

Thanks to unconed, dries, killes and especially webchick for explaining things.

(sorry for the messy post)

StatusFileSize
new9.32 KB

Keeping w/HEAD.

Because of http://drupal.org/node/76824 I think the need for this patch increased, so we don't just "blank" the page on every 404.

(I did also take down the test site cause noone visited/used it)

Status:Needs review» Needs work

Why would anyone want to disable the 404 HTTP header? I do not think that should be a setting.

Status:Needs work» Needs review
StatusFileSize
new8.49 KB

Rerolled without the ability to toggle headers...

Trying to fix html...

StatusFileSize
new8.54 KB

Keeping w/HEAD. People, if you want to see all of the above stuff in core, please make a review of this patch so I can focus on the rest of it. Thanks!

I think the default 404 message is too technical. People don't care whether URLs of a website are case-sensitive. Plus, it is not necessarily true.

Can we create a new issue for this? The entire page is screwed up because invalid HTML usage. Thanks.

I think the default 404 message is too technical.

I'm affraid that I've done it as user-friendly as could, I must ask someone else to step in and help me or point me in the right direction.

People don't care whether URLs of a website are case-sensitive. Plus, it is not necessarily true.

Try:
http://drupal.org/node/1
http://drupal.org/NODE/1
http://drupal.org/?q=node/1
http://drupal.org/?q=NODE/1

Can we create a new issue for this? The entire page is screwed up because invalid HTML usage. Thanks.

Can we wait until the first part of this patch gets in?

Cheers Johan Forngren

StatusFileSize
new8.46 KB

Updated 404 message to "We are sorry, the requested page (%path) was not found on this webserver. Either the URL does not exist or the page you were looking has been deleted."

Status:Needs review» Needs work

- Please drop the %path functionality.

- In 'Enable error message', 'Enable' seems redundant. Or maybe it should be 'Display'.

- It is not clear how this works. What happens with the error message when I set an alternate 404 page. Even so, what is the added value? People can already customize the message through an alternate 404 page. The texteare is easier to use, but maybe we should eliminate the 'Alternate 404 page' feature then. Right now, it is not clear how the textarea and the alternate 404 page mix.

Status:Needs work» Needs review
StatusFileSize
new7.92 KB

- Please drop the %path functionality.

- In 'Enable error message', 'Enable' seems redundant. Or maybe it should be 'Display'.

- It is not clear how this works. What happens with the error message when I set an alternate 404 page. Even so, what is the added value? People can already customize the message through an alternate 404 page. The texteare is easier to use, but maybe we should eliminate the 'Alternate 404 page' feature then. Right now, it is not clear how the textarea and the alternate 404 page mix.

Droped, changed, explained, postponed :p

There are still two mechanisms to configure these? :S

Ok, here's the deal;

There are 6 options in total, 3 for 403, and a copy of those for 404. The options are:

Page to display:
[TEXTFIELD] This page is displayed when a 403 error occurs. If you are not using clean URLs, specify the part after "?q=". If unsure, leave blank.

This is the 'classic' 403 setting, i.e. what page users should be redirected to when a 403 occurs.

Display error message
[Checkbox] Enabling this feature will display the message below when a 403 error occurs, even if "Page to display" is set.

When this is checked, the message below will be shown (drupal_set_message), independent if "Page to display" is set or not

Message:
[textarea]

The message, see above

(sorry for messing up the xhtml even more, but as you said; it's fubar. Hope you'd understand)

Status:Needs review» Needs work

Looks okay, but I think the language could use some work.
- 404 and 403 are HTTP codes, not meant for people. Go ahead and leave '40x - ' out.
- There aren't really any 'files', should be 'Page not found.'
- 'web server' is not one word.
- Don't abbreviate 'admins'

'Other error reporting' has a weak name and groups two unrelated fields together. The fields can be left without a fieldset.

Status:Needs work» Needs review
StatusFileSize
new8.35 KB

Fixed all of drumm's issues above.

I have also created new default messages, but I'm not sure about the t() use, see below. They have been discused in irc and some native English speakers (sorry, don't recall names) have aproved it.

Cheers Johan Forngren

<?php
  $form
['403']['site_403_message'] = array(
   
'#type' => 'textarea',
   
'#title' => t('Message'),
   
'#default_value' => variable_get('site_403_message', t('<p>You do not have permission to view this content.</p>
<ul>
<li> You may need to log in.</li>
<li> Visit the front page of <sitename> and browse for the information you want.</li>
<li> Click the "Back" button of your browser to try another link.</li>
</ul>'
)),
  );
?>

<?php
  $form
['404']['site_404_message'] = array(
   
'#type' => 'textarea',
   
'#title' => t('Message'),
   
'#default_value' => variable_get('site_404_message', t('<p>The requested content was not found on this site. The content might have been removed or had its location changed.</p>
<ul>
<li> If you typed the URL manually, please check the spelling.</li>
<li> Visit the front page of <sitename> and browse for the information you want.</li>
<li> Click the "Back" button of your browser to try another link.</li>
</ul>'
)),
  );
?>

If this t() use doesn't fulfill the the coding standards, please submit a corrected version or tell me what needs to be fixed. Thanks

I'm not sure why #default_value jumped down a line in second example, but that's not the case in the patch.

StatusFileSize
new8.48 KB

Keeping w/HEAD.

Version:x.y.z» 6.x-dev
Status:Needs review» Needs work

patching file includes/common.inc
Hunk #1 FAILED at 318.
Hunk #2 succeeded at 360 (offset 13 lines).
Hunk #3 succeeded at 375 with fuzz 1 (offset 13 lines).
Hunk #4 succeeded at 402 (offset 13 lines).
1 out of 4 hunks FAILED -- saving rejects to file includes/common.inc.rej
patching file modules/system/system.module
Hunk #1 succeeded at 603 (offset 22 lines).
Hunk #2 succeeded at 673 (offset 22 lines).

This is a large patch. Please think about ways to logically break this up into quickly reviewable chunks which stand on their own.

Indeed, I have been to busy with school to have time for this patch. However, when 5.x is released I'll correct and submit this again.

I'll let this rest until chx's gigantic menu patches are i place.

I hoped this would make its way into D5 but I guess we're all resting on chx's new menu magic.

lets keep this one alive. i think the menu stuff has settled down now. only minor tweaks there left that don't affect 403/404.

See also http://drupal.org/node/141162. forngren: if we're going to improve the 404s as much as your patch suggests, then I /support/ the addition of the checkboxes mentioned in that issue. My current "by design" suggestion is fine in the current state of Drupal - but would really force the user to lose a whole bunch of good stuff if one has to choose between the features in this patch vs. "merely" displaying blocks.

+1 for me on that, since it was my patch.

I like most everything in this patch, but agree with the above comment that it is getting large (but then it is for a new release, not current application).

Instead of the default message idea, is there any way to have it just create a default page for each error and let the admins use that as a starting point for customization? That might reduce this patch's size, and encourage more admins to actually create such pages.

Status:Needs work» Postponed

Postponed until next hunting season, i.e. 7.x-dev. Thanks for all feedback so far.

Version:6.x-dev» 7.x-dev

Assigned:forngren» Unassigned

Free issue give-away ;)

Category:task» feature
Status:Postponed» Needs work

Status:Needs work» Needs review
StatusFileSize
new6.31 KB
Failed: Failed to apply patch.
[ View ]

Free re-roll compliments of patch bingo!

Robin

Status:Needs review» Needs work

The last submitted patch failed testing.

It's been a while, has anyone made any progress with this?

Version:7.x-dev» 8.x-dev
Issue tags:+Needs tests

Is there any duplicates of the issue?