Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#22 | drupal.system-blocks-default-regions.22.patch | 1.31 KB | sun |
#16 | drupal8.system-blocks-default-regions.16.patch | 2.81 KB | sun |
#7 | drupal8.system-blocks-default-regions.7.patch | 2.76 KB | sun |
#4 | drupal8.system-blocks-default-regions.4.patch | 2.83 KB | sun |
#1 | drupal8.system-blocks-default-regions.1.patch | 1.07 KB | sun |
Comments
Comment #1
sunWe 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.
Comment #2
sunComment #3
sunoopsie, I forgot the docs changes from #0 in #1.
So will need a re-roll, but let's get some reviews first.
Comment #4
sunMerged the docs-change in.
Comment #5
xjmThis seems like something that merits a test, but I'm not sure we can add the test without using the testing profile?
Comment #6
sunAs 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.
Comment #7
sunI'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)
Comment #8
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedNote 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.
Comment #9
sun@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.
Comment #10
sun#7: drupal8.system-blocks-default-regions.7.patch queued for re-testing.
Comment #11
sun#7: drupal8.system-blocks-default-regions.7.patch queued for re-testing.
Comment #12
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThemes 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.
Comment #13
sunThat 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).
Comment #14
dcrocks CreditAttribution: dcrocks commentedWhat 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?
Comment #15
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThanks dcrocks. I already showed sun that in IRC, but forgot to reference it here.
Comment #16
sunFixed that comment and added pointers to the relevant functions, as discussed here.
Comment #17
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedProviding this inline @see's are ok, I'd say RTBC.
Comment #18
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedWell ... needs work is not exactly RTBC ... whoops.
About the @see's:
Comment #19
catchThis 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.
Comment #20
tstoecklerI'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.
Comment #21
xjmBumping that other issue to major seems reasonable to me.
Comment #22
sunBackport 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.
Comment #23
tstoecklerLooks 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.
Comment #24
webchickHm. 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?
Comment #25
tstoecklerWell #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.
Comment #26
dcrocks CreditAttribution: dcrocks commentedThis 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.
Comment #27
tim.plunkettFixing tag.
Comment #28
Devin Carlson CreditAttribution: Devin Carlson commented#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.
Comment #29
sun#22: drupal.system-blocks-default-regions.22.patch queued for re-testing.
Comment #30
David_Rothstein CreditAttribution: David_Rothstein commentedAlthough #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?
Comment #31
sunThat'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!