block_list() renders all blocks, even on 404

I always assumed that if $show_blocks is FALSE, then no blocks would render into any region. we can't change that behavior for D5 or D6, but we might want to consider that for D7.

block_list doesn't have a $show_blocks parameter.

pwolanin's patch looks harmless to me. Actually, it doesn't seem to change anything, left and right sidebar regions aren't with and without the patch for me.

This patch works as expected, but I see it as a good candidate for some refactoring in order to do some unit testing on it.

The patch makes a clear distinction between loading block information and rendering blocks. Here's another version of this patch that refactors block_list() into the private functions _block_load_blocks() and _block_render_blocks().

The patch isn't pretty because of the code being moved around, but it's necessary if we want to test the case for which this issue was originally written.

flobruit: thanks for the patch, however it's not in unified diff format. Please see http://drupal.org/patch for more.

catch: thank you for the note, that was my bad. Here's the patch in the correct format.

pwolanin, I agree with you on the by-ref, and in this case passing only the blocks for the desired regions makes a lot of sense.

There are no test cases for the block module yet. I started writing some, but in order to automatically test the current issue I need to be able to make a call to _block_render_blocks() on a "page not found" which turns out to be a little tricky. I'll create a separate issue to create tests for block.module so that this patch can be committed.

The code and approach look good but can we rename $block_region back to $blocks -- it's more consistent with the rest of Drupal's coding style.

I'm left wondering if we can further "clean up" this functionality and its APIs. Like, would it make sense to load blocks on a per-region basis instead of loading all regions at once. Specifically, I have a website where I have a custom block region which is only used on the main page of the site (i.e. only the main page has one or two blocks assigned to that region). Given a good set of APIs, I might be able to optimize my theme not to load a list of redundant blocks. I'm not sure it would buy me a significant performance gain but I figured I'd bring up a potential use case.

OK, I reviewed this once more and it looks good. I still think there is room for improvement but this is probably the most important step, and we can bolt incremental improvements on top of this.

OK, I reviewed this once more and it looks good. I still think there is room for improvement but this is probably the most important step, and we can bolt incremental improvements on top of this.

This refactoring patch looks a bit scary to apply to Drupal 6 without much testing and although it does not modify any APIs, it does rework quite some amount of code. I'd prefer to have a more limited changeset to work with, which is easier to verify, review and test.

The patch from #1 came before the refactoring but was already fully functional. It still applies to 6.x-dev and works as expected.

Does indeed apply cleanly to 6.x and works fine.

Walked through this patch line by line and it looks to be all-good. Thanks, committed to 6.x. (Dries already committed to 7.x, so this makes this one fixed).

This is a straight(!) patch from the one in comment #20. Not tested.

Committed to 5.x.

In 5.x this caused a regression, and doesn't even work, because the blocks are all called explicitly in _phptemplate_default_variables(). See the patch/comment at http://drupal.org/node/281042#comment-916600 for a fix to this problem that will work.

It's in phptemplate.engine instead of block.module, because $show_headers is available there, so that's where the logic should be anyway.

this patch breaks drupal_add_js (at least thats what we managed to reproduce) from blocks

Can you be more explicit about which patch you're talking about. The patch at:
http://drupal.org/node/281042#comment-921255 (http://drupal.org/files/issues/fix_block_loading_on_404-v3.patch)
should probably be applied with
http://drupal.org/files/issues/block_regression_fix.patch
since the block_list change is ineffective (and breaks an assumption in theme_page() ), but I believe it's arbitrary.

If it's my patch, where is your drupal_add_js() getting called from? All of my drupal_add_js() calls in modules and blocks are working.

I meant the patch that got committed into 5.8. I probably didn't read your comment in #27 properly, we're talking about the same thing. I just wanted to stress that with this stock drupal 5.8 blocks might be broken.

Your patch on http://drupal.org/node/281042#comment-916613 does provide a fix.

Setting this back to closed, there is another issue where the work is being done.

Considering that this patch caused several problems, which were attempted to be fixed by http://drupal.org/node/281042 - which caused further problems, I am wondering if this is the right direction - is there a less intrusive way to disable block rendering for 404 pages only or (perhaps simpler, especially for a stable core) could this simply be done in contrib with a module that loads the requested content in hook_init and does print theme('page', $output, FALSE)?

Maybe just revert it all.

Further updates on this are in #281042: regression in 5.8 from 5.7 theme('blocks','all') broken - so no header alteration possible. I suspect the remaining problems are isolated and sometimes caused by not using updated versions of various modules.