Download & Extend

Allow administrator to decide if blocks should be displayed for site_404 pages

Project:Drupal core
Version:7.x-dev
Component:base system
Category:feature request
Priority:normal
Assigned:Unassigned
Status:closed (won't fix)

Issue Summary

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

AttachmentSizeStatusTest resultOperations
error_reporting.patch.txt1.86 KBIgnored: Check issue status.NoneNone

Comments

#1

Still appllied.

+1 for this patch.

#2

@lilou: Are your ready to RTBC the Status?

#3

Work great ...

Only local images are allowed.

AttachmentSizeStatusTest resultOperations
404-block.jpg52.07 KBIgnored: Check issue status.NoneNone

#4

Status:needs review» reviewed & tested by the community

#5

Status:reviewed & tested by the community» needs work

I would use a checkbox personally for binary settings.

#6

Status:needs work» needs review

@dimitrig01 : you're right.

AttachmentSizeStatusTest resultOperations
404-block-checkbox.png51.54 KBIgnored: Check issue status.NoneNone
error_reporting-6.patch1.76 KBIdleFailed: Failed to apply patch.View details | Re-test

#7

Status:needs review» reviewed & tested by the community

Yes, I think so to.

#8

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.

#9

Status:reviewed & tested by the community» needs work

Guess that makes it CNW then.

#10

Status:needs work» needs review

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.

AttachmentSizeStatusTest resultOperations
error_reporting-7.patch2.48 KBIdleUnable to apply patch error_reporting-7.patchView details | Re-test

#11

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.

#12

How about:

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

AttachmentSizeStatusTest resultOperations
error_reporting-8.patch2.54 KBIdleUnable to apply patch error_reporting-8.patchView details | Re-test

#13

Status:needs work» needs review

Sure.

#14

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?

#15

Blocks are always displayed on 403 pages.

#16

Oh, cool :)

Rerolled with whitespace issues fixed.

AttachmentSizeStatusTest resultOperations
221399-404-block.patch2.67 KBIdlePassed: 11116 passes, 0 fails, 0 exceptionsView details | Re-test

#17

Status:needs review» reviewed & tested by the community

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.

#18

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

#19

Status:reviewed & tested by the community» needs work

The last submitted patch failed testing.

#20

Status:needs work» reviewed & tested by the community

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

#21

Status:reviewed & tested by the community» fixed

Committed to CVS HEAD. Thanks.

#22

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.

#23

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

#24

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

#25

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

#26

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.

#27

Status:needs work» fixed

Rendered obsolete by #423992

#28

Status:fixed» closed (won't fix)
nobody click here