Problem/Motivation

Use-case 1:

When a profile installs a block for a theme, it is created for all enabled themes.
See e.g. the Multilingual Demo profile: https://www.drupal.org/project/multilingual_demo

For reproducing:

  1. Install the multilingual_demo profile (simplytest.me works perfectly for reproducing this bug)
  2. Go to "Single Export": admin/config/development/configuration/single/export
  3. Select "Block" for "Configuration type"
  4. In "Configuration name" you will see "duplicated" names. They are the blocks created for the different enabled themes.

I set component other as I cannot find the culprit yet.

Use-case 2:

The Thunder distribution wants to adopt Olivero as our default theme.

The problem is Olivero ships some blocks in config/install. They get installed first. After that, our Thunder Admin Theme gets enabled. We have some block configuration for that in config/optional of Thunder but that gets ignored because profile configuration gets imported at the end of the installation. That means for the Admin Theme block_theme_initialize runs during the installation and creates copies of oliveros blocks.

Proposed resolution

Remaining tasks

  • Attach a patch fixing it.
  • Review it manually.
  • Add tests for it.

User interface changes

None.

API changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

Component: other » install system

The culprit :)

penyaskito’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
708 bytes

Alexpott suggested this fix on IRC and worked for me.

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/modules/block/block.module
@@ -147,7 +147,7 @@ function block_themes_installed($theme_list) {
-  if (!$has_blocks) {
+  if (!$has_blocks && !drupal_installation_attempted()) {

Let's move this to block_themes_installed() so we save queries and loops during the installer.

alexpott’s picture

Component: install system » block.module

Wrong culprit... well it's kind of both of them so this seems only fair :)

jlbellido’s picture

Issue tags: +SprintWeekend2015

I'm going to work on this during the SprintWeekend2015.

jlbellido’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
542 bytes
1.04 KB

Changed #2 with the suggested changes in #3 and running tests.

I've tested it manually and it works for me. Tests still needed.

penyaskito’s picture

Status: Needs review » Needs work

Needs works, tests missing yet.

Anonymous’s picture

I have just stumbled upon this thing.
I would welcome a setting for the block module to disable this feature because themes are now able to provide their own block instances via configuration so if a theme does that and out of nothing there are blocks that have no place to be there the developer get's really confused.

I know this can be easily turned off by hook_module_implements_alter but I don't like it and I'd rather have a setting that I can turn on/off globally or per theme.

My issue is for the post-installation state but this issue covers it so I won't be creating a new one.

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

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should 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.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should 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.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should 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.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should 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.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should 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.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.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: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should 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.

chr.fritsch’s picture

Version: 8.9.x-dev » 9.2.x-dev
Status: Needs work » Needs review
FileSize
885 bytes

So this is really annoying. We are currently working on activating Olivero as the default theme for Thunder.

Olivero ships some blocks in config/install. They get installed first. After that the seven theme gets enabled. We have some block configuration for that in config/install of Thunder but that get's ignored because profile configuration get's imported at the end. That means for the seven theme block_theme_initialize runs and creates random blocks...

So here is a rerolled patch.

Status: Needs review » Needs work

The last submitted patch, 17: 2404105-17.patch, failed testing. View results

chr.fritsch’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
5.57 KB
6.44 KB

Here is a test.

The last submitted patch, 19: 2404105-19-FAIL.patch, failed testing. View results

alexpott’s picture

+++ b/core/modules/block/block.module
@@ -88,6 +89,12 @@ function block_page_top(array &$page_top) {
+  // Disable this functionality during the installation, because blocks might
+  // be installed as optional config.
+  if (InstallerKernel::installationAttempted()) {
+    return;
+  }

Should this also check if config is syncing for the same reasons?

Hmmm... I see why this is not really causing an issue... it's because we only do the random block creation when there are no blocks.

  // Initialize theme's blocks if none already registered.
  $has_blocks = \Drupal::entityTypeManager()->getStorage('block')->loadByProperties(['theme' => $theme]);
  if (!$has_blocks) {

I guess this is a super super rare situation during a config sync. Why is this not protected us during an install?

chr.fritsch’s picture

It depends on the themes that get installed. For themes with blocks in config/install it's not a problem, because this config get's directly imported during the installation. But for themes that only have blocks in config/optional or only have block config in the profile, these blocks get imported at the end of the installation and then block_theme_initialize runs before.

alexpott’s picture

Issue tags: +Needs followup

@chr.fritsch - thanks for answering the q. So adding a follow-up to deal with enabling such a theme via config sync seems something we should do.

chr.fritsch’s picture

So you are talking about the following use-case:

I have an installed site with some themes enabled. Then I add for example claro to the core.extensions file and execute a drush cim. Then I could image that the following two things could happen:
- the optional config of claro is installed
- block_theme_initialize creates blocks for claro

But instead, nothing of that happens. I think block_theme_initialize creates some blocks, but they get immediately deleted.

$ drush cim                                                                                                                                (9.2.x|✚6…)
+------------+----------------+-----------+
| Collection | Config         | Operation |
+------------+----------------+-----------+
|            | core.extension | Update    |
+------------+----------------+-----------+

 Import the listed configuration changes? (yes/no) [yes]:
 > y

 [notice] Synchronized extensions: install claro.
 [notice] Synchronized configuration: delete block.block.claro_tools.
 [notice] Synchronized configuration: delete block.block.claro_main_menu.
 [notice] Synchronized configuration: delete block.block.claro_footer.
 [notice] Synchronized configuration: delete block.block.claro_account_menu.
 [notice] Synchronized configuration: delete block.block.claro_branding.
 [notice] Synchronized configuration: delete block.block.claro_breadcrumbs.
 [notice] Synchronized configuration: delete block.block.claro_content.
 [notice] Synchronized configuration: delete block.block.claro_messages.
 [notice] Synchronized configuration: delete block.block.claro_powered.
 [notice] Synchronized configuration: delete block.block.claro_search.
 [notice] Synchronized configuration: delete block.block.claro_help.
 [notice] Synchronized configuration: delete claro.settings.
 [notice] Synchronized configuration: delete block.block.claro_local_actions.
 [notice] Synchronized configuration: delete block.block.claro_local_tasks.
 [notice] Synchronized configuration: delete block.block.claro_page_title.
 [notice] Finalizing configuration synchronization.
 [success] The configuration was imported successfully.

I think that is ok.

alexpott’s picture

@chr.fritsch yeah - but it'd be nice to not do the unnecessary creations and deletions.

chr.fritsch’s picture

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/modules/block/block.module
@@ -88,6 +89,12 @@ function block_page_top(array &$page_top) {
+  // Disable this functionality during the installation, because blocks might
+  // be installed as optional config.
+  if (InstallerKernel::installationAttempted()) {
+    return;
+  }

So this then prompts the question what if blocks are not installed as optional config?!?!?

Do we have to check if the theme does have optional config?

Don't we want to maintain this behaviour for themes with no blocks...

I think we need to call this from install_install_profile() if the block module is present for all themes that have been installed...

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
2.9 KB
8.21 KB

Right, I added it install_install_profile and also added a test for it.

chr.fritsch’s picture

Added some comments and found a way we don't have to change the installer.

alexpott’s picture

  1. +++ b/core/modules/block/block.module
    @@ -86,8 +87,16 @@ function block_page_top(array &$page_top) {
    +  // Disable this functionality during the installation, because blocks might
    +  // be installed as optional config.
    

    Let's flesh out this comment a bit.

      // Disable this functionality prior to install profile installation because
      // block configuration is often optional or provided by the install profile
      // itself. block_theme_initialize() will be called when the install profile is
      // installed.
    
  2. +++ b/core/modules/block/block.module
    @@ -86,8 +87,16 @@ function block_page_top(array &$page_top) {
    + * @see block_modules_installed
    

    Missing () - should be @see block_modules_installed()

  3. +++ b/core/modules/block/block.module
    @@ -133,6 +142,22 @@ function block_theme_initialize($theme) {
    +/**
    + * Implements hook_modules_installed().
    + */
    

    @see block_themes_installed()

  4. I think this patch has really nice test coverage of all the situations.
alexpott’s picture

I created #3182738: Delay calling hook_modules_installed() and hook_themes_installed() during site install for a larger discussion about hook_EXTENSION_installed() and site install.

chr.fritsch’s picture

chr.fritsch’s picture

Issue summary: View changes

Added our use case to the IS

daniel.bosen’s picture

Status: Needs review » Reviewed & tested by the community

I tested the issue by simply configuring the minimal theme to have olivero as the default theme. Without the patch, minimals admin theme (stark) gets some blocks added from Olivero on installation. For example the "Powered by Drupal" Block is added.

With the patch only the stark blocks, that are shipped in minimals config/install folder are created. This looks good!

alexpott’s picture

Title: When a profile installs a block for a theme, it is created for all enabled themes » [backport] When a profile installs a block for a theme, it is created for all enabled themes
Version: 9.2.x-dev » 9.1.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed 4a4770271b and pushed to 9.2.x. Thanks!

Going to ask other committers for a +1 to backport. We can't go as far as 8.9.x because we're missing

diff --git a/core/profiles/testing_themes_blocks/themes/testing_theme_optional_blocks/testing_theme_optional_blocks.info.yml b/core/profiles/testing_themes_blocks/themes/testing_theme_optional_blocks/testing_theme_optional_blocks.info.yml
index 1a37158f51..8d24aae829 100644
--- a/core/profiles/testing_themes_blocks/themes/testing_theme_optional_blocks/testing_theme_optional_blocks.info.yml
+++ b/core/profiles/testing_themes_blocks/themes/testing_theme_optional_blocks/testing_theme_optional_blocks.info.yml
@@ -13,4 +13,3 @@ regions:
   page_top: 'Page top'
   page_bottom: 'Page bottom'
   sidebar_first: 'First sidebar'
-
diff --git a/core/profiles/testing_themes_blocks/themes/testing_theme_required_blocks/testing_theme_required_blocks.info.yml b/core/profiles/testing_themes_blocks/themes/testing_theme_required_blocks/testing_theme_required_blocks.info.yml
index 7392a69ab3..baa33b506b 100644
--- a/core/profiles/testing_themes_blocks/themes/testing_theme_required_blocks/testing_theme_required_blocks.info.yml
+++ b/core/profiles/testing_themes_blocks/themes/testing_theme_required_blocks/testing_theme_required_blocks.info.yml
@@ -13,4 +13,3 @@ regions:
   page_top: 'Page top'
   page_bottom: 'Page bottom'
   sidebar_first: 'First sidebar'
-
diff --git a/core/profiles/testing_themes_blocks/themes/testing_theme_without_blocks/testing_theme_without_blocks.info.yml b/core/profiles/testing_themes_blocks/themes/testing_theme_without_blocks/testing_theme_without_blocks.info.yml
index 6e2a3b8ccd..6d47cb66fa 100644
--- a/core/profiles/testing_themes_blocks/themes/testing_theme_without_blocks/testing_theme_without_blocks.info.yml
+++ b/core/profiles/testing_themes_blocks/themes/testing_theme_without_blocks/testing_theme_without_blocks.info.yml
@@ -13,4 +13,3 @@ regions:
   page_top: 'Page top'
   page_bottom: 'Page bottom'
   sidebar_first: 'First sidebar'
-

Fixed coding standards on commit.

  • alexpott committed 4a47702 on 9.2.x
    Issue #2404105 by chr.fritsch, jlbellido, penyaskito, alexpott: When a...
alexpott’s picture

Title: [backport] When a profile installs a block for a theme, it is created for all enabled themes » When a profile installs a block for a theme, it is created for all enabled themes
Status: Patch (to be ported) » Fixed

Discussed with @catch and we agree that this can be backported.

  • alexpott committed 2f070fc on 9.1.x
    Issue #2404105 by chr.fritsch, jlbellido, penyaskito, alexpott: When a...

Status: Fixed » Closed (fixed)

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