block_theme_initialize should not create blocks during config sync

Discussed this with Berdir. Here is an initial patch.

Seems like a perfect test case scenario.

Actually not sure if this is the best approach? With or without the patch the theme being installed will not get the default blocks you would get when enabling a theme the normal way.

I don't know what you mean. That's the whole point of this issue? Config import must not never have any side effects, it should only import exactly the configuration that you prepared and nothing. You have to install as usual the theme at some point, then it initializes the blocks, then you customize them, and then you export and import the final state.

Ah just doesn’t seem like a valid workflow. But if that’s the case I don’t think there’s any test case to add. Unless there is way to see output from drush.

Based on #9 the patch in #6 does what is expected.

I manually edited my core.extension.yml file by adding bartik to it
Did a drush cim
Verified I did not see any blocks created.

Since with or without the patch the blocks aren't there after configuration is imported I'm not sure there is a test for this. Unless we can see the output of drush cim.

One way to test for it would be to have a hook_block_insert() implementation in a test module and registering what kind of blocks get deleted during config import, but yes, I'm not sure it's worth that.

That, or we make a kernel test where we directly call that function and simulate that config sync is on, that might be easier?

I vote for the 2nd option.

Actually not 100% how to simulate the act of importing partially. Any good examples?

I ended up here because I'm doing a migration from core 9.5.11 to 10.1.5. On 9.5.11 we had Adminimal theme as admin theme and now we are enabling and configuring the Claro theme. The enabled Claro blocks are all set in yml config files (default blocks, no custom modifications). The problem is that when I import the configuration using customer's environment database the following error shows up:

In EntityStorageBase.php line 519:                                                                                              
                                                                                                                                  
    'block' entity with ID 'claro_footer' already exists.

Using patch on #6 it avoids the import error. However, the blocks we have set in yml files doesn't get imported. Moreover, If do a "drush cex", it deletes the claro blocks I had defined on those files:

 [notice] Differences of the active config to the export directory:

+------------+-----------------------------------------+-----------+
| Collection | Config                                  | Operation |
+------------+-----------------------------------------+-----------+
|            | block.block.claro_breadcrumbs           | Delete    |
|            | block.block.claro_content               | Delete    |
|            | block.block.claro_messages              | Delete    |
|            | block.block.claro_local_actions         | Delete    |
|            | block.block.claro_page_title            | Delete    |
|            | block.block.claro_primary_local_tasks   | Delete    |
|            | block.block.claro_secondary_local_tasks | Delete    |
+------------+-----------------------------------------+-----------+

Why they get deleted? Why they don't get imported?

Once the configuration import using customer's environment database is done, If I uninstall claro and I reenable it using the UI, all the default blocks are recreated. Then, If I export the configuration and I compare it with previous claro blocks yml file, the only change is in the uuid property. So it seems that for deploying on customer environment, we would need to uninstall and install again the Claro theme.

Added tests and got everything to green. I used AI to help me put this together as I am still pretty green to writing tests.

The 3 tests are:

testNoBlocksCreatedDuringConfigSync
Verifies that block_themes_installed() does not create blocks during a config sync.

testBlockHooksModulesInstalledDuringSync
Confirms BlockHooks::modulesInstalled() avoids block creation during config sync.

testBlockHooksModulesInstalledWithoutSync
Ensures BlockHooks::modulesInstalled() does create blocks when not syncing.

Adding Needs tests tag back in. I am awful at this. If I attempt again, I will assign it to myself but if anyone want to work on this, I will thank them profusely.

You weren't far off, had to debug through it as well. kernel tests are hard, there are always hidden subtle things that do not match a default setup.

In this case, some issues were:

* there already were some blocks that were created as claro has default blocks so I emptied that, a test theme might be better for this.
* And then the test passed but for the wrong reason, because no default theme was set in the kernel test, that doesn't happen automatically, so had to that.

added a second test where we expect the initialize, refactored things to initialize shared things in setup, also use invoke() instead of calling the function as this will move to OOP soon.

Thanks for the work, the explanation and the inline comments. I made one more minor change to fix coding standards and now all the tests are green. Removing Needs tests tag and I will add a change record.

Noting this should also close #3372078: Themes installed by recipes have incorrect/duplicate block config when applied to a site that has an existing theme

This is a straightforward bug fix and the tests prove that the bug is, in fact, tested.

Setting to NW because the test added here is failing.

I refactored the test for clarity, but didn't really change what it does. Restoring RTBC here, since we'd really like this in 11.2. Feel free to send it back to NR for a separate set of eyes if that is unpalatable :)

Looks like the change to Drupal\Hook\BlockHooks::modulesInstalled() does not have tests? Was this intentional?

If not, it probably could be set up as a similar kernel test where a the block modules_installed hook is invoked on a profile in both syncing and non syncing cases to make sure blocks are and aren't added to installed themes correctly.

If there's urgency to get this in, maybe that test can be a follow up (or the modulesInstalled change reverted), if technically scope is about theme install only.

Let's do a similarly hacky test for modules_installed... it's all pretty side-effecty anyway.

Added test looks good (as well as the rest of it).

+1, I added a CR for the change in behavior and reviewed the test too.

This is a bug and there are no BC changes, I think this is fine to go in during a patch release so removing the target tag.
Requeued the failing test
Saving issue creds

Committed to 11.x and backported to 11.2.x

I think as this is a bug fix it could also be backported further. Setting to Patch to be ported to have branches created for 10.5/10.4 and 11.1

Thanks all

Published change record, which we may need to update if we backport

Could this be a breaking change if backported?

MRs for 10.4.x, 10.5.x, and 11.1.x up.

Could this be a breaking change if backported?

I'm not seeing it, as long as the CR is updated to point at the right version.

#3105597: Stop copying block configuration from active theme when enabling a new theme is at least related, if not duplicate.

This is going to save time during config import because the config import will delete all the blocks created by block theme initialize. This is why we've not noticed it till recipes because that's using the cofig sync flag to control what the module installer is doing.

Moving to NR for the port MRs.

The gitlab diff in 11.1.x looked weird, nothing from the reroll but gitlab just noting

But appears to be good backports.

Thanks for the RTBC @smustgrave. It’d be great to see this backported.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Committed 4726bd5 and pushed to 11.1.x. Thanks!
Committed 4e19d04 and pushed to 10.6.x. Thanks!
Committed 6d982d1 and pushed to 10.5.x. Thanks!

Did not push to 11.1.x - that's security only.

This issue reappears for me now with Drupal 11.3.0 release.
It seems the logic got moved into an OOP hook, but that might not run correctly for some reason?

See #3105597-43: Stop copying block configuration from active theme when enabling a new theme