Block name collision on theme creation

Confirming this problem, which might result in an exception like this:
exception 'Drupal\Core\Entity\EntityStorageException' with message ''block' entity with ID '...' already exists.'

+1 for Aki Tendo's suggestion to re-use the implemented behavior of BlockForm::getUniqueMachineName(), which requires some refactoring first.

I have prepared a patch regarding this issue. It does not refactor the BlockForm as suggested though. It simply adapts the implementation of \Drupal\block\BlockForm::getUniqueMachineName() into block.module's block_theme_initialize($theme) function.

This is still an issue, and something I've run into when enabling a new subtheme. Attached is a rough patch for how this might look with the code pulled out into a separate service (which will undoubtedly break tests). If this is the kind of approach envisioned above I can work this into something more robust and update the associated test coverage.

This should fix BlockFormTest

Let's try that again with the correct interface file name and some tidying up

Hello everyone!
What the steps to reproduce this issue?

Hello thalles,

It's been a long time since my last check. But i think I remember the steps to reproduce this issue:

Initial setup

Prepare two themes:

• Theme 1: themeone, Active, default
• Theme 2: themetwo, Disabled

Steps

1. Using Theme 1 theme, place a block with id: a_block_with_a_cool_id
2. In the same theme, place a block with this id: themetwo_a_block_with_a_cool_id
3. Enable Theme 2. In the process, this will copy the blocks from the default theme themeone to themetwo

and prepend the theme machine name to the final block ID.

4. The copy process fails because, when trying to copy a_block_with_a_cool_id to themetwo, this will create themetwo_a_block_with_a_cool_id, resulting in a failure for ID duplicate (because already exists in themeone).

This is from memory, so I hope I don't give any wrong indication.

Thanks @Dakwamine!

For some reason it's required to add yourthememachinename_BLOCKID prefix to avoid the error.

Patch on #15 works like a charm!

#15 resolved this issue on an installation of 9.2.0 where our campus theme and module package initializes and configures custom blocks.

Patch #15 fixed the issue for me as well. Thanks

Re-roll of #15 on 9.4.

Oops. Forgot the patch...

Fixed code style failures.

Is #3061266: Prefix block machine name suggestions with the theme machine name relevant?

Reading https://www.drupal.org/project/drupal/issues/3061266 and that may be a duplicate or least trying to accomplish the same thing

Tested

With seven theme uninstalled
Created a block with machine name seven_page_title
Installed seven
Saw there is no seven_page_title block now when there should be.

Am I missing a step?

To answer #32, please see #2858897-17: Block name collision on theme creation to reproduce the issue. I've added that into the Issue Summary, since it's incredibly important.

Also, moving back to Needs Review, as I don't see NW steps in #32.

The work would be that seven page title block was never created.

So essentially I followed #17

Enabled themeA (claro in my case)
Created a block with machine name seven_page_title which is just a custom block
Enabled seven
seven_page_title is one of sevens block but it doesn't get created so now my theme has no block to add the title.

Updating related issue tickets. I could be wrong but I feel this may be blocked until https://www.drupal.org/project/drupal/issues/3061266 is in?

Updated/Reroll patch for D9.

@smustgrave

When your theme has the block e.g. breadcrumb (initial by installer) and default_themename_breadcrumb (created later), then drupal would create the block seven_breadcrumb and then it tries once again for the block default_themename_breadcrumb, because it replace default_themename with seven. Just take a look at block_theme_initialize.

With the patch the name is seven_breadcrumb_2.

Patches applies also for 10.0.x/10.1.x.

So retested #40 and confirmed on

D10.1.x
Created a block called stark_page_title IN the olivero theme
Installed stark
The created page title block for stark was given machine name stark_page_title_2

So the fix seems to be working only thing missing is a test case. The manual steps I did above should be good for one.

Tagged for IS update so it can be included that _2 will be added to conflicting blocks. Easier for the committer later.

Think this is very close good job everyone!

added patch against #40 in 10.1.x

Thank you for your interest but the patch in #40 applies to 10.1 so 43 was not needed

Hiding unnecessary re-roll in #43.

