Currently, system_default_region() simply returns the first region in the list of regions. This isn't great, because the first displayed region is usually something like 'header', which in general is not a good default.

Proposed resolution

Fall back to the content region if there is one, and only after that fall back to the first region found.

Potentially improve this logic more generally, by allowing themes to specify roles/hints for their regions, in #3278410: block_theme_initialize() sledgehammers square blocks into round regions, allow themes to provide a mapping of regions to 'roles'.

A previous solution was to allow themes to specify their default region, but this is likely to be 'content' in most cases, so doesn't necessarily offer a lot more than improving the fallback logic without configuration.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dane Powell created an issue. See original summary.

dcrocks’s picture

Status: Active » Needs review
FileSize
716 bytes

Here is a patch that adds a parameter('block_default_region') to the theme's info.yml file. If it is set in a theme, when that theme goes thru install any blocks merged from the current theme that don't have matching regions to the new theme will go into the region specified in 'block_default_region' if it exists. Otherwise it falls back to the old rule.
For example, I changed stark.info.yml to:

name: Stark
type: theme
description: 'An intentionally plain theme with no styling to demonstrate default Drupal’s HTML and CSS. Learn how to build a custom theme from Stark in the <a href="https://www.drupal.org/theme-guide">Theming Guide</a>.'
package: Core
version: VERSION
core: 8.x
base theme: false
block_default_region: 'content'

This moved several blocks that were previously placed in 'sidebar_first' to 'content' when the stark theme was installed. If you have a better suggestion for the parameter name, please say so.

Status: Needs review » Needs work

The last submitted patch, 2: 2635166-2.patch, failed testing.

swentel’s picture

