I am working on a theme that has sidebar_first and sidebar_second defined but hidden(not ready to use yet). When I enable the theme in a clean install of 7.x-dev the new theme automatically picks up the same block to region allocation as is done in Bartik. Which means that blocks are allocated to hidden regions that are not viewable in the admin/structure/blocks page and therefore I can't change them. Since the database seems to remember the block/region setup for my theme even if I delete it and copy it back, I had to unhide the region in my .info, clear cache, and then was able to move them where I wanted, and then change my .info again and clear cache again.
It is mildly annoying that a new theme picks up bartik's default block/region allocation to begin with but I guess any new person can figure it out. But it is a real problem if they are allocated to hidden regions on 1st enablement. This is more than an annoyance to me and I sincerely hope this isn't as designed.

CommentFileSizeAuthor
#77 test-block-not-hidden-1103590-D7-77.patch6.74 KBTor Arne Thune
#72 test-block-not-hidden-1103590-71-D7.patch6.06 KBxjm
#71 test-block-not-hidden-1103590-71.patch6.08 KBxjm
#71 interdiff-70-71.patch889 bytesxjm
#70 test-block-not-hidden-1103590-70.patch6.79 KBTor Arne Thune
#70 test-block-not-hidden-1103590-70-D7.patch6.75 KBTor Arne Thune
#68 test-block-not-hidden-1103590-68.patch6.79 KBTor Arne Thune
#68 test-block-not-hidden-1103590-68-D7.patch6.75 KBTor Arne Thune
#65 test-block-not-hidden-1103590-65.patch6.78 KBTor Arne Thune
#65 test-block-not-hidden-1103590-65-D7.patch6.74 KBTor Arne Thune
#62 test-block-not-hidden-1103590-62.patch6.79 KBTor Arne Thune
#62 test-block-not-hidden-1103590-62-D7.patch6.76 KBTor Arne Thune
#55 test-block-not-hidden-1103590-55.patch6.72 KBTor Arne Thune
#55 test-block-not-hidden-1103590-55-D7.patch6.75 KBTor Arne Thune
#51 test-block-not-hidden-fail-1103590-51.patch10.32 KBTor Arne Thune
#51 test-block-not-hidden-1103590-51.patch9.52 KBTor Arne Thune
#44 test-block-not-hidden-fail-1103590-44.patch11.96 KBTor Arne Thune
#44 test-block-not-hidden-1103590-44.patch11.17 KBTor Arne Thune
#38 blocksForNewThemeAfterFirstEnable.png91.03 KBTor Arne Thune
#38 frontPageForNewThemeAfterFirstEnable.png34.15 KBTor Arne Thune
#35 test-block-not-hidden-1103590-35.patch11.12 KBTor Arne Thune
#35 test-block-not-hidden-fail-1103590-35.patch11.91 KBTor Arne Thune
#30 test-block-not-hidden-1103590-30.patch3.56 KBTor Arne Thune
#30 test-block-not-hidden-fail-1103590-30.patch3.7 KBTor Arne Thune
#20 only_use_new_themes_visible_regions_for_blocks-1103590-20.patch881 bytesdcrocks
#17 only_use_new_themes_visible_regions_for_blocks-1103590-17.patch880 bytesdcrocks
#15 only_use_new_themes_visible_regions_to_assign_blocks-1103590-15.patch954 bytesdcrocks
#13 only use new themes visible regions to assign blocks-1103590-11.patch954 bytesdcrocks
#11 only use new theme's visible regions to assign blocks-1103590-11.patch954 bytesdcrocks
#7 1103590-usevisibleregions-7-d8.patch830 bytesdcrocks
#6 1103590-usevisibleregions-6-d8.patch830 bytesdcrocks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dcrocks’s picture

Title: Can't allocate block to region » blocks being added to 'hidden' region when theme 1st enabled

Maybe a clearer title

Jeff Burnz’s picture

Issue tags: +DrupalWTF

Sounds bad, subscribe.

dcrocks’s picture

This is similar to http://drupal.org/node/778608. Changing the line #615 in block.module from:
$regions = system_region_list($theme);
to:
$regions = system_region_list($theme, REGIONS_VISIBLE);

fixes the problem. 'system_region_list' is called from 10 places in Drupal.
I don't think I'm quite up to cutting patches yet, so I hope someone else will look at this and maybe someone should scan to see if there are other places where the call needs to be changed at the same time.

dcrocks’s picture

I need to qualify the above. All I tested was a newly installed drupal with just Bartik, to which I added my theme. The blocks previously added to a 'hidden' region were now, by default, added to the default region(which I guess is the 1st (visible)one listed in my .info file). I don't know if this would impact the installation of Drupal itself. Does Bartik also go through 'block_theme_initialize'?
I also looked at the other places system_region_list is called and a number of them had the change, but not all. I don't understand enough to tell if all the necessary changes were made.

dcrocks’s picture

Component: theme system » block.module

Since the failure occurs in block_theme_initialize in block.module, I guess I should change the component, not that it is getting a lot of attention.

dcrocks’s picture

Version: 7.x-dev » 8.x-dev
Status: Active » Needs work
FileSize
830 bytes

Here is my 1st attempt at a patch. According to the docs, I should develop this for D8, and then it would get backported. The code is identical in both D7 and D8. Hope someone looks at it and may understand if it needs more serious testing than my test case.

dcrocks’s picture

Ok, I wasn't really on version 8. I guess my 'git clone' command was wrong. I did it again and I think I have right clone.

dcrocks’s picture

Status: Needs work » Reviewed & tested by the community

Not much response. I understand this an edge case but it makes sense in the code to make this change. I have tested it on several clean downloads of d7 and one d8 and it hasn't failed yet. But I need this before I can move my theme from sandbox to production so I am going to upgrade it.

Jeff Burnz’s picture

Status: Reviewed & tested by the community » Needs review

Except you can't RTBC your own patches, needs peer review.

dcrocks’s picture

I'd love to have some. But nothing for the block module seems to be getting much attention.

dcrocks’s picture

I assume I did something wrong, so I'll try again.

Status: Needs review » Needs work
dcrocks’s picture

Status: Needs work » Needs review
FileSize
954 bytes

Try again

Status: Needs review » Needs work
dcrocks’s picture

Status: Needs work » Needs review
FileSize
954 bytes

Encodings! try again

Status: Needs review » Needs work
dcrocks’s picture

Status: Needs work » Needs review
FileSize
880 bytes

Changed comment

dcrocks’s picture

Issue tags: -DrupalWTF

If anyone has a moment to look at the code.

tim.plunkett’s picture

Status: Needs review » Needs work
Issue tags: +DrupalWTF, +Needs backport to D7
+++ b/modules/block/block.moduleundefined
@@ -612,7 +612,8 @@ function block_theme_initialize($theme) {
+    // Apply only to new themes visible regions.

If this is plural, needs to be "themes'". Otherwise, "theme's".

dcrocks’s picture

dcrocks’s picture

Status: Needs work » Needs review
dcrocks’s picture

A review would be appreciated. Did find another way around issue without going back and forth modifying .info file or database. Example given in http://drupal.org/node/1192028. I think this is due to proper call to "system_region_list" in block.admin.inc line 323. Still a simple fix and tired of kluge.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Makes sense to me. And works, obviously.

webchick’s picture

Title: blocks being added to 'hidden' region when theme 1st enabled » (needs tests) blocks being added to 'hidden' region when theme 1st enabled
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Makes sense. I committed and pushed this patch to 8.x. and 7.x since we're unlikely to break this part of the code again.

We should have a test for this, though. Setting back to needs work.

dcrocks’s picture

Never done that before. Pointer on how to create test?

webchick’s picture

http://drupal.org/simpletest is where the documentation is. It might be a bit outdated (please fix it if so!) #drupal-contribute on IRC is also a great place to ask questions.

Basically, we want to put into code the steps used to cause this bug and confirm it's fixed. That way we never break it again in the future.

silverwing’s picture

spam clean-up (excuse the noise)

silverwing’s picture

overzealous :(

Tor Arne Thune’s picture

Status: Needs work » Needs review
FileSize
3.7 KB
3.56 KB

I created a test for this, but I can't seem to reproduce the bug here. The second patch (test-block-not-hidden-fail-1103590-30.patch) undoes the fix committed in this issue, so the test is supposed to fail, but it does not. The search form block is still inherited to a visible region (content).

Either I have misunderstood the issue and written a test that does not apply, or the bug described in the OP is caused by something else.

Status: Needs review » Needs work

The last submitted patch, test-block-not-hidden-fail-1103590-30.patch, failed testing.

Tor Arne Thune’s picture

Ah, so I have to fix the exceptions too. No room to be lazy here ;) New patches coming up, but the same issue in #30 still stands.

dcrocks’s picture

Status: Needs work » Needs review

It will take me a little bit to figure out your code but the scenario is:
Default theme(eg. bartik) has 'search' block assigned to 'sidebar_first' region
New theme has 'sidebar_first' defined but 'hidden' in .info file:
regions[sidebar_first] = Sidebar first
regions_hidden[] = sidebar_first
Without patch, when new theme 1st enabled, 'search' block is assigned to region 'sidebar_first'.
When you go to admin/structures/blocks the 'search' block is not displayed anywhere because
1) It won't show up in block::region list because the region is hidden, and
2) It is assigned to a region so therefore won't show up in disabled block list

It is interesting because if you went to 'bartik'/admin/structures/blocks/'sidebar_first'/'search' 'configure', under 'region settings' the entry for 'new theme' would show 'search' assigned to 'sidebar_first', and could change it to 'none' there. There the 'form' doesn't use 'system_region_list'

dcrocks’s picture

The last paragraph is confusing. What I am saying is that if you go to the block's configuration page thru a theme where the region is visible, you can change its region assignment for a theme where the region is invisible. Should it work that way?

Tor Arne Thune’s picture

Added a page.tpl.php file where I removed sidebar_first and sidebar_second to the test theme to remove notices. As in #30, test-block-not-hidden-fail-1103590-35.patch should fail, but does not. The search form block is successfully inherited to the Content region without the fix that was committed :(

Tor Arne Thune’s picture

Without patch, when new theme 1st enabled, 'search' block is assigned to region 'sidebar_first'.
When you go to admin/structures/blocks the 'search' block is not displayed anywhere because
1) It won't show up in block::region list because the region is hidden, and
2) It is assigned to a region so therefore won't show up in disabled block list

Yes, this is what I thought too, but I can't reproduce this. Without the fix, the search form block is inherited (from Bartik) to the Content region when sidebar_first is a hidden region for the newly enabled theme. I was under the impression that the issue was that the block would be inherited to the sidebar_first region and therefore become "invisible."

dcrocks’s picture

Don't understand. Just did a quick install of drupal, reverted the fix, and got the results expected. The database clearly shows 'search' assigned to sidebar_first for both bartik and 'new theme'. Are you sure you are using bartik as default? Even with the patch in, the blocks should be assigned to the theme's default region,
"* Assign an initial, default set of blocks for a theme.
*
* This function is called the first time a new theme is enabled. The new theme
* gets a copy of the default theme's blocks, with the difference that if a
* particular region isn't available in the new theme, the block is assigned
* to the new theme's default region.
"
according to the code in block.module.
On my theme that appears to be the 1st visible region, which isn't the 'content' region for me or Bartik. 'content' is the 1st visible region for seven though. Actually just noticed that the blocks assigned to sidebar_first in bartik are invisible on 'seven's block page as well. That is because they are assigned to 'dashboard sidebar', an invisible 'overlay region' the dashboard module creates for themes.

Actually the logic still doesn't make sense. If the region isn't visible, the blocks shouldn't be assigned to any region, certainly not the 'default region'. It should be on the 'disabled' block list(which actually is more like an 'available' blocks list).

Tor Arne Thune’s picture

Then it's probably a flaw in the test. Simpletest does a clean installation of Drupal with all the default settings (Bartik as default theme) and performs the test. I'm not able to find where the problem is though. The search form block should not be displayed on the front page in the content region (see second attachment) if it's assigned to the hidden sidebar_first region. I see from the verbose messages of simpletest (first attachment) that all the blocks that were in the sidebar_first region of Bartik are in the content region of the new theme.

First attachment shows a screenshot of the result of $this->drupalGet('admin/structure/block');
Second attachment shows a screenshot of the result of $this->drupalGet('');

dcrocks’s picture

This really doesn't quite match seven either. seven actually has a sidebar_first region, and it is also hidden by default, so you would expect, and do get the same non-patch results I get. This may be due with the way themes(and the dashboard) get initialized. It is interesting that user/login gets assigned to 'content' for seven but search is invisible on seven's block page. Could you test on the navigation block instead of the search block? The search block is assigned to 'dashboard sidebar' for seven but not navigation.

dcrocks’s picture

Is there a fresh install between each test? When does theme initialization occur? Dashboard initialization? It appears to be using the set of blocks defined by bartik but it as if they hadn't been assigned to regions yet. This is weird. Garland and stark have no regions defined in .info, but look what happens when they are enabled.

dcrocks’s picture