Resolving Issue Summary update to fix feedback in #41.

I added the test into \Drupal\Tests\block\Kernel\NewDefaultThemeBlocksTest::testNewDefaultThemeBlocks and also add native typehint for \Drupal\block\BlockMachineNameGeneratorInterface::getUniqueMachineName.

I merged the test into the existing test.

Fixed CCF for #47.
Please review.

@tobiasb thank you for the tests!

FYI for test time you can attach a test-only patch and then a full patch so we can see the test-only fail and the full patch pass.
Issue summary makes sense for what's being accomplished

Tested on 10.1.x by:
With stark uninstalled
Created a block called stark_page_title with matching machine name
Installed stark
Verified custom block stark_page_title was created with machine name stark_stark_page_title
Starks Page title was created wtih machine name stark_page_title_2

+1 from me.

Random ckeditor5 failure.

Thanks for working on this data integrity issue!

1. +++ b/core/modules/block/src/BlockForm.php
   @@ -88,14 +95,17 @@ class BlockForm extends EntityForm {
   +  public function __construct(EntityTypeManagerInterface $entity_type_manager, ExecutableManagerInterface $manager, ContextRepositoryInterface $context_repository, LanguageManagerInterface $language, ThemeHandlerInterface $theme_handler, PluginFormFactoryInterface $plugin_form_manager, BlockMachineNameGeneratorInterface $machine_name_generator) {
   

   Just documenting here that it's potentially OK to change the constructor so that the new service is required without a deprecation, since there is a factory method.

2. +++ b/core/modules/block/src/BlockMachineNameGenerator.php
   @@ -0,0 +1,53 @@
   +  public function getUniqueMachineName(string $suggestion) {
   
   +++ b/core/modules/block/src/BlockMachineNameGeneratorInterface.php
   @@ -0,0 +1,21 @@
   +  public function getUniqueMachineName(string $suggestion);
   

   This should have a return typehint.

3. +++ b/core/modules/block/src/BlockMachineNameGenerator.php
   @@ -0,0 +1,53 @@
   +    // Get all the blocks which starts with the suggested machine name.
   

   This is a bit grammatically weird. I'd suggest:

   Get all the block machine names that begin with the suggested string.

4. +++ b/core/modules/block/src/BlockMachineNameGeneratorInterface.php
   @@ -0,0 +1,21 @@
   + * Generates unique machine names for blocks.
   

   I would add "based on a suggested string" to this docblock. It's not a randomly generated ID.

5. +++ b/core/modules/block/src/BlockMachineNameGeneratorInterface.php
   @@ -0,0 +1,21 @@
   +   * Generates a unique machine name for a block.
   

   Again, I'd add "based on a suggested string."

6. +++ b/core/modules/block/src/BlockMachineNameGeneratorInterface.php
   @@ -0,0 +1,21 @@
   +   *   The suggested block id.
   

   "ID" should be capitalized.

7. +++ b/core/modules/block/tests/src/Kernel/NewDefaultThemeBlocksTest.php
   @@ -4,6 +4,8 @@
   +use function str_replace;
   +use function strtolower;
   

   I have never seen us add use statements for global PHP functions like this before. The only place anything like this in core exists is in the annotation component, which is copied from Doctrine and not our code. Let's remove them.

8. +++ b/core/modules/block/tests/src/Kernel/NewDefaultThemeBlocksTest.php
   @@ -33,10 +35,10 @@ public function testNewDefaultThemeBlocks() {
        // Add two instances of the user login block.
   

   I'd expand this to say "with machine names that will collide" to make it clear that this is intentional.

9. +++ b/core/modules/block/tests/src/Kernel/NewDefaultThemeBlocksTest.php
   @@ -72,6 +74,8 @@ public function testNewDefaultThemeBlocks() {
   +    // Collision therefore \Drupal\block\BlockMachineNameGenerator::getUniqueMachineName appends a counter.
   

   This comment should be wrapped at 80 characters. Also, I'd suggest:

   Since there is a collision with an existing block,
   the block machine name generator automatically appends a counter.

10. Last two points aside, I think it was actually better to have a dedicated test for the behavior of the two blocks colliding. (In general, we should avoid coupling the test coverage of different aspects of the API when possible.)

Thanks!

I'm actually halfway toward changing my mind on point 1; maybe we should default the services to NULL and raise a deprecation message. Form controllers are internal, but the change will break e.g. CacheBlockForm. So having a change record right there for them would be an improvement. If we do add that, though, it does not require a deprecation test. Reference:
https://www.drupal.org/about/core/policies/core-change-policies/drupal-d...

Either way the issue is minor-only (so 10.1.x-only for now) and needs a change record also.

Thanks!

Crediting @larowlan who helped me think through the consequences of the constructor change.

Attempted to address all points in #52

Took a shot at the CR also.

phpcs fix

Removed line from phpstan that was causing issue.

Fixed wrong function call.

+++ b/core/modules/block/src/BlockForm.php
@@ -88,14 +95,17 @@ class BlockForm extends EntityForm {
+   * @param BlockMachineNameGeneratorInterface $machine_name_generator
+   *   The unique machine name generator.
    */
-  public function __construct(EntityTypeManagerInterface $entity_type_manager, ExecutableManagerInterface $manager, ContextRepositoryInterface $context_repository, LanguageManagerInterface $language, ThemeHandlerInterface $theme_handler, PluginFormFactoryInterface $plugin_form_manager) {
...
     $this->storage = $entity_type_manager->getStorage('block');
     $this->manager = $manager;
     $this->contextRepository = $context_repository;
     $this->language = $language;
     $this->themeHandler = $theme_handler;
     $this->pluginFormFactory = $plugin_form_manager;
+    $this->machineNameGenerator = $machine_name_generator;

As I said above, let's change this so that the Machine name generator defaults to NULL and is loaded from the static container when NULL.

We could also use constructor property promotion here. Do this:

public function __construct(EntityTypeManagerInterface $entity_type_manager, ExecutableManagerInterface $manager, ContextRepositoryInterface $context_repository, LanguageManagerInterface $language, ThemeHandlerInterface $theme_handler, PluginFormFactoryInterface $plugin_form_manager, protected ?BlockMachineNameGeneratorInterface $machineNameGenerator = NULL)

Then we would not need the class member documentation about it.

Like this?

Looks like there's a bunch of out-of-scope stuff in the patch, and it's missing the deprecation for the NULL value of the service. I have a colleague who is working on a fix and will upload it shortly.

Thanks @xjm @smustgrave
I uploaded the Fail patch and the proper patch with the modifications and the grammar changes that the previous patches don't include

Could you upload an interdiff to see the changes please

Also #62 talks about adding protected ?BlockMachineNameGeneratorInterface $machineNameGenerator = NULL does that still need to happen?

sorry, I forgot to add the interdiff again the #61 patch because the #63 has a bunch of out-of-scope stuff.
And as I mentioned it applied the grammar changes suggested and the deprecation for the NULL value of the service with the trigger_error that has the change record link

Thanks!

Think the only thing missing is the last part of #62

Contrary to what I said in #62, for now we should only use constructor promotion in BlockMachineNameGenerator. We still have the issue that the existing parameters are snake_case and the class members are supposed to be lowerCamelCase and we're not supposed to mix both in the same file. There's an open coding standards issue to revisit that policy, but it hasn't been adopted yet. So for now, only wholly new classes should use constructor promotion.

Thanks @smustgrave and @fabiansierra5191. I think this is close! A few more things:

1. +++ b/core/modules/block/src/BlockForm.php
   @@ -88,14 +95,21 @@ class BlockForm extends EntityForm {
   -  public function __construct(EntityTypeManagerInterface $entity_type_manager, ExecutableManagerInterface $manager, ContextRepositoryInterface $context_repository, LanguageManagerInterface $language, ThemeHandlerInterface $theme_handler, PluginFormFactoryInterface $plugin_form_manager) {
   +  public function __construct(EntityTypeManagerInterface $entity_type_manager, ExecutableManagerInterface $manager, ContextRepositoryInterface $context_repository, LanguageManagerInterface $language, ThemeHandlerInterface $theme_handler, PluginFormFactoryInterface $plugin_form_manager, BlockMachineNameGeneratorInterface $machine_name_generator = NULL) {
   +    if ($machine_name_generator === NULL) {
   +      @trigger_error('Calling BlockMachineNameGenerator::__construct() without the $machine_name_generator argument is deprecated in drupal:10.1.0 and will be required in drupal:11.0.0. See https://www.drupal.org/node/3333575', E_USER_DEPRECATED);
   +      $machine_name_generator = \Drupal::service('block.machine_name_generator');
   +    }
   

   This looks good. The one thing we still need to do here is update the change record to mention the new machine name service and that it's now required here.

2. +++ b/core/modules/block/src/BlockForm.php
   @@ -377,7 +392,7 @@ protected function submitVisibility(array $form, FormStateInterface $form_state)
   -   * Generates a unique machine name for a block.
   +   * Based on a suggested string generates a unique machine name for a block.
   
   +++ b/core/modules/block/src/BlockMachineNameGeneratorInterface.php
   @@ -0,0 +1,21 @@
   +   * Based on a suggested string generates a unique machine name for a block.
   

   Sorry, I should have been more clear. This sentence needs to be the other way around:

   Generates a unique machine name for a block based on a suggested string.

3. +++ b/core/modules/block/src/BlockMachineNameGenerator.php
   @@ -0,0 +1,53 @@
   +final class BlockMachineNameGenerator implements BlockMachineNameGeneratorInterface {
   

   Is there any particular reasoning behind making this final? Why have an interface if the core implementation is final?

4. +++ b/core/modules/block/src/BlockMachineNameGenerator.php
   @@ -0,0 +1,53 @@
   +   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entityTypeManager
   +   *   Entity type manager.
   

   "The entity type manager service."

5. +++ b/core/modules/block/src/BlockMachineNameGenerator.php
   @@ -0,0 +1,53 @@
   +    // Iterate through potential IDs until we get a new one. E.g.
   +    // 'plugin', 'plugin_2', 'plugin_3', etc.
   

   Another "E.g.". This should be:

   For example, 'plugin', 'plugin_2', 'plugin_3', etc.

6. +++ b/core/modules/block/tests/src/Kernel/NewDefaultThemeBlocksTest.php
   @@ -23,13 +23,35 @@ class NewDefaultThemeBlocksTest extends KernelTestBase {
   +   * The theme installer service.
   +   *
   +   * @var \Drupal\Core\Extension\ThemeInstallerInterface
       */
   -  public function testNewDefaultThemeBlocks() {
   +  protected $theme_installer;
   +
   +  /**
   +   * The default theme.
   +   */
   +  protected $default_theme;
   +
   

   I should have caught this before, but as class members, these should use lowerCamelCase.

7. +++ b/core/modules/block/tests/src/Kernel/NewDefaultThemeBlocksTest.php
   @@ -23,13 +23,35 @@ class NewDefaultThemeBlocksTest extends KernelTestBase {
   +   * Check the blocks are correctly copied by block_themes_installed().
   

   Nit: "Checks that".

8. +++ b/core/modules/block/tests/src/Kernel/NewDefaultThemeBlocksTest.php
   @@ -59,9 +81,76 @@ public function testNewDefaultThemeBlocks() {
   +      // E.g., if the old theme has block.block.stark_admin,
   

   I was reminded today that folks find "i.e." and "e.g." confusing, so we should say "For example" instead.

   Reference: #2684683: [meta] Replace i.e. and e.g. with English words

9. +++ b/core/modules/block/tests/src/Kernel/NewDefaultThemeBlocksTest.php
   @@ -59,9 +81,76 @@ public function testNewDefaultThemeBlocks() {
   +    // Install a hidden base theme and ensure blocks are not copied.
   ...
   +    // Installing a hidden base theme does not copy the blocks from the default
   +    // theme.
   

   I think the second comment here is mostly just repeating the first, so we can remove the second one.

10. +++ b/core/modules/block/tests/src/Kernel/NewDefaultThemeBlocksTest.php
    @@ -59,9 +81,76 @@ public function testNewDefaultThemeBlocks() {
    +   * Checks a theme block is still created when same ID exists.
    

    This isn't still quite there... it needs to be "Checks that a theme block..."

11. +++ b/core/modules/block/tests/src/Kernel/NewDefaultThemeBlocksTest.php
    @@ -59,9 +81,76 @@ public function testNewDefaultThemeBlocks() {
    +    $new_theme = 'test_theme';
    

    Is there any reason to store test_theme to a variable, rather than using it as a string literal? It's the exact same number of characters to type either way, and actually more when the variable has to be concatenated into strings.

12. +++ b/core/modules/block/tests/src/Kernel/NewDefaultThemeBlocksTest.php
    @@ -72,12 +161,17 @@ public function testNewDefaultThemeBlocks() {
    +    // Collision therefore
    +    // \Drupal\block\BlockMachineNameGenerator::getUniqueMachineName
    +    // appends a counter.
    

    "The test_theme_user_login_block machine name is already in use, so therefore..."

+++ b/core/modules/block/src/BlockForm.php
@@ -88,14 +95,21 @@ class BlockForm extends EntityForm {
+   * @param BlockMachineNameGeneratorInterface $machine_name_generator
+   *   The unique machine name generator.
    */
-  public function __construct(EntityTypeManagerInterface $entity_type_manager, ExecutableManagerInterface $manager, ContextRepositoryInterface $context_repository, LanguageManagerInterface $language, ThemeHandlerInterface $theme_handler, PluginFormFactoryInterface $plugin_form_manager) {
+  public function __construct(EntityTypeManagerInterface $entity_type_manager, ExecutableManagerInterface $manager, ContextRepositoryInterface $context_repository, LanguageManagerInterface $language, ThemeHandlerInterface $theme_handler, PluginFormFactoryInterface $plugin_form_manager, BlockMachineNameGeneratorInterface $machine_name_generator = NULL) {

Oh, and since this defaults to null, it needs to be typed as BlockMachineNameGeneratorInterface|null in the docs and typehinted with nullable
?BlockMachineNameGeneratorInterface in the signature.

Thanks @xjm for the feedback,
I did all changes suggested except for the bullet 11 on the comment #71 test_theme variable is been using to replace the new theme in various places and I think we can keep it.

From what I can see points in #71 and #72 have been addressed

Good job!

1. +++ b/core/modules/block/src/BlockMachineNameGenerator.php
   @@ -0,0 +1,53 @@
   +/**
   + * {@inheritdoc}
   + */
   +class BlockMachineNameGenerator implements BlockMachineNameGeneratorInterface {
   

   Having a swappable block machine name generator service feels interesting. I'm not sure about this being part of the API. Also having {@inheritdoc} as a class doc feels wrong. Note sure I've seen that before.

   Plus I wonder how this fits in with
   #1643386: Strip useless "_" at beginning and end of JS-transliterated machine names which is pointing towards a need to consolidate the various places we generate machine names in core.

2. +++ b/core/modules/block/src/BlockMachineNameGenerator.php
   @@ -0,0 +1,53 @@
   +    // Get all the block machine names that begin with the suggested string.
   +    $query = $this->blockStorage->getQuery();
   +    $query->condition('id', $suggestion, 'CONTAINS');
   +    $block_ids = $query->execute();
   

   Do we need to add an accessCheck() method here?

Thanks @alexpott for the feedback!

I addressed your points by changing the comment for BlockMachineNameGenerator class and setting the accessCheck() method.

Hope this one is the one!

Appears #75 was addressed

@alexpott looking at #1643386: Strip useless "_" at beginning and end of JS-transliterated machine names seems to be removing a trailing _
Maybe they could adopt the solution we did here but that ticket may stalled as it hasn't been touched in 3 weeks.

.

Wrong file, so ticket moved to previous status

While I'm not 100% which patch is RTBC, all of the patches provided after @alexpott feedback are tripping "Custom commands failed" so that should be addressed before this can again be RTBC'd.

I think one of the weekly retests caused a phpstan error. Definitely wouldn't of marked RTBC if it was showing that before. So it's new.

#76 is the correct patch; #78 reverts one of the changes from it.

The failure on #76 is strange:

Running PHPStan on *all* files.
 ------ ----------------------------------------------------------------------- 
  Line   core/modules/block/src/BlockForm.php                                   
 ------ ----------------------------------------------------------------------- 
         Ignored error pattern #^Relying on entity queries to check access by   
         default is deprecated in drupal\:9\.2\.0 and an error will be thrown   
         from drupal\:10\.0\.0\. Call                                           
         \\Drupal\\Core\\Entity\\Query\\QueryInterface\:\:accessCheck\(\) with  
         TRUE or FALSE to specify whether access should be checked\.$# in path  
         /var/www/html/core/modules/block/src/BlockForm.php was not matched in  
         reported errors.                                                       
 ------ ----------------------------------------------------------------------- 

 ------ ----------------------------------------------------------------------- 
  Line   core/modules/block/src/BlockMachineNameGenerator.php                   
 ------ ----------------------------------------------------------------------- 
  36     Relying on entity queries to check access by default is deprecated in  
         drupal:9.2.0 and an error will be thrown from drupal:10.0.0. Call      
         \Drupal\Core\Entity\Query\QueryInterface::accessCheck() with TRUE or   
         FALSE to specify whether access should be checked.                     
         💡 See https://www.drupal.org/node/3201242                             
 ------ ----------------------------------------------------------------------- 

 [ERROR] Found 2 errors     

I think what this actually means is that the baseline is expecting an error there, and this patch is fixing it. So, we should try removing this hunk from the baseline as well and see if that resolves the issue:

                -
                 	message: "#^Relying on entity queries to check access b\
y default is deprecated in drupal\\:9\\.2\\.0 and an error will be thrown from drupal\\:10\\.0\\.0\\. Call \\\\Drupal\\\\Core\\\\Entity\\\\Query\\\\QueryInterface\\:\\:accessCheck\\(\\) with TRUE or FALSE to specify whether access should be checked\\.$#"
                        count: 1
                        path: modules/block/src/BlockForm.php

I have someone working on this now.

I'm working on this

New changes were introduced on core/modules/block/src/BlockForm.php, so I did a rebase from #76 and fixed a conflict, and then I removed the line on core/phpstan-baseline.neon that was causing phpstan to fail.

diff --cc core/modules/block/src/BlockForm.php
index 8eb188020e,4583ed58c0..0000000000
--- a/core/modules/block/src/BlockForm.php
+++ b/core/modules/block/src/BlockForm.php
@@@ -387,28 -402,7 +402,32 @@@ class BlockForm extends EntityForm 
     */
    public function getUniqueMachineName(BlockInterface $block) {
      $suggestion = $block->getPlugin()->getMachineNameSuggestion();
++<<<<<<< HEAD
 +    if ($block->getTheme()) {
 +      $suggestion = $block->getTheme() . '_' . $suggestion;
 +    }
 +
 +    // Get all the blocks which starts with the suggested machine name.
 +    $query = $this->storage->getQuery();
 +    $query->condition('id', $suggestion, 'CONTAINS');
 +    $block_ids = $query->execute();
 +
 +    $block_ids = array_map(function ($block_id) {
 +      $parts = explode('.', $block_id);
 +      return end($parts);
 +    }, $block_ids);
 +
 +    // Iterate through potential IDs until we get a new one. E.g.
 +    // 'plugin', 'plugin_2', 'plugin_3', etc.
 +    $count = 1;
 +    $machine_default = $suggestion;
 +    while (in_array($machine_default, $block_ids)) {
 +      $machine_default = $suggestion . '_' . ++$count;
 +    }
 +    return $machine_default;
++=======
+     return $this->machineNameGenerator->getUniqueMachineName($suggestion);
++>>>>>>> df87c63266 (#76)
    }

Thanks @schlaukopf! Testing locally, this seems to resolve the PHPStan issue, and also addresses the merge conflict introduced by #3061266: Prefix block machine name suggestions with the theme machine name.

So we now have the following in core/modules/block/src/BlockForm.php:

   public function getUniqueMachineName(BlockInterface $block) {
     $suggestion = $block->getPlugin()->getMachineNameSuggestion();
     if ($block->getTheme()) {
       $suggestion = $block->getTheme() . '_' . $suggestion;
     }
+    return $this->machineNameGenerator->getUniqueMachineName($suggestion);

Question then is, does the logic of adding the theme prefix belong in BlockForm, or should we inject the theme into the machine name generator?

Regarding #75.1 and whether we should consolidate machine name generation APIs, I think that's out of scope for this issue, since this is still fixing a major-possibly-critical data integrity bug. First of all, we'll need separate implementations in JS and PHP regardless -- the JS generates a suggested machine name for the user, but the PHP has to do its own validation either way.

Tagging for a followup issue to explore how we might consolidate the machine name generation APIs.

Test #84

With claro + olivero installed
Created a block with machine name stark_page_title
Installed stark theme and get fatal error
Block is not created for stark theme
Applied patch
Repeated steps
Block got created for stark with machine name stark_stark_page_title

Opened #3345038: Possibly consolidate machine name generation API per #87

As per #86, I still think the theme prefixing from the other issue needs to be moved out of the form controller and into the new API, or we at least need to discuss it. Thanks!

My mistake think I read #86 and #87 as the same

if we want to move the theme prefixing to the new API, I think we need to move this from block_theme_initialize (core/modules/block/block.module) as well

if (str_starts_with($default_theme_block_id, $default_theme . '_')) {
  $id = str_replace($default_theme, $theme, $default_theme_block_id);
}
else {
  $id = $theme . '_' . $default_theme_block_id;
}
$id = \Drupal::service('block.machine_name_generator')->getUniqueMachineName($id);

perhaps the whole if/else statement?

So relooking at #86 and I can't think of a reason not to add

if ($block->getTheme()) {
$suggestion = $block->getTheme() . '_' . $suggestion;
}

To the generator. Would make it the 2nd parameter (maybe default it to NULL?)

For #91

That could probably be included also in the generator. Not sure if it that and the if statement above could be combined to do the same?

I moved the theme name as a function parameter, I didn't move the whole if-statement from #91, because I'm not sure if replacing the old theme prefix should apply to all calls to that function.

This should fix the error in the pipeline

This should fix the error in the pipeline

This seems better by moving. Thanks!

The patch in #97 is over 600kb but the interdiffs are 766 bytes 🤔

I forgot to rebase, so this is after #84 where I move the theme prefixing into the new API.

Face palming myself for missing that. Thanks @laurii!

Followed the same steps in #88

random failure User.Drupal\Tests\user\Kernel\Migrate\MigrateUserStubTest

I'm not sure #75 has been addressed, largely the 'do we want a separate service for this' question

Tagging for #106, @tim.plunkett thoughts?

It's probably overkill, but it fits with a standard Drupal level of overkill :D

Reviewed the code overall, and again with the lens of #106, and I think this is good to go

This could be a new method on the BlockRepository, instead of a separate service?

#101 no longer applies.

I got a conflict with core/phpstan-baseline.neon file, I check the patch from #101 and removed those lines.
Please review.

Reroll looks good.

We have two core committers here questioning the need for a service and an interface for this (#109, #75) and I tend to agree

I agree, it would make sense to put this on the block repository. If we don't expect people to swap this out, there's no need for an interface either.

Saving some reviewer credits.

- Rebased onto 11.x
- Moved method onto Block Repository service and removed block name generator service
- Updated deprecation warning
- Fixed test after rebase

Think that covers concerns from #75, #108 and #115 - setting as needs review.

CR will also have to be updated

I've updated the CR - its also ready for review.

I missed that it looks like the pipeline above failed, but its showing as tests pass on the issue itself. Rebasing and running again.

Thanks for the CR update!

Committed and pushed 8339e0ebe6c to 11.x and 61030f7a4af to 10.2.x. Thanks!

Here is a roll of this patch for D10.1.x that removes the `core/phpstan-baseline.neon` change that has already been applied to core.