Name sounds fine to me.

  1. +++ b/core/modules/block/block.module
    @@ -126,8 +126,13 @@ function block_theme_initialize($theme) {
    +        $block->setRegion($info['block_default_region']);
    

    needs indentation

  2. +++ b/core/modules/block/block.module
    @@ -126,8 +126,13 @@ function block_theme_initialize($theme) {
    +        else {
             $block->setRegion(system_default_region($theme));
    

    same here

dcrocks’s picture

Status: Needs work » Needs review
FileSize
812 bytes
981 bytes

Here is another patch with a small improvement and formatting fixes.

Status: Needs review » Needs work

The last submitted patch, 5: 2635166-5.patch, failed testing.

Jeff Burnz’s picture

don't want bike shed this but underscores feel a bit out of place here, how about just...

default region: content or region default: content

dcrocks’s picture

Status: Needs work » Needs review
FileSize
954 bytes
960 bytes

Trying to get logic right.

Status: Needs review » Needs work

The last submitted patch, 8: 2635166-8.patch, failed testing.

dcrocks’s picture

Status: Needs work » Needs review
FileSize
752 bytes
733 bytes

system_get_info() returns an empty array in some of the tests so the way the code was formtted generated errors. Hopefully this fixes it.

note that since I test that the region provided by block_default_region is a valid region if an invalid one is specified the fail is silent. It just shows the old behavior.

tim.plunkett’s picture

+++ b/core/modules/block/block.module
@@ -126,7 +126,13 @@ function block_theme_initialize($theme) {
+        $info = system_get_info('theme', $theme);
...
         $block->setRegion(system_default_region($theme));

Shouldn't this logic be located in system_default_region? That function should respect this setting, not just block.module's invocation of it.

dcrocks’s picture

The only call to this function in core is from the block module. I guess I just thought of this as a block thing and not a theme thing. Could move it easily but any other use cases for system_default_region?

tim.plunkett’s picture

You're right that it's only called by block module in core.
But it's in system.module, and anyone in contrib could call it.

Anyone calling it would likely want the same thing block module gets...

joachim’s picture

Status: Needs review » Needs work

Nothing other than block module uses it in core, but we've no idea whether anything else might use it.

So I agree with #11: this logic should be somewhere where system_default_region() can find it.

So either in system_default_region(), or baked into the stored theme info in ThemeHandler::rebuildThemeData()?

joachim’s picture

Hmm although I think it makes more sense in ThemeHandler::rebuildThemeData(), there's a problem with doing that:

function system_default_region($theme) {
  $regions = array_keys(system_region_list($theme, REGIONS_VISIBLE));
  return isset($regions[0]) ? $regions[0] : '';
}

The default for themes that don't set a default region is the first region in the list, *out of the visible ones*.

The problem is that in ThemeHandler::rebuildThemeData(), we don't have the list of hidden regions, because that's something system module adds in the alter hook:

function system_system_info_alter(&$info, Extension $file, $type) {
  // Remove page-top and page-bottom from the blocks UI since they are reserved for
  // modules to populate from outside the blocks system.
  if ($type == 'theme') {
    $info['regions_hidden'][] = 'page_top';
    $info['regions_hidden'][] = 'page_bottom';
  }
}

So we need to add the 'default region' key to the theme info before we invoke the alter hook, but we don't know which regions are definitely visible until the alter hook has run and has altered the data :(

dcrocks’s picture

Status: Needs work » Needs review
FileSize
1.39 KB
662 bytes

This patch moves the code to system_default_region in system.module.

Status: Needs review » Needs work

The last submitted patch, 16: 2635166-14.patch, failed testing.

dcrocks’s picture

Status: Needs work » Needs review
FileSize
673 bytes

Sorry, wrong patch. This is the real #14.

dcrocks’s picture

Shouldn't system_default_region() be returning BlockInterface::BLOCK_REGION_NONE instead of '' when a theme has no regions?

joachim’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/system/system.module
    @@ -1145,7 +1145,13 @@ function system_system_info_alter(&$info, Extension $file, $type) {
    +      return $info['block_default_region'];
    +    }
    

    The indentation is wrong here.

    The existing return needs to be indented too now that it's in an else {} block.

> Shouldn't system_default_region() be returning BlockInterface::BLOCK_REGION_NONE instead of '' when a theme has no regions?

Probably not, because a) that constant is part of block module, which system module doesn't depend on, and b) that would be an API change rather than an addition, which we can't do in 8.x.x.

We should maybe take this opportunity to add some docs to this that explain how the default region is figured out.

dcrocks’s picture

Shouldn't this need a change record as well?

dcrocks’s picture

Status: Needs work » Needs review
FileSize
1.19 KB
1.16 KB

Small changes

dcrocks’s picture

I added a draft change record Themes can specify default region for block placement. Please review.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

markhalliwell’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Was about to RTBC this, but realized there is no test for this. Patch still applies though.

joachim’s picture

Also:

+++ b/core/modules/system/system.module
@@ -1136,7 +1136,11 @@ function system_system_info_alter(&$info, Extension $file, $type) {
+ * Gets the name of the default region for a given theme, which will be one of:
+ *   A region name specified by 'block_default_region' in the theme's info.yml

First line of a docblock should be a single line paragraph.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

shivam kaushal’s picture

Assigned: Unassigned » shivam kaushal
shivam kaushal’s picture

Assigned: shivam kaushal » Unassigned
Status: Needs work » Needs review
FileSize
833 bytes

Status: Needs review » Needs work

The last submitted patch, 37: Allow-themes-to-specify-default-region-2635166-36.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

anmolgoyal74’s picture

Status: Needs work » Needs review
FileSize
867 bytes
772 bytes

system_get_info() is deprecated has been removed in 9.x.
https://www.drupal.org/node/2709919

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

alexpott’s picture

Category: Feature request » Task
Issue tags: -Needs tests
FileSize
2.38 KB

The original issue summary mentioned choosing the content region as the default region if it exists as a better choice than header. After discussing with @catch and @spokje it was felt that this is a better approach than having yet another thing to configure. This makes the behaviour of block_theme_initialise a bit better because stuffing everything into the header region by default is not a great look.

Added test coverage of the change too.

No interdiff because this is a new approach based on the existing issue summary.

alexpott’s picture

The reason this came up today is due to #3278342: Theming blocks bonanza and how themes are supposed to look after being switched to.

Attached to this comment are some before and after screenshots for install 9.4.x standard and then installing bartik and setting to the default theme. Not going to embed the screenshots because they are a full browser window.

alexpott’s picture

Improved the useless comment to be more useful.

The last submitted patch, 42: 2635166-42.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 44: 2635166-44.patch, failed testing. View results

joachim’s picture

> The original issue summary mentioned choosing the content region as the default region if it exists as a better choice than header. After discussing with @catch and @spokje it was felt that this is a better approach than having yet another thing to configure

Ok, but then that's no longer what this issue is about.

And what if a theme doesn't have a 'content' region?

I'd say the new proposal is maybe a quicker fix to the problem, and could get in as a separate issue which is a much quicker fix.

alexpott’s picture

Fixing the tests.

alexpott’s picture

Status: Needs work » Needs review
  1. +++ b/core/modules/block/tests/src/Functional/BlockTest.php
    @@ -253,7 +253,7 @@ public function testBlock() {
    -      'region' => 'left_sidebar',
    +      'region' => 'sidebar_first',
    

    The left_sidebar region does not exist in stark so this was relying (unintentionally) on system_default_region()

  2. +++ b/core/modules/block/tests/src/Functional/BlockUiTest.php
    @@ -67,7 +67,7 @@ protected function setUp(): void {
    -        'tr' => '6',
    +        'tr' => '5',
    

    Because a block has moved from the header to the content region the row in the block UI that this block is in has changed.

catch’s picture

We often have multiple approaches to fix the same problem on one issue, sometimes it's easier to spin different approaches out, sometimes it's easier to keep discussion in one place. Don't have a strong preference with this one.

Having said that, on providing more of an API for this, I think rather than a single default region, what would help with the UX of switching default themes and blocks landing in more appropriate places, is the ability to provide a mapping of regions to 'roles' - 'header', 'primary navigation', 'secondary navigation', 'highlighted', 'primary sidebar', 'secondary sidebar', 'breadcrumbs', 'content', 'footer'. Then if both the old theme and new theme map a region to one of those roles, the blocks could be moved 1-1 even if the regions have different names.

joachim’s picture

True, but the issue summary and even the issue title don't reflect the latest approach at all.

And agreed on the region roles approach.

catch’s picture

Title: Allow themes to specify default region » block_theme_initialize() falls back to putting blocks in the first region it finds, provide a better default
Issue summary: View changes
joachim’s picture

Status: Needs review » Needs work

Thanks.

So the roadmap is a quick fix in this issue, and a more comprehensive follow-up in #3278410: block_theme_initialize() sledgehammers square blocks into round regions, allow themes to provide a mapping of regions to 'roles'? That sounds good to me.

+++ b/core/modules/system/system.module
@@ -951,8 +951,8 @@ function system_system_info_alter(&$info, Extension $file, $type) {
+  return isset($regions['content']) ? 'content' : key($regions) ?? '';

I think a comment to explain the order of fallback here would be good.

Also, do we need the ?? at the end? Would there ever be a case where a theme has no regions at all?

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture