Problem/Motivation

If you create a new theme and don't define your own regions, it will inherit the default regions defined in core/lib/Drupal/Core/Extension/ThemeHandler.php. These regions are defined in a seemingly random order, so that e.g. header is below content, and footer is in the middle of the list:
Screenshot of default regions.

This makes it hard to visually scan for the region you need, and is pretty much guaranteed to not reflect the rendered order of the regions.

Proposed resolution

Re-order the regions to follow generally accepted practices. Obviously this is somewhat subjective, but I think we can all agree on some sane defaults, such as header being at the top, and footer being at the bottom.

Remaining tasks

Review patch.

User interface changes

Display of block regions on admin/structure/block/list will change, but this should be a strictly visual administrative change- underlying code and rendered output will not change.

API changes

None

Data model changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dane Powell created an issue. See original summary.

Dane Powell’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
1.48 KB

This patch adjusts the default order to roughly match Bartik's:

Header
Primary menu
Secondary menu
Page top
Page bottom
Highlighted
Breadcrumb
Help
Content
Sidebar first
Sidebar second
Footer

Status: Needs review » Needs work

The last submitted patch, 2: drupal-2603598-2.patch, failed testing.

Dane Powell’s picture

Status: Needs work » Needs review
FileSize
3.32 KB
1.65 KB

Ugh, so this caused the block tests to fail. This is because they work by hardcoding the index of the table row in the block admin ui corresponding to a certain region, and then checking that the block exists at that row (barf).

This patch fixes that by instead looking for the block and then confirming that its region is set correctly- no hardcoding of magic numbers.

Dane Powell’s picture

Issue tags: +Usability
joelpittet’s picture

I hope this can get into 8.0.1 or something.

star-szr’s picture

Just want to say thanks for this! We can probably get it in after release in an 8.0.x version or 8.1.x version, doesn't seem to be disruptive at all.

Dane Powell’s picture

If you can review the patch and mark this issue as Reviewed and Tested by the Community, that would go a long way towards getting it into core.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +mobile

Yes the order is much better. #mobilefirst

NaheemSays’s picture

In past theming for Drupal 7, I have noticed that when enabling and switching themes, blocks from regions that no longer exist in the new theme are placed in the first region listed.

Often this is the header and this can often make the site temporarily unuseable.

I do not know if this is still the case with Drupal 8, but when listing regions, this would be useful to consider (and potentially list the content region first as it is generally the most adaptable to handle generic blocks).

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

#10 is entirely correct - see block_theme_initialize() - it still exists in D8. So we definitely need to think about how the result of system_default_region() is worked out. I don't think that the current behaviour is correct (sidebar_first) and I don't think the behaviour with the current patch is correct either.

Dane Powell’s picture

@alexpott what would you prefer, placing the content region first, or modifying block_theme_initialize() to place blocks into the content region if available, instead of the first available region?

alexpott’s picture

Version: 8.0.x-dev » 8.1.x-dev
Issue tags: +Needs change record

@Dane Powell well the issue is the system_default_region() logic. OTOH it has behaved this way at least since Drupal 7. The default region in Bartik for both 7 and 8 is header. The problem with this change is that is changing the result of system_default_region() for themes with no regions. I think what we should do here is move this 8.1.x and proceed but also file a follow up to allow themes to declare a different default region from the first region. As this will change the default region for themes which don't declare any regions I think we need a very small CR for the new behaviour.

Dane Powell’s picture

Okay, so we all agree that the current method of picking the default region is ridiculous. But can we please not let that get in the way of committing this relatively unrelated patch?

Before the patch, the default region for themes not defining their own would be sidebar_first, which is pretty ridiculous.

After the patch, the default region is header, which is equally ridiculous.

So technically, yes this is a change, but such a trivial one that I don't see the need for a CR, and the whole default region picking system should be revamped as a follow-up ticket.

Dane Powell’s picture

Status: Needs work » Needs review
Dane Powell’s picture

joachim’s picture

I think we could fix this on 8.0.x if:

- we fix #2635166: block_theme_initialize() falls back to putting blocks in the first region it finds, provide a better default, making the new theme property optional
- as part of that, we set the default region to 'header' for themes that don't define regions and don't define a default region (the default default ;), at least on 8.0.x. For 8.1.x, regionless themes can maybe have 'content' as a better default? But that's a minor point
- we then fix this issue, and themes that don't define regions continue to have the same default region.

joachim’s picture

Hmm nope, what am I saying? That's not quite right -- #2635166: block_theme_initialize() falls back to putting blocks in the first region it finds, provide a better default is only for 8.1.x, as it adds new functionality.

But what we can do on 8.0.x is preserve the default region that's returned for themes that do not specify any regions, like this:

function system_default_region($theme) {
  // Special case to retain behaviour with themes that define no regions and
  // rely on the default region list.
  $theme_raw_info = \Drupal::service('info_parser')->parse($module->getPathname());
  if (empty($theme_raw_info['regions'])) {
    return 'sidebar_first';
  }

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

It's not very pretty, but it allows us to fix this UX bug on 8.0.x. For 8.1.x, we can change the behaviour and provide a CR.

joachim’s picture

Updated patch that includes my suggestion from above.

This wouldn't need a change record, as the default region behaviour is unchanged. As such, we can fix this on 8.1.x and also on 8.0.x.

Once this bug is fixed, we can implement #2635166: block_theme_initialize() falls back to putting blocks in the first region it finds, provide a better default on 8.1.x. As part of that, we'd probably want to clean up clean up the special case handling added in this patch, and thus at that point do a CR for the behaviour change.

NaheemSays’s picture

Is there any guarantee that sidebar_first will exist?

joelpittet’s picture

re #20 I don't think so as the defaults are unioned so if defined in our theme it will override the defaults. More likely 'content' will exist... but still it may not.

joachim’s picture

> Is there any guarantee that sidebar_first will exist?

--- a/core/lib/Drupal/Core/Extension/ThemeHandler.php
+++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php

@@ -1138,13 +1138,21 @@ function system_system_info_alter(&$info, Extension $file, $type) {
+  // Special case to retain current behaviour for themes that define no regions
+  // (and thus rely on the default list of regions).
+  $theme = \Drupal::service('theme_handler')->getTheme($theme_name);
+  $theme_raw_info = \Drupal::service('info_parser')->parse($theme->getPathname());
+  if (empty($theme_raw_info['regions'])) {
+    return 'sidebar_first';
+  }
+

Yes there is. Because we only do this for the themes that do not define ANY regions in their theme.info file. Therefore they get exactly the list of defaults from ThemeHandler::rebuildThemeData().

Does the code need a bit more commenting to explain this more clearly?

joelpittet’s picture

@joachim is there a reason it fallback to sidebar_first and not content when the theme doesn't provide any? And shouldn't the ActiveTheme hold that information in the registry instead of having to parse the info yaml again?

dcrocks’s picture

Not sure why you need to special case themes that don't include region definitions in their info.yml. The way I read the code they will get exactly the default region list. Stark, for example, doesn't specify any regions, yet when you install it it has exactly the default region list. And since sidebar_first is currently 1st in that list, it will be the default region for Stark.

dcrocks’s picture

One possible argument for the current ordering of the list is that the default region will be sidebar_first. If it were the header then a bunch of blocks that would be inappropriate for the header would show up there. For example, for stark, with the new list the 'powered_by' and 'footer' blocks would appear in the header. No matter what list is used any admin will immediately clean all that up. It is my impression from the issue summary the only purpose for this issue is that the admin block layout UI not look so unnatural for new themes without any region designations in their info.yml. Maybe sidebar_first should be left on top.

joachim’s picture

> is there a reason it fallback to sidebar_first and not content when the theme doesn't provide any?

Yes, to preserve the current default region behaviour, so this bug can be fixed on 8.0.x as well as 8.1.x.

> And shouldn't the ActiveTheme hold that information in the registry instead of having to parse the info yaml again?

AFAICT, if we look at ActiveTheme we will get the actual list of regions, and for a theme that doesn't specify any that will be the default list. So in that case, we can't tell if it was a theme with no regions in its info.yml, or a theme that just happened to define the same list itself.

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 reroll

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.

anya_m’s picture

alexpott’s picture

Issue tags: +Needs tests

@anya_m thanks for working on this issue.

+++ b/core/modules/system/system.module
@@ -1123,13 +1123,21 @@ function system_system_info_alter(&$info, Extension $file, $type) {
+  // Special case to retain current behaviour for themes that define no regions
+  // (and thus rely on the default list of regions).
+  $theme = \Drupal::service('theme_handler')->getTheme($theme_name);
+  $theme_raw_info = \Drupal::service('info_parser')->parse($theme->getPathname());
+  if (empty($theme_raw_info['regions'])) {
+    return 'sidebar_first';
+  }

This shouldn't be done here. Doing it potentially means unnecessary file system reads. Maybe we could calculate the default region in ThemeHandler::rebuildThemeData().

We also need a test the special case scenario.

Status: Needs review » Needs work

The last submitted patch, 34: 2603598-34.patch, failed testing. View results

andypost’s picture

+++ b/core/modules/system/system.module
@@ -1123,13 +1123,21 @@ function system_system_info_alter(&$info, Extension $file, $type) {
-function system_default_region($theme) {
-  $regions = array_keys(system_region_list($theme, REGIONS_VISIBLE));
+function system_default_region($theme_name) {
+  // Special case to retain current behaviour for themes that define no regions
+  // (and thus rely on the default list of regions).
+  $theme = \Drupal::service('theme_handler')->getTheme($theme_name);

I think the related should came in first

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.

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.

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.