Problem

  • When Block module is installed (later on, not as part of a install profile), the Main content block is disabled and not assigned to any region.
  • Thus, when trying to assign another block to some region, you suddenly get the error message:

    Region for Main page content block field is required.

Goal

  • Automatically assign Main content block to "content" region if possible.
  • Ease usage of Testing profile.
Files: 
CommentFileSizeAuthor
#22 drupal.system-blocks-default-regions.22.patch1.31 KBsun
PASSED: [[SimpleTest]]: [MySQL] 39,255 pass(es).
[ View ]
#16 drupal8.system-blocks-default-regions.16.patch2.81 KBsun
PASSED: [[SimpleTest]]: [MySQL] 34,463 pass(es).
[ View ]
#7 drupal8.system-blocks-default-regions.7.patch2.76 KBsun
PASSED: [[SimpleTest]]: [MySQL] 34,452 pass(es).
[ View ]
#4 drupal8.system-blocks-default-regions.4.patch2.83 KBsun
PASSED: [[SimpleTest]]: [MySQL] 34,247 pass(es).
[ View ]
#1 drupal8.system-blocks-default-regions.1.patch1.07 KBsun
PASSED: [[SimpleTest]]: [MySQL] 34,205 pass(es).
[ View ]
drupal8.system-main-block-content.0.patch2.44 KBsun
PASSED: [[SimpleTest]]: [MySQL] 34,168 pass(es).
[ View ]

Comments

StatusFileSize
new1.07 KB
PASSED: [[SimpleTest]]: [MySQL] 34,205 pass(es).
[ View ]

We should do the same for the Help block.

Some tests try to assert that their help text is correctly output as expected on certain pages.

Title:Auto-suggest to assign system_main block to 'content' region by defaultAssign system_main block to 'content' region and help block to 'help' region by default

oopsie, I forgot the docs changes from #0 in #1.

So will need a re-roll, but let's get some reviews first.

StatusFileSize
new2.83 KB
PASSED: [[SimpleTest]]: [MySQL] 34,247 pass(es).
[ View ]

Merged the docs-change in.

This seems like something that merits a test, but I'm not sure we can add the test without using the testing profile?

As soon as #1373142: Use the Testing profile. Speed up testbot by 50% lands, this will be implicitly tested by many tests.

Implicit testing of functionality is of course conflicting with the goals of that very issue. But as these are merely "suggestions" and not really hard expectations, I don't think we really need tests for them. The suggestions only apply if Block module is installed, and the current theme actually implements "content" and "help" regions. If that's not the case, nothing happens.

StatusFileSize
new2.76 KB
PASSED: [[SimpleTest]]: [MySQL] 34,452 pass(es).
[ View ]

I've shortened the two comments to be less wordy and fit on one line; no need for that verboseness. ;)

This looks ready to fly for me.

At least the documentation improvement should be backported. (though I don't see why the functional change couldn't either)

Note that the region property is currrently buggy. #512464: Auto-enable a block's 'status' with a non-empty 'region' when a module is installed aims to fix that problem and enable all blocks like that, instead of having long block lists in the installation profile.

@Niklas Fiekas: I'm aware of that issue (and rolled some patches for it). This patch is closely related. However, I'm no longer sure whether we actually want to do #512464, because it hard-codes a range of assumptions that only apply to core profiles and core themes. Anyway, to be discussed over there.

Status:Needs review» Needs work

