Auto-enable a block's 'status' with a non-empty 'region' when a module is installed

subscribe. this has long been a dark corner of drupal. time to clean it up!. what happens after install if the install profile does not have a left/right/footer region? i'm not sure what should happen - i guess the block should appear as disabled in the blocks ui.

I went through the installer and fiddled with block movement and all looks good here. The issue of blocks being placed into regions that the theme does not support is a separate issue. This issue is just bug fix.

This looks like a pretty good improvement. From the code it looks like those, who would like to have a custom set of blocks (not what modules define by default) in an install profile could just add the blocks with disabled (status = 0) rows, and block_rehash() would obey existing block data anyway.

I am also happy to see the default enabled blocks feature actually work after so long :)

Two possibly minor nitpicks:

- you use static $theme_key, while drupal_static() is all the rage for better testability (even though the function has an argument to change the static, drupal_static() provides a generic way
- not sure I'd do a separate foreach() in system_block_list() on the two menu names, but instead do that via in_array() in the previous loop

A couple hunks are failing now. @Damien - could you reroll? This was RTBC before so not much left.

We were just facing this issue and were planning to make a patch, but appearently it was already done :-)

Hope it can be commited before the codefreeze

+++ modules/block/block.admin.inc
@@ -124,9 +126,13 @@ function block_admin_display_form_submit($form, &$form_state) {
+function _block_compare($a = NULL, $b = NULL, $theme = NULL) {
+  static $theme_key, $regions;
+  if (isset($theme)) {
+    $theme_key = $theme;
+    $regions = NULL;
+    return;

That return looks superfluous to me.

+++ modules/block/block.module
@@ -285,16 +293,22 @@ function block_get_blocks_by_region($region) {
+function block_rehash_all() {
+  $themes = db_query("SELECT name FROM {system} WHERE type = :type AND status = :status", array(':type' => 'theme', ':status' => 1));

Why only enabled themes?

If there is a reason, we should explain in the PHPDoc description.

+++ modules/block/block.test
@@ -259,10 +259,11 @@ class NewDefaultThemeBlocks extends DrupalWebTestCase {
-      $this->assertEqual($blocks[$block->module][$block->delta], $block, t('Block matched'));
+      $this->assertEqual($blocks[$block->module][$block->delta], $block, t('Block matched: %module-%delta', array('%module' => $block->module, '%delta' => $block->delta)));

Since you're concatenating the placeholders here, it would be good to use @ instead of % placeholders, I think.

+++ profiles/default/default.profile
@@ -90,76 +90,6 @@ function default_profile_task_list() {
 function default_profile_tasks(&$task, $url) {
-  
-  // Enable some standard blocks.
-  $values = array(
-    array(
-      'module' => 'system',
-      'delta' => 'main',
-      'theme' => 'garland',
-      'status' => 1,
-      'weight' => 0,
-      'region' => 'content',
-      'pages' => '',
-      'cache' => -1,
-    ),

I really like those being removed... but I do wonder how install profiles will be able to setup blocks.

The more I think about that question (already asked moshe in IRC), the more I think the question doesn't belong into this issue... here, we're just moving the default block configuration, and apparently, both core install profiles are using the default settings exposed by core modules, so we just don't care. :)

This review is powered by Dreditor.

Re-rolled against latest HEAD with all suggestions.

That was part of the tutorial: In above demonstration, you see how the testbot behaves when code uses a constant that is not defined. The issue that we have ~13,000 passing tests, ~5,000 failing tests, but only ~13,000 tests in total is hopefully an already known issue, but for us, it really only matters to use constants that are defined.

:-P

Falures are due to region name changes. Also note that dashboard region now displays on the system blocks.

Hopefully, this should pass.

This patch would really help the UX team in cleaning up the interface... Bojhan struggled to find the place where the Search block's title is setup in #607410: Remove Search this site , and yeah, it's a total WTF.

.

Hi, just wondering if there's an update on this topic - I'm going through the motions of setting up a site in D7 and installing a theme (in this case Basic 7.x) and I'm getting a lovely set of error messages just from trying to add existing blocks (I haven't created any of my own) to my test site. See attached photo of the error message.

"Notice: Undefined index: region in block_admin_display_form_submit() (line 121 of /home/orchidil/public_html/scor/modules/block/block.admin.inc)."

Also, whatever changes I've made results in these error messages plus "The block settings have been updated" and yet, when I reload the page in both firefox and safari (not-logged-in)... I see absolutely no difference. What I'm looking for is for the little log-in block to vanish as an example.

Search block is not enabled upon installing the module. It should, or shouldn't it? This patch should fix it.

Downgrading all D8 criticals to major per http://drupal.org/node/45111

Tagging novice for a straight reroll of #20 using Git.

Just tried it. Rerolling this isn't trivial (but maybe still novice) since lot's of code has been refactored in the meantime.

Alright, let's try this one.

Two intresting failures: How can we handle admin themes and the dashboard? Is the secound assertion still valid?

Thanks for taking this on Niklas. I'll see if somebody can help this move along (I can't :-)

Summarize issue.

Maybe:

- *   - 'region': (optional) Initial value for theme region within which this
- *     block is set. Most modules do not provide an initial value, and
- *     any value provided can be modified by a user on the block configuration
- *     screen. Note: If you set a region that isn't available in the currently
- *     enabled theme, the block will be disabled.
+ *   - 'regions': (optional) Array of region names. If one of them is available
+ *     in the theme this will be the default region for the block. Otherwise
+ *     the block will be disabled by default.
+ *     Any value provided can be modified by a user on the block configuration
+ *     screen.

No ... better idea. Will try it out and write it down and maybe discuss in IRC shortly.

I'm no longer sure whether we actually want to do this, because it hard-codes a range of assumptions that only apply to core profiles and core themes.

I asked sun for some clarification in IRC:

Perhaps the actual way forward rather is to provide a more simple helper function for installation profiles to enable blocks. One essential consequence of the change proposal is that a module's blocks are automatically enabled when you install the module.

And I'm not necessarily sure whether we really want that. E.g., if you install Book module or Poll module or whatever, then the Book Navigation and Latest Poll blocks would or could be automatically enabled.

Meanwhile I think that auto-enabling blocks makes only sense for certain/common "edge-cases", such as the main content, help, or user menu block. I.e., the essential stuff only.

This issue is about two things:

1. region in hook_block_info() is buggy and doesn't work as expected.
2. Use region to give lot's of blocks default regions.

Since 2 is questionable, should we just get 1 in? Maybe together with #1373312: Assign system_main block to 'content' region and help block to 'help' region by default (followup)? We can always do 2 later or differently.

Reword proposed solution.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Agreed. I've updated/rewritten the issue summary.

Reroll with just that.

mmm, not sure what remains of that except for the hook_modules_enabled() implementation.

I guess that attached patch is what we actually want to change here. As outlined in the summary, this somewhat depends on #1373312: Assign system_main block to 'content' region and help block to 'help' region by default (followup)

#43: drupal8.block-region-status.42.patch queued for re-testing.

#43: drupal8.block-region-status.42.patch queued for re-testing.

Updated issue summary.

This has already been fixed in HEAD.