Currently, the $show_blocks variable allows modules, 404 error pages, etc to turn off the left and right sidebars. This is supposed to be an optimization to reduce processing in certain circumstances. See #76824: Drupal should not handle 404 for certain files and #80201: Don't show blocks on bootstrapped 404 pages for the original discussion.

Unfortunately, Drupal has no way to know which regions a site administrator would consider essential to display on every page on the site. e.g. if the primary navigation is a block in the left region, that block must be shown on 404 pages, etc. So turning off just the left and right sidebars is pretty arbitrary.

So we have 2 options:

  1. Remove this functionality entirely or
  2. change the $show_blocks to a $hide_regions variable which holds names of regions to turn off when performance is crucial. So, $hide_regions would be a per-theme array that held the names of any regions that would need to be optionally hidden for that theme.

Thoughts?

CommentFileSizeAuthor
#16 show-blocks-423992-10.patch1.54 KBdavyvdb
#14 drupal.show-blocks-revert.patch9.96 KBsun
#9 show-blocks-423992-9.patch9.96 KBjohnalbin
#6 system-404-regions-423992-2.patch3.9 KBdavyvdb
#4 system-404-regions-423992.patch2.59 KBdavyvdb

Comments

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented
Title: Change show_blocks page variable to allow arbitrary regions to be hidden » Remove show_blocks page variable

After talking to Moshe briefly in IRC, he recommended we remove it.

a site mgr has to give rules about what blocks should show on pages like 403/404 and such. modules that need block control can use various tools including hook_page_alter()

Changing issue title. Hopefully a patch should be easy to make in the next few days.

davyvdb’s picture

davyvdb commented

I have documented this once on my blog. You can do this in the theme right now. I do this on every site now. It's also interesting when you use multiple page templates where you have different regions for each page template.

First I disable the show_blocks

http://www.drupalcoder.com/story/411-showing-left-and-right-region-on-pa...

Then I create my own block_list that does the checks using a mapping function

function phptemplate_get_template_regions($region) {
        $map = array(
                'page' => array('content', 'left', 'right'),
                'page-left' => array('content', 'left'),
        );
        return $map[$region];
}

http://www.drupalcoder.com/story/313-improve-drupals-performance-by-not-...
http://www.drupalcoder.com/story/339-improve-drupals-performance-by-not-...

Do you think we need a UI for this? Throttle would have been nice here.

An idea for a UI : Add checkboxes for all regions to /admin/appearance/settings/garland (and other themes) asking... Which regions should never be hidden if Drupal decides to leave out a region for performance. Drupal tends to do this on pages like "page not found", "access denied".

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented

Note that when removing show_blocks, we'd need to remove the “Display blocks on default 404 (not found) page” checkbox on admin/settings/site-information as well.

@Davy, D7 already has the flexibility built-in that would allow a contrib module to have that kind of interface to those user who need that kind of performance boost. So I think we should remove the show_blocks functionality from core (meaning always show regions on 404/403) and let contrib handle a region/block hiding UI for 404/403 pages.

davyvdb’s picture

davyvdb commented
Status: Active » Needs review
StatusFileSize
new system-404-regions-423992.patch2.59 KB

I've removed the form stuff from Site information.
I've done a variable_del in system_update.
I've cleaned up drupal_not_found.

Status: Needs review » Needs work

The last submitted patch failed testing.

davyvdb’s picture

davyvdb commented
Status: Needs work » Needs review
StatusFileSize
new system-404-regions-423992-2.patch3.9 KB

Status: Needs review » Needs work

The last submitted patch failed testing.

davyvdb’s picture

davyvdb commented
Status: Needs work » Needs review

This patch actually works fine, but it keeps failing because of a bug in the block tests.

http://drupal.org/node/557024

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented
StatusFileSize
new show-blocks-423992-9.patch9.96 KB

Davy, awesome start.

The changes you made to drupal_not_found() look too extreme because it removes the the calling of HOOK_element_info_alter on the page. I've reverted that bit. And combined your patch in #557024: Wrong URL in drupalGet results to wrong test results. Also, a global search for show_blocks shows we missed some spots.

Here's a new patch.

Status: Needs review » Needs work

The last submitted patch failed testing.

davyvdb’s picture

davyvdb commented
Status: Needs work » Reviewed & tested by the community

Aha, you want to remove the underlying show_block stuff too. Looks great now!

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented
Priority: Normal » Critical
Status: Fixed » Active

Bah. That patch broke the System: Drupal Error Handlers simpletests in modules/simpletest/tests/error.test. :-p

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Active » Reviewed & tested by the community
StatusFileSize
new drupal.show-blocks-revert.patch9.96 KB

Straight roll-back makes error tests pass again.

davyvdb’s picture

davyvdb commented
Status: Reviewed & tested by the community » Needs work

Hold on, I'm creating a patch to fix this (an not re-roll this).

davyvdb’s picture

davyvdb commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new show-blocks-423992-10.patch1.54 KB

This fixes this.

The cause was that show_blocks is removed from theme('maintenance_page')

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented

I looked at the patch in #16 and it is correctly removing the 2nd parameter from all the calls to theme_maintenance_page in D7.

RTBC from me.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed. Thanks.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Fixed » Needs work
Issue tags: +Needs documentation

Needs to be added to the http://drupal.org/update/theme/6/7 theme update guide, and perhaps to the module update guide?

catch’s picture

catch
he/him
Primary language English
commented
Priority: Critical » Normal

#653068: Update documentation is incomplete

effulgentsia’s picture

effulgentsia commented
Title: Remove show_blocks page variable » Document removal of show_blocks page variable

.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Title: Document removal of show_blocks page variable » Remove show_blocks page variable

It's best to leave the title as it was. The "Needs documentation" tag takes care of notifying us what needs to be done.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Issue tags: -Needs documentation +Needs documentation updates

changing tagging scheme

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs work » Fixed

Added this to http://drupal.org/update/themes/6/7#show-blocks

Status: Fixed » Closed (fixed)
Issue tags: -theme, -Needs documentation updates

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue tags: +theme, +Needs documentation updates

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.