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.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

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.

sun’s picture

Title: Auto-suggest to assign system_main block to 'content' region by default » Assign system_main block to 'content' region and help block to 'help' region by default
sun’s picture

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

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

sun’s picture

Merged the docs-change in.

xjm’s picture

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

sun’s picture

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.

sun’s picture

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)

Niklas Fiekas’s picture

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.

sun’s picture

@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.

sun’s picture

Issue tags: -Testing system
sun’s picture

Issue tags: +Testing system
Niklas Fiekas’s picture

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.

sun’s picture

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).

dcrocks’s picture

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?

Niklas Fiekas’s picture

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

sun’s picture

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

Niklas Fiekas’s picture

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.

Niklas Fiekas’s picture

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. ;)

catch’s picture

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.

tstoeckler’s picture

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.

xjm’s picture

Bumping that other issue to major seems reasonable to me.

sun’s picture

Version: 8.x-dev » 7.x-dev
FileSize
1.31 KB

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.

tstoeckler’s picture

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.

webchick’s picture

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?

tstoeckler’s picture

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.

dcrocks’s picture

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.

tim.plunkett’s picture

Issue tags: +Needs backport to D7

Fixing tag.

Devin Carlson’s picture

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.

sun’s picture

David_Rothstein’s picture

Title: Assign system_main block to 'content' region and help block to 'help' region by default » Assign 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?

sun’s picture

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.