block_theme_initialize should not create blocks during config sync

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. 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?

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 🐛 Claro installed by recipes has incorrect block config Active

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

larowlan → closed merge request !11926

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.

📌 Copy block configuration from admin theme when enabling an admin theme Active 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.