Stop copying block configuration from active theme when enabling a new theme

Currently, this is postponed on #3103375: Let themes indicate whether they work as front-end and/or admin themes, based on the proposed resolution of "Copy block configuration from the current admin theme rather than the default theme when enabling an admin theme".
Is it possible this could be circumvented by changing this to "Copy block configuration from the current admin theme rather than the default theme when enabling an switching to a new admin theme"?

It doesn't seem like this copying of blocks is actually needed until a theme is specified as an admin theme (as opposed to enabled ) . It's also possible that a user may want to enable a theme specified as an admin-supporting theme to be used as the default, so it's not a given that a theme being enabled (even one that is traditionally admin) will benefit from having the block config copied. One of the proposed solutions in #3103375 is to make it possible to flag a theme as one that supports the admin end, as opposed to the specification limiting the theme to just admin or FE.

I think shipping specific blocks for Claro is a good idea. When people enable it and then expect the frontend blocks to stay the same that indeed sounds like a minor use case and it should happen relatively early in site building that there is not that much to manually fix then I think. I think this should help bridge the gap until Claro becomes the default and the 80% case will be people already having it when it again becomes less of a problem.

This is popping up in the Recipes initiative. We can enable and configure a theme, but because of the block duplication of the existing theme, a recipe creator needs to know which theme the recipe is installed over to be able to place/modify the blocks.

Since themes can (and do) provide blocks from their own config now, do we need the duplication functionality at all?

I agree with @thejimbirch in #15. Can we not just rip out the bizarre duplication stuff? Why do we still need it? AFAICT, it just interferes with the ability of themes to provide blocks that make sense for them.

Yes. I have also run into this problem, specifically while enabling themes with recipes. It does seem like pretty bizarre behavior to me, but I've never lived in a world without it... so :shrug:

It looks like this is happening in block.module, in the block_theme_initialize() function (which is called from the block_themes_installed() hook).

/**
 * Initializes blocks for installed themes.
 *
 * @param $theme_list
 *   An array of theme names.
 *
 * @see block_modules_installed()
 */
function block_themes_installed($theme_list) {
  // 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.
  if (InstallerKernel::installationAttempted() && \Drupal::config('core.extension')->get('module.' . \Drupal::installProfile()) === NULL) {
    return;
  }

  foreach ($theme_list as $theme) {
    // Don't initialize themes that are not displayed in the UI.
    if (\Drupal::service('theme_handler')->hasUi($theme)) {
      block_theme_initialize($theme);
    }
  }
}

It looks like we're already aware of some issues during profile installation and skip this block duplication then. If the duplication cannot be removed altogether, perhaps we could also make a special case and not duplicate if a recipe is being applied.

(Personally, I don't like the special treatment approach for recipes. Since a recipe can be applied to a site at any time, I don't think it makes much sense to treat them the same as an installation profile, rather it should be treated the same as though someone manually enabled the theme, at least by the theme system.)

Can we update this issue to be all themes and not just admin themes?

I see we have two paths forward we could take.

1. Remove the block duplication code completely and require themes to provide their own block configuration. AFAICT, they should/do already.

2. Add an exemption for recipes in block_theme_initialize() like there is for install profiles.

Is there any progress on this, or any way to circumvent this and move forward?
Recipes are included in 10.3.x, I feel like the priority of solving this should be a little higher now.

I can't seem to get it fixed myself, other than patching core and removing the functionality entirely, but that seems a little nuclear...

On the other hand, I personally do think that this copy functionality should be removed.
It definitely doesn't make much sense to copy blocks from one theme to another, given that regions could be entirely different.

I guess there is some consensus to get this functionality removed?
What are the downsides to this approach? Themes without their own block configuration would look empty after installation I guess?

If we don't want to take the removal route, is it an option to check for the usage of the 'config.storage.checkpoint' service?
We could potentially add extra logic in block_themes_installed(), that checks whether we just created a new checkpoint (which is what happens when installing a recipe, but not when manually enabling a theme)?
Kind of the same approach as the current logic that checks if we are during profile installation.

I'm not familiar enough with the checkpoint storage to know if that's feasible or not. Not sure if checkpoints get deleted afterwards - if not, this is not an option anyway.

Opened an MR with a naive first draft of this. Curious how much will break as a result. 😅

Looks like some stuff broke. :)

Failed functional tests were

- BlockHiddenRegionTest
- InstallerThemesBlocksProfileTest

Related to the config sync issue but definitely not a duplicate.

That said, I don't really agree with the issue.

Themes *can* provide default blocks and then the default blocks aren't copied. claro does that. If that wasn't done, then copying the default blocks seems like a reasonable fallback to me.

I hate block_theme_initialize(), because of #3529040: Weird admin blocks if drupal_cms_admin_ui used on a non-Drupal CMS site and #3563831: [regression] 11.3 Creating blocks via block_theme_initialize . The whole idea of it is just not really relevant in a world of recipes and themes providing default configuration.

Admin themes should provide default blocks. They should be opinionated -- they are defining most of the administrative UI! There's no need for core to babysit admin themes that inexplicably ship without a proper suite of blocks. I think we should expect admin themes to ship a complete look and feel, rather than trying to jerry-rig something together from the existing blocks. It just makes no sense to me.

Besides, aren't admin themes as a concept pretty much going away anyhow? My understanding is that Gin will be moved into core as a new theme called admin, and that's going to be the theme for Drupal's backend. Done and done.

