This patch allows a 404 (not found) page to be configured to theme with blocks if the site_404 page is supplied.

Files: 
CommentFileSizeAuthor
#16 221399-404-block.patch2.67 KBsmk-ka
Passed: 11116 passes, 0 fails, 0 exceptions
[ View ]
#12 error_reporting-8.patch2.54 KBjeffschuler
Unable to apply patch error_reporting-8.patch
[ View ]
#10 error_reporting-7.patch2.48 KBjeffschuler
Unable to apply patch error_reporting-7.patch
[ View ]
#6 404-block-checkbox.png51.54 KBlilou
#6 error_reporting-6.patch1.76 KBlilou
Failed: Failed to apply patch.
[ View ]
#3 404-block.jpg52.07 KBlilou
error_reporting.patch.txt1.86 KBearnie

Comments

lilou’s picture

Still appllied.

+1 for this patch.

earnie’s picture

@lilou: Are your ready to RTBC the Status?

lilou’s picture

StatusFileSize
new52.07 KB

Work great ...

Only local images are allowed.

lilou’s picture

Status:Needs review» Reviewed & tested by the community
dmitrig01’s picture

Status:Reviewed & tested by the community» Needs work

I would use a checkbox personally for binary settings.

lilou’s picture

Status:Needs work» Needs review
StatusFileSize
new1.76 KB
Failed: Failed to apply patch.
[ View ]
new51.54 KB

@dimitrig01 : you're right.

earnie’s picture

Status:Needs review» Reviewed & tested by the community

Yes, I think so to.

Dries’s picture

Interestingly, the 404 and 403 field descriptions don't describe what happens when you leave those fields blank. I think we should describe what happens, as we hint at it in the description of the checkbox. If you wipe your memory and context, it is hard to make sense of the different form elements. I recommend that we massage the form descriptions some more.

webchick’s picture

Status:Reviewed & tested by the community» Needs work

Guess that makes it CNW then.

jeffschuler’s picture

Status:Needs work» Needs review
StatusFileSize
new2.48 KB
Unable to apply patch error_reporting-7.patch
[ View ]

Re-rolled to apply against current D7 HEAD.

Incorporated very brief but more descriptive text for default 403/404 page fields:
Changed: If unsure, specify nothing.
To: Leave blank to display a generic "access denied" page.
And: Leave blank to display a generic "page not found" page.

Also attempted to simplify the checkbox description text and added "default" to the checkbox title to make it more clear this applies to the default 404 page.

tstoeckler’s picture

Status:Needs review» Needs work

   // To conserve CPU and bandwidth, omit the blocks.

This needs to be changed to something like:

// To conserve CPU and bandwidth, allow the user to omit the blocks.

jeffschuler’s picture

StatusFileSize
new2.54 KB
Unable to apply patch error_reporting-8.patch
[ View ]

How about:

// Optionally omit the blocks to conserve CPU and bandwidth.

tstoeckler’s picture

Status:Needs work» Needs review

Sure.

smk-ka’s picture

Nice. I'd even RTBC it, but what about the next developer who requires to display a fully themed "click here to signup" page for visitors hitting the access denied page?

jeffschuler’s picture

Blocks are always displayed on 403 pages.

smk-ka’s picture

StatusFileSize
new2.67 KB
Passed: 11116 passes, 0 fails, 0 exceptions
[ View ]

Oh, cool :)

Rerolled with whitespace issues fixed.

catch’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:+Needs text review

Looks good to me, was RTBC before and those points are covered. I don't really like that we're cluttering the performance page with this, but it is a performance setting, so not much to be done about that.

catch’s picture

Issue tags:-Needs text review

umm, I read back after readding the text and it's been reviewed by a few people, de-tagging.

Status:Reviewed & tested by the community» Needs work

The last submitted patch failed testing.

lilou’s picture

Status:Needs work» Reviewed & tested by the community

Setting to previous status - testbot was broken (failed to install).

Dries’s picture

Status:Reviewed & tested by the community» Fixed

Committed to CVS HEAD. Thanks.

moshe weitzman’s picture

To me, this is pref bloat. It is a 5 line contrib module to implement this in hook_page_alter(). We need less special cases, not more. I think the whole #show_blocks should go away now. Just show them all and let contrib or site builder decide rules otherwise. #show_blocks is absurd since it only works for the left and right regions.

Pasqualle’s picture

Status:Fixed» Needs work

I think the "show them all" is not good enough for Drupal core #232037: block_list() renders all blocks, even on 404
but Moshe is right, there could be a better solution..

tstoeckler’s picture

Marked #116895: Show regions at 404 page as duplicate.

JohnAlbin’s picture

@moshe I really agree with you on #show_blocks.

@Pasqualle: The default should be to show them all.

How could Drupal core possibly know which regions of a given theme should be shown/hidden on a 404 page. Is it going to inspect the region first to make sure there isn't a critical navigation menu block? There should be a toggle next to each theme region on whether it should be rendered on 404 pages.

And, given the lateness of the D7 cycle, that is why it should be a contrib module. Hell, its the inverse of my 404 blocks module; I'd be happy to write it after code freeze. Or before, if that's what's required to get #show_blocks removed from core.

JohnAlbin’s picture

Alternatively, you could have a per-block setting for “hide on 404” (while still killing #show_blocks); I'm less certain about the performance savings on that option, though.

davyvdb’s picture

Status:Needs work» Fixed

Rendered obsolete by #423992

davyvdb’s picture

Status:Fixed» Closed (won't fix)