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

Still appllied.

+1 for this patch.

Work great ...

I would use a checkbox personally for binary settings.

@dimitrig01 : you're right.

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.

Guess that makes it CNW then.

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.

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

How about:

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

Sure.

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?

Blocks are always displayed on 403 pages.

Oh, cool :)

Rerolled with whitespace issues fixed.

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.

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

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

Committed to CVS HEAD. Thanks.

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.

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

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

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

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.

Rendered obsolete by #423992