IMHO, block_theme_initialize() is a relic of the distant past, when it was felt that admin themes would be, like, a thing. I'm not sure reality has really borne that out, frankly, although I could be wrong.

In Drupal CMS, I'm heavily leaning towards disabling \Drupal\block\Hook\BlockHooks::themesInstalled() until block_theme_initialize() is removed from core outright. This behavior causes more confusion and pain than it's worth, and we can safely assume our end users want the admin UI to be good, but they don't really want to mess with admin themes. They can reasonably expect any backend UI issues to be fixed in Navigation, Gin, or another part of core.

So if you ask me: get Gin into core, then deprecate the function completely, with no replacement, and burn this wart off in Drupal >=12.

EDIT: Granted, this function doesn't seem to differentiate between frontend themes and admin themes, but that's somehow even worse in my opinion. It violates the principle of least surprise with wanton abandon. A theme needs to be (most people expect it to be) a complete look and feel with opinions, not just a bunch of blocks you seamlessly graft from one region to another just because they have the same name. It's an anti-feature because it looks like buggy behavior to the uninitiated. That, right there, is in my book a very good reason to remove it.

Posted #3566782-14: Deprecate block_theme_initialize() and move the logic to a service in the hopes that we can just plain deprecate the function forever.

I was going back on forth on whether I should comment here at all as #33 is quite... emotional, but I couldn't resist in the end.

I understand that there are use cases where this is not desired. There are already multiple cases where this is explicitly skipped. One of them is if the theme in question provides at least one block on its own. This covers how claro admin theme works in core. And since #3182716: block_theme_initialize should not create blocks during config sync, it is also skipped when doing a config sync. I understand that recipes currently end up in a weird inbetween state, where they do _not_ install the config a theme provides but at the same time it's also not a config sync (or is it?). that just sounds like a straight up bug that we should fix.

That said, I think there are still valid use cases when this *is* desirable. For example with the starterkit theme approach. when you generate a new starterkit theme and make that your new default theme, then it possibly saves you a lot of work when you use your own theme going forward. Especially when you maybe already did site building and configured lots of things before switching to your own theme. Or when have a use case where you have multiple themes and vary it by domain or something like that. Typically, for a new non-admin theme, you *do* want to start with what your current default theme has.

I don’t suppose we could make it an explicitly opt-in feature, rather than an implicit one?

The behavior of “only do this if the theme has no blocks of its own” feels bizarre and surprising to me. What if we had an info file key where the theme could specifically ask to inherit the blocks from the active theme?

I thought about a .info.yml key too, but I I don't think it works whether that's opt in or opt out. 99% of themes that are created are going to be frontend themes and mostly want this behavior.

Recipe are not required. Taking gin as an example, it ships with it's own block config. It's only a problem when it's enabled as part of a recipe because then falls inbetween existing cases. How a theme is enabled is the primary condition of whether or not this should run or not (beside the theme itself being opinionated and shipping its own blocks).

A new starterkit theme initially probably should get blocks, but not if you enable it through a recipe or config deployment.

Disabling block_theme_initialize while applying a recipe would be an acceptable compromise, since that would at least hide the problem for Drupal CMS users.

But I have no idea how to implement that, except using some sort of icky flag mechanism.

Should enabling a recipe automatically set the isSyncing() option? Then this would already work and I'd assume that if you use a recipe, all existing logic "should I run some custom setup or not" would apply quite well to it? Something like media_library_install() for example or symfony_mailer_lite_install(). How does that work right now?

I think that would be a question for @alexpott. I know that the recipe system does set up and execute a config import, but that’s not in effect during the entire process of applying a recipe.

\Drupal\Core\Recipe\RecipeRunner::installTheme sets the syncing flag. #3182716: block_theme_initialize should not create blocks during config sync is meant to make this work. There is a comment in that issue however that this no longer works in 11.3. I suspect the reason is the theme OOP changes, we rebuild the container after a theme gets installed and then the new config installer doesn't have the syncing flag anymore.

I verified that the following changes result in the syncing check behave as expected:

diff --git a/core/lib/Drupal/Core/DrupalKernel.php b/core/lib/Drupal/Core/DrupalKernel.php
index e9707de59d7..8a3873cac25 100644
--- a/core/lib/Drupal/Core/DrupalKernel.php
+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -985,6 +985,7 @@ protected function initializeContainer() {
     $this->containerNeedsDumping = FALSE;
     $session_started = FALSE;
     $all_messages = [];
+    $is_syncing = FALSE;
     if (isset($this->container)) {
       // Save the id of the currently logged in user.
       if ($this->container->initialized('current_user')) {
@@ -1008,6 +1009,8 @@ protected function initializeContainer() {
       }

       $all_messages = $this->container->get('messenger')->all();
+
+      $is_syncing = $this->container->get('config.installer')->isSyncing();
     }

     // If the module list hasn't already been set in updateModules and we are
@@ -1070,6 +1073,8 @@ protected function initializeContainer() {
       }
     }

+    $this->container->get('config.installer')->setSyncing($is_syncing);
+
     \Drupal::setContainer($this->container);

     // Allow other parts of the codebase to react on container initialization in

In other words, carry over the syncing flag from the old to the new container, similar to the messages. The existing check in themesInstalled() then returns early.

This is newly broken in 11.3 after being fixed in 11.2, but it's also always been broken for hook_modules_installed() which are likely causing similar issues for hook implementations trying to respect the is syncing flag.

I'd suggest we create a new issue to fix that and then we can IMHO won't fix this.

Added #3572171: Persist is_syncing across container rebuilds to track it.