+++ b/core/modules/system/system.moduleundefined
@@ -2029,8 +2029,11 @@ function system_user_timezone(&$form, &$form_state) {
+    // Auto-enable in 'content' region by default, if the theme defines one.

Themes have to define a 'content' region, I think.

After that: Yeah ... those are probably the only blocks where we can properly do this (changed my mind on this one), so let's get it in.

Status:Needs work» Needs review

Themes have to define a 'content' region, I think.

That would be new to me. On what code or functionality do you base that requirement?

AFAIK, it's a mere coincidence that all core themes have a 'content' region. Unless I overlooked some hidden spot, the content region is not hard-coded anywhere, so technically, a theme without a literal 'content' region but using a different region name should be possible.

The only hard-coded reference I know of exists in drupal_render_page(), but that is only triggered if no page region controlling module is enabled that dealt with the main content block already (e.g., Block, Panels/Panels Everywhere, etc).

What about the code about line 168(7.x) in system.admin.inc that flags an error if the 'content' region is undefined? And drupal documentation ( http://drupal.org/node/171205#regions ) on defining regions?

Thanks dcrocks. I already showed sun that in IRC, but forgot to reference it here.

StatusFileSize
new2.81 KB
PASSED: [[SimpleTest]]: [MySQL] 34,463 pass(es).
[ View ]

Fixed that comment and added pointers to the relevant functions, as discussed here.

Status:Needs review» Needs work

+++ b/core/modules/system/system.moduleundefined
@@ -2035,8 +2035,12 @@ function system_user_timezone(&$form, &$form_state) {
+    // @see system_themes_page(), drupal_render_page()

Providing this inline @see's are ok, I'd say RTBC.

Status:Needs work» Reviewed & tested by the community

Well ... needs work is not exactly RTBC ... whoops.

About the @see's:

(00:48:46) sun: NiklasFiekas: mmm, there's a header with title "See Also sections" on that page. However, apparently, the docs on that page are not in line with the actual implementation of the parser, which is able to handle consecutive @see pointers. Coder module even has explicit tests for that. Anyway, as this @see doesn't appear in phpDoc, but in an inline comment instead, it has no effect in the...
(00:48:48) sun: ...first place. ;)

Status:Reviewed & tested by the community» Patch (to be ported)

This seemed like something that needs a test, but in case anyone thinks that too, read back to #5/#6, I'm fine with getting this in to unblock that issue.

Committed/pushed to 8.x, moving to 7.x for backport.

Status:Patch (to be ported)» Needs review

I'm pretty sure this makes #1172560: The block X was assigned to the invalid region Y and has been disabled. a core bug. The problem is that you get a notice everytime a block is assigned to a region which doesn't exist. Now 'content' is a region that is required for all themes, but 'help' is not. So when using a theme which doesn't have that region, you will see those notices everytime you save the block admin page.
I'm moving to 'needs review' at least, I don't know if we want to fix this problem over there (and consequently bump that one to major) or here.

Bumping that other issue to major seems reasonable to me.

Version:8.x-dev» 7.x-dev
StatusFileSize
new1.31 KB
PASSED: [[SimpleTest]]: [MySQL] 39,255 pass(es).
[ View ]

Backport to D7.

Without this patch, #1373142: Use the Testing profile. Speed up testbot by 50% is pretty much impossible to backport to D7, since the appearance of these two blocks is assumed by default by a plethora of tests. Currently, they are manually configured and enabled via standard.install. The Standard profile is able to do that, because it installs Block module. The Testing profile is not able to do so, because Block module is optional, so at the time testing.install is run, Block module's tables do not even exist yet.

Status:Needs review» Reviewed & tested by the community

Looks good. I'll post a follow-up for the extra doc fixes in the D8 patch. Let's not hold #1373142: Use the Testing profile. Speed up testbot by 50% up on that.

Status:Reviewed & tested by the community» Needs review

Hm. I'm a bit concerned backport-wise with #20. I wonder if we should postpone this on #1172560: The block X was assigned to the invalid region Y and has been disabled. for D7. Thoughts?

Well #1172560: The block X was assigned to the invalid region Y and has been disabled. is really an easy issue code-wise, it's just stuck on whether we want to display a notice or not. Really it just needs some Drupal hero to go in there and say: "We don't want notices because ..." (I personally failed at conveying that in that issue.)

So in case this blocks something as important as #1373142: Use the Testing profile. Speed up testbot by 50% my personal view would be to just commit this to D7 and mark it needs work in D8 again or alternatively mark #1172560: The block X was assigned to the invalid region Y and has been disabled. major for D8 or something. That is, needless to say, just my two cents, though.

This may also impact #1293550: Setting a currently disabled theme as admin puts block admin in a broken state, at least for one case. All 3 may need to be done.

Issue tags:+needs backport to D7

Fixing tag.

Status:Needs review» Reviewed & tested by the community

#1172560: The block X was assigned to the invalid region Y and has been disabled. has been committed to Drupal 7 and the patch in #22 still applies cleanly.

Title:Assign system_main block to 'content' region and help block to 'help' region by defaultAssign system_main block to 'content' region and help block to 'help' region by default (followup)
Version:7.x-dev» 8.x-dev
Priority:Major» Normal
Status:Reviewed & tested by the community» Active
Issue tags:+7.15 release notes

Although #1172560: The block X was assigned to the invalid region Y and has been disabled. is committed, that still doesn't change the fact that after this patch, themes which don't have a 'help' region will get the ugly warning message displayed on the screen when the Block module is turned on...

On the other hand, that doesn't seem as bad as the bug which this fixes, so I went ahead and committed this to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/6d1ce38

This is a bit of a scary change, so hopefully we won't regret this :)

We should leave this open for D8, though, to figure out a better fix that doesn't result in that warning message (or at least file a followup issue for that)...

Also, why weren't the documentation changes from the D8 patch included in the D7 backport?

Status:Active» Fixed

after this patch, themes which don't have a 'help' region will get the ugly warning message displayed on the screen when the Block module is turned on

That's such a rare edge-case (which can be too easily worked around), I think we should just simply wait whether anyone will file bug report for it.

Thanks!

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