I think I may have discovered the source of the discrepancy. In your .info file you defined sidebar_left ONLY with the statement:
regions_hidden[sidebar_first] = Left sidebar
while I used the 2 lines as in #33 above. The result is that in your test theme sidebar-left is defined in 'regions_hidden' array but not in the 'regions' array, while in mine it is in both. So in your test the sidebar_first region would not even be returned by 'system_region_list'. If you look at the code for 'system_region_list' you'll see that both arrays are used to produce the result set.
I don't know what is the correct syntax. My intent was that a re-user of my theme could simply delete(or comment out) the pertinent regions_hidden line from the .info file to 'un-hide' the region. In your syntax they would merely have to edit regions_hidden... to regions... But if that is the standard syntax then 'system_region_list(theme, option)' wouldn't need the REGIONS_VISIBLE option. So where do we go from here?

dcrocks’s picture

A few points:

I can't find any documentation on region_hidden except http://drupal.org/update/themes/6/7#closure.

The system adds 'page_top' and 'page_bottom' to region_hidden as if it were a numeric array, which is the way I used it and in the example given in the reference above. The way it is specified in the test(#35) add entries as if associative array.

The in_array test in system_region_list is not the fastest thing around, has some known issues, and I don't how reliable it is if the array has multiple member types.

Drupal insists that a 'content' region be specified, but not help, page_top , or page_bottom, though users are STRONGLY encouraged to include them if they override regions in their .info file. Are these not required regions?

The message given when the content region is missing is not very informative if not misleading.

There doesn't seem to be much validation done on entries in .info. For example, you can specify a region as hidden that isn't in the region list.

It seems that some new issues should be created, at least for documentation.

dcrocks’s picture

@Tor Arne Thune Any chance of correcting your test for standard usage so this can get finished?

Tor Arne Thune’s picture

Finally made some time to follow through on this.

I think I may have discovered the source of the discrepancy. In your .info file you defined sidebar_left ONLY with the statement:
regions_hidden[sidebar_first] = Left sidebar

Thank you! It was a misunderstanding on my part as to how to actually use this feature. Made the necessary changes and the patch that reverts your fix in #20 (test-block-not-hidden-fail-1103590-44.patch) fails the test on my installation, so it's good to go.

First patch is supposed to fail and second is supposed to pass.

Patches apply cleanly to D7 as well.

dcrocks’s picture

Thank you. I added a comment to .info documentation: http://drupal.org/node/171205 , but should I also open a documentation issue?

Tor Arne Thune’s picture

I added a comment to .info documentation: http://drupal.org/node/171205 , but should I also open a documentation issue?

Sounds good. Yeah, why not open an issue. I can't think of any other places where theme info file properties are documented, though.

dcrocks’s picture

Created a new issue, #1243144: 'regions_hidden' attribute in theme .info file not documented, for documentation. How does this one get closed?

Tor Arne Thune’s picture

Someone needs to review the test in #44 (test-block-not-hidden-1103590-44.patch and mark it RTBC or needs work.

Tor Arne Thune’s picture

xjm’s picture

+++ b/modules/block/block.moduleundefined
@@ -613,7 +613,7 @@ function block_theme_initialize($theme) {
-    $regions = system_region_list($theme, REGIONS_VISIBLE);
+    $regions = system_region_list($theme);

Note for other reviewers: This line rolls back the original fix to demonstrate the failing test.

+++ b/modules/block/block.testundefined
@@ -83,7 +83,7 @@ class BlockTestCase extends DrupalWebTestCase {
-    // Check if the block can be moved to all availble regions.
+    // Check if the block can be moved to all available regions.

@@ -264,7 +264,7 @@ class BlockTestCase extends DrupalWebTestCase {
-    // Check if the block can be moved to all availble regions.
+    // Check if the block can be moved to all available regions.

@@ -278,7 +278,7 @@ class BlockTestCase extends DrupalWebTestCase {
-    // Confirm that the regions xpath is not availble
+    // Confirm that the regions xpath is not available

I'd suggest submitting these documentation fixes as a separate patch in a new documentation issue. Generally we want to avoid mixing different fixes in the same patch. Edit: also, the last one will need a period. :)

+++ b/modules/block/block.testundefined
@@ -708,3 +708,39 @@ class BlockTemplateSuggestionsUnitTest extends DrupalUnitTestCase {
+ * Test blocks not inherited to hidden regions when enabling new theme.

Should be "Tests" blocks.

Tor Arne Thune’s picture

Thanks for the feedback. Much appreciated. Re-rolled with your suggestions.
First patch proves that the test works by reverting the fix committed in #24, and should fail.
Second patch should pass.
Patch still applies cleanly to D7 as well.

xjm’s picture

Title: (needs tests) blocks being added to 'hidden' region when theme 1st enabled » (tests added) blocks being added to 'hidden' region when theme 1st enabled
Status: Needs review » Reviewed & tested by the community

Looks good to me.

xjm’s picture

Issue tags: -Needs tests

Opened #1267092: Some comment cleanup in block.test as a followup. Untagging 'cause the tests are there. :)

sun’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/themes/tests/block_test_theme/page.tpl.php
@@ -0,0 +1,137 @@
+ * @file
+ * Default theme implementation to display a single Drupal page.
+ *
+ * Available variables:

If this page.tpl.php override is required for the test in any way, then its @file phpDoc should explain how and why. All of the default phpDoc of the original template file can and should be removed.

20 days to next Drupal core point release.

Tor Arne Thune’s picture

Status: Needs work » Needs review
FileSize
6.75 KB
6.72 KB

Sure thing. I removed the sidebar code from page.tpl.php to avoid exceptions about the sidebar variables not being defined. Added a @file comment in page.tpl.php to explain it.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

quicksketch’s picture

This patch will conflict with #22336: Move all core Drupal files under a /core folder to improve usability and upgrades due to adding a new file (themes/tests/block_test_theme/page.tpl.php). Technically the one-day grace period for that patch starts tomorrow, but I'd prefer not to need to reroll that patch again before then as it's a bit more complicated to reroll than other patches.

Tor Arne Thune’s picture

That's fine. I'll make a new patch for D8 after the commit of #22336: Move all core Drupal files under a /core folder to improve usability and upgrades.

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +DrupalWTF, +Needs backport to D7

The last submitted patch, test-block-not-hidden-1103590-55.patch, failed testing.

Tor Arne Thune’s picture

Assigned: Unassigned » Tor Arne Thune

Rerolling.

Tor Arne Thune’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
6.76 KB
6.79 KB

Voilà. Setting back to RTBC as it's just a reroll.

catch’s picture

Status: Reviewed & tested by the community » Needs review
+ * Tests blocks not inherited to hidden regions when enabling new theme.

Shouldn't it be "inherited by"?

xjm’s picture

Status: Needs review » Needs work
Issue tags: +Novice
+ * Tests blocks not inherited to hidden regions when enabling new theme.

Shouldn't it be "inherited by"?

Yeah. While we're at it, let's change this to be more clear:

Tests that hidden regions do not inherit blocks when a new theme is enabled.

I checked and this fits 80 chars.

A novice could reroll with this change; tagging.

Tor Arne Thune’s picture

Status: Needs work » Needs review
FileSize
6.74 KB
6.78 KB

Thanks, sounds much better. Rerolled with changed comment.

xjm’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice

That looks better. I checked through the patch and the rest looks fine as well.

catch’s picture

Status: Reviewed & tested by the community » Needs work

Sorry folks could we change this to the same language in the other places it's used as well:

Checks that a newly enabled theme does not inherit blocks to its hidden regions.

for example.

Tor Arne Thune’s picture

Status: Needs work » Needs review
FileSize
6.75 KB
6.79 KB

Sure, makes sense.

Status: Needs review » Needs work

The last submitted patch, test-block-not-hidden-1103590-68.patch, failed testing.

Tor Arne Thune’s picture

Status: Needs work » Needs review
FileSize
6.75 KB
6.79 KB

Test in patch got cut short by one closing curly brace.

xjm’s picture

So the reason I didn't suggest changing that second docblock (and I thought about it) is that it's indented by two from the class (naturally) and therefore was going to be 81 characters. We don't want to wrap the one-line summary onto two lines. :)

However, here's a wording that fits in both places.

Edit: and it's 6:00a and I named the interdiff 'patch' so that's going to make testbot fail. Are bhai.

xjm’s picture

D7 version.

Status: Needs review » Needs work

The last submitted patch, interdiff-70-71.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review

Yeah, yeah.

Tor Arne Thune’s picture

Status: Needs review » Reviewed & tested by the community

Fine with me :)

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Needs review

Thanks! Committed/pushed to 8.x, moving to 7.x.

Tor Arne Thune’s picture

Great, thanks for the help on my first automated test :)
Re-uploading the D7 patch to trigger testbot for D7 branch.

Tor Arne Thune’s picture

Status: Needs review » Reviewed & tested by the community

And should probably be RTBC for 7.x, as there are no changes.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 7.x. Thanks!

Tor Arne Thune’s picture

Title: (tests added) blocks being added to 'hidden' region when theme 1st enabled » Blocks being added to 'hidden' region when theme first enabled
Assigned: Tor Arne Thune » Unassigned

Status: Fixed » Closed (fixed)
Issue tags: -DrupalWTF, -Needs backport to D7

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