Prefix block machine name suggestions with the theme machine name

Hello here is a patch for this issue.

Thanks for the review.

Hello,

Here is the fixed patch for the issue.

Thanks for the review.

This patch looks like it's only tests so far. I'm not seeing anything that implements the change the issue summary talks about.

retagging

In Umami profile the blocks are prefixed with the theme name: umami_branding for exemple

+++ b/core/modules/block/src/BlockForm.php
@@ -146,7 +146,7 @@ public function form(array $form, FormStateInterface $form_state) {
-      '#default_value' => !$entity->isNew() ? $entity->id() : $this->getUniqueMachineName($entity),
+      '#default_value' => !$entity->isNew() ? $entity->id() : $this->getUniqueMachineName($entity, $theme),

@@ -405,12 +405,15 @@ protected function submitVisibility(array $form, FormStateInterface $form_state)
-  public function getUniqueMachineName(BlockInterface $block) {
+  public function getUniqueMachineName(BlockInterface $block, $theme) {
...
+    $suggestion = $theme . '_' . $suggestion;

This is the actual change made by the patch, the rest is updating tests. There is an issue when a theme is not preselected when adding the block.

If you go to /en/admin/structure/block/add/local_tasks_block/, the machine name has the default theme in the name but when you use the select to change the theme (and region) the machine name is not updated.

To be the least problematic we could do:

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

That would avoid changing the signature of a public method as an added bonus.

@nod_ I am working on it.

@nod_ Hi i am very happy to working with you.I have changed the patch as suggest by you in #18
Could you please review .
Thanks
Pavnish

Checking failed test cases

Fixing PHP lint error.

1. +++ b/core/modules/block/src/BlockForm.php
   @@ -388,12 +388,17 @@ protected function submitVisibility(array $form, FormStateInterface $form_state)
   +   * @param string $theme
   +   *   The theme machine name in which the block is placed.
   

   This needs to be removed, the parameter doesn't exists anymore

2. +++ b/core/modules/block/tests/src/Unit/BlockFormTest.php
   @@ -130,7 +130,7 @@ public function testGetUniqueMachineName() {
   @@ -140,18 +140,18 @@ public function testGetUniqueMachineName() {
   
   @@ -140,18 +140,18 @@ public function testGetUniqueMachineName() {
   -    $this->assertEquals('test_2', $block_form_controller->getUniqueMachineName($blocks['test']));
   +    $this->assertEquals('theme_machine_name_test_2', $block_form_controller->getUniqueMachineName($blocks['test']));
   ...
   -    $this->assertEquals('other_test_3', $block_form_controller->getUniqueMachineName($blocks['other_test']));
   -    $this->assertEquals('other_test_3', $block_form_controller->getUniqueMachineName($blocks['other_test_1']));
   -    $this->assertEquals('other_test_3', $block_form_controller->getUniqueMachineName($blocks['other_test_2']));
   +    $this->assertEquals('theme_machine_name_other_test_3', $block_form_controller->getUniqueMachineName($blocks['other_test']));
   +    $this->assertEquals('theme_machine_name_other_test_3', $block_form_controller->getUniqueMachineName($blocks['other_test_1']));
   +    $this->assertEquals('theme_machine_name_other_test_3', $block_form_controller->getUniqueMachineName($blocks['other_test_2']));
   ...
   -    $this->assertEquals('last_test', $block_form_controller->getUniqueMachineName($last_block));
   +    $this->assertEquals('theme_machine_name_last_test', $block_form_controller->getUniqueMachineName($last_block));
   

   This is not necessary, the test works fine as-is.

Hi @nod_
Thanks for the feedback. implemented the feedback. Kindly review.
Thanks

Looks good to me

The issue title makes this look like more of a change than it actually is. Looks sensible to me and reduces the chance for collisions.

However, this fails cspell:

home/catch/www/drupal/core/modules/views/tests/fixtures/update/block.block.bartik_exposedformtest_exposed_blockpage_1.yml:10:12 - Unknown word (exposedformtest)
/home/catch/www/drupal/core/modules/views/tests/fixtures/update/block.block.bartik_exposedformtest_exposed_blockpage_1.yml:10:36 - Unknown word (blockpage)

I didn't see how that config was used, I removed it and the few tests I ran still worked, let's see what testbot says.

can't replicate the failures locally... adding back the config and renamed it.

so... load bearing config file huh.

I'm pretty sure it used to work this way and we undid it for reasons. So yeah this leads me to #2043527: Theme name is included in block machine name but should be stored as a key instead where we kinda of did the opposite of this. But that was a bit different because this is about the id and not the dot structure of the config name. I think this change is okay but I've pinged @tim.plunkett to gather other thoughts.

Another good thing is that block_theme_initialize() supports this way of naming blocks so that makes sense.

1. +++ b/core/modules/block/tests/src/Functional/Views/DisplayBlockTest.php
   similarity index 95%
   rename from core/modules/block_content/tests/modules/block_content_test/config/install/block.block.foobargorilla.yml
   
   rename from core/modules/block_content/tests/modules/block_content_test/config/install/block.block.foobargorilla.yml
   rename to core/modules/block_content/tests/modules/block_content_test/config/install/block.block.classy_foobargorilla.yml
   
   +++ b/core/modules/block_content/tests/modules/block_content_test/config/install/block.block.classy_foobargorilla.yml
   @@ -5,7 +5,7 @@ dependencies:
   -id: foobargorilla
   +id: classy_foobargorilla
   

   Why have we changed this config - we're only changing a suggestion - we don;'t have to have a theme name here and many existing sites won't so let's not make it look like it is enforced.

2. +++ b/core/modules/block_content/tests/src/Functional/BlockContentCreationTest.php
   similarity index 91%
   rename from core/modules/config/tests/config_override_integration_test/config/install/block.block.config_override_test.yml
   
   rename from core/modules/config/tests/config_override_integration_test/config/install/block.block.config_override_test.yml
   rename to core/modules/config/tests/config_override_integration_test/config/install/block.block.classy_config_override_test.yml
   
   rename to core/modules/config/tests/config_override_integration_test/config/install/block.block.classy_config_override_test.yml
   index be0616ff50..250010bf66 100644
   
   index be0616ff50..250010bf66 100644
   --- a/core/modules/config/tests/config_override_integration_test/config/install/block.block.config_override_test.yml
   
   --- a/core/modules/config/tests/config_override_integration_test/config/install/block.block.config_override_test.yml
   +++ b/core/modules/config/tests/config_override_integration_test/config/install/block.block.classy_config_override_test.yml
   
   +++ b/core/modules/config/tests/config_override_integration_test/config/install/block.block.classy_config_override_test.yml
   +++ b/core/modules/config/tests/config_override_integration_test/config/install/block.block.classy_config_override_test.yml
   @@ -1,4 +1,4 @@
   
   @@ -1,4 +1,4 @@
   -id: config_override_test
   +id: classy_config_override_test
   
   +++ b/core/modules/config/tests/config_override_integration_test/src/CacheabilityMetadataConfigOverride.php
   @@ -19,9 +19,9 @@ public function loadOverrides($names) {
        $state = \Drupal::state()->get('config_override_integration_test.enabled', FALSE);
   -    if (in_array('block.block.config_override_test', $names) && $state !== FALSE) {
   +    if (in_array('block.block.classy_config_override_test', $names) && $state !== FALSE) {
          $overrides = $overrides + [
   -        'block.block.config_override_test' => [
   +        'block.block.classy_config_override_test' => [
              'settings' => ['label' => 'Overridden block label'],
            ],
          ];
   @@ -49,7 +49,7 @@ public function createConfigObject($name, $collection = StorageInterface::DEFAUL
   
   @@ -49,7 +49,7 @@ public function createConfigObject($name, $collection = StorageInterface::DEFAUL
       */
      public function getCacheableMetadata($name) {
        $metadata = new CacheableMetadata();
   -    if ($name === 'block.block.config_override_test') {
   +    if ($name === 'block.block.classy_config_override_test') {
   
   similarity index 94%
   rename from core/modules/config/tests/config_override_test/config/install/block.block.call_to_action.yml
   
   rename from core/modules/config/tests/config_override_test/config/install/block.block.call_to_action.yml
   rename to core/modules/config/tests/config_override_test/config/install/block.block.classy_call_to_action.yml
   
   rename to core/modules/config/tests/config_override_test/config/install/block.block.classy_call_to_action.yml
   index 8951c0d22b..e6b80a6efd 100644
   
   index 8951c0d22b..e6b80a6efd 100644
   --- a/core/modules/config/tests/config_override_test/config/install/block.block.call_to_action.yml
   
   --- a/core/modules/config/tests/config_override_test/config/install/block.block.call_to_action.yml
   +++ b/core/modules/config/tests/config_override_test/config/install/block.block.classy_call_to_action.yml
   
   +++ b/core/modules/config/tests/config_override_test/config/install/block.block.classy_call_to_action.yml
   +++ b/core/modules/config/tests/config_override_test/config/install/block.block.classy_call_to_action.yml
   @@ -5,7 +5,7 @@ dependencies:
   
   @@ -5,7 +5,7 @@ dependencies:
        - block_content
      theme:
        - classy
   -id: call_to_action
   +id: classy_call_to_action
   
   +++ b/core/modules/config/tests/config_override_test/src/PirateDayCacheabilityMetadataConfigOverride.php
   @@ -23,9 +23,9 @@ public function loadOverrides($names) {
   -      if (in_array('block.block.call_to_action', $names)) {
   +      if (in_array('block.block.classy_call_to_action', $names)) {
            $overrides = $overrides + [
   -          'block.block.call_to_action' => [
   +          'block.block.classy_call_to_action' => [
   
   +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/FieldBlockTest.php
   similarity index 90%
   rename from core/modules/locale/tests/modules/locale_test/config/optional/block.block.test_default_config.yml
   
   rename from core/modules/locale/tests/modules/locale_test/config/optional/block.block.test_default_config.yml
   rename to core/modules/locale/tests/modules/locale_test/config/optional/block.block.classy_test_default_config.yml
   
   +++ b/core/modules/locale/tests/modules/locale_test/config/optional/block.block.classy_test_default_config.yml
   @@ -3,7 +3,7 @@ status: true
   -id: test_default_config
   +id: classy_test_default_config
   
   +++ b/core/modules/locale/tests/src/Kernel/LocaleConfigManagerTest.php
   @@ -87,7 +87,7 @@ public function testGetDefaultConfigLangcode() {
   -      'id' => 'test_default_config',
   +      'id' => 'classy_test_default_config',
   
   @@ -110,12 +110,12 @@ public function testGetDefaultConfigLangcode() {
   -    $this->assertNull(\Drupal::service('locale.config_manager')->getDefaultConfigLangcode('block.block.test_default_config'), 'The block.block.test_default_config is not shipped configuration.');
   +    $this->assertNull(\Drupal::service('locale.config_manager')->getDefaultConfigLangcode('block.block.classy_test_default_config'), 'The block.block.classy_test_default_config is not shipped configuration.');
   ...
   -    $this->assertEqual('en', \Drupal::service('locale.config_manager')->getDefaultConfigLangcode('block.block.test_default_config'), 'The block.block.test_default_config is shipped configuration.');
   +    $this->assertEqual('en', \Drupal::service('locale.config_manager')->getDefaultConfigLangcode('block.block.classy_test_default_config'), 'The block.block.classy_test_default_config is shipped configuration.');
   
   +++ b/core/tests/Drupal/KernelTests/Core/Config/CacheabilityMetadataConfigOverrideTest.php
   @@ -66,14 +66,14 @@ public function testConfigEntityOverride() {
   -    $block = $entity_type_manager->getStorage('block')->load('call_to_action');
   +    $block = $entity_type_manager->getStorage('block')->load('classy_call_to_action');
   ...
   -    $this->assertEqual(['config:block.block.call_to_action', 'pirate-day-tag'], $block->getCacheTags());
   +    $this->assertEqual(['config:block.block.classy_call_to_action', 'pirate-day-tag'], $block->getCacheTags());
   

   Same with all these. I don't think we should be changing them if we don't have to.

3. +++ b/core/modules/locale/tests/src/Kernel/LocaleConfigManagerTest.php
   @@ -110,12 +110,12 @@ public function testGetDefaultConfigLangcode() {
   diff --git a/core/modules/views/tests/fixtures/update/block.block.bartik_exposed_form_test_exposed_block_page_1.yml b/core/modules/views/tests/fixtures/update/block.block.bartik_exposed_form_test_exposed_block_page_1.yml
   
   diff --git a/core/modules/views/tests/fixtures/update/block.block.bartik_exposed_form_test_exposed_block_page_1.yml b/core/modules/views/tests/fixtures/update/block.block.bartik_exposed_form_test_exposed_block_page_1.yml
   new file mode 100644
   
   new file mode 100644
   index 0000000000..3e4547104d
   
   index 0000000000..3e4547104d
   --- /dev/null
   
   --- /dev/null
   +++ b/core/modules/views/tests/fixtures/update/block.block.bartik_exposed_form_test_exposed_block_page_1.yml
   

   Why do we need this?

Just doing a little tag tidying. Nice work everyone!!

I think adding the theme name to the suggestion is fine and makes sense. But I also think that this is just a suggestion and should not in anyway be mandated by a coder rule. Therefore I think we should do the changes recommended in #39 as we shouldn't be making unnecessary test changes to comply with a Coder rule that core is unlikely to ever adopt.

I just made a copy of a subtheme.

I was very surprised to see the site messed up because CSS and JQuery could not be applied anymore.

What I observed:
the blocks were created in the first subtheme, and the name of the original subtheme was not included in the original machine name, so the blocks' ID were similar to #block-blockname
when I copied the theme, all blocks had new names such as #block-themename-blockname

this is very confusing and creates a lot of unnecessary work when copying themes.

if including the name of the theme in the block ID becomes the standard, then I would suggest to also make it a standard when a block is created in the original theme.

and maybe admin users should be warned about it when creating a block, that they are not allowed to name the block as they want and that they can't give it an ID that remains stable from one theme to another.

Sounds like a possible duplicate of https://www.drupal.org/project/drupal/issues/2858897

Cleaning out old tags from 2019-2021. Per #49, linking #2858897: Block name collision on theme creation as a similar issue.

Could almost say this one is a blocker for https://www.drupal.org/project/drupal/issues/2858897

Sounds like it needs reroll but also address the issues in #39

Working on this issue.

re-rolled the #32 patch for 9.5.x.

Un-assigning this task.

Updating the IS to note the work that needs to be done, as #53 inadvertently moved to Needs Review (NR).

This still needs work (NW), see #3061266-39: Prefix block machine name suggestions with the theme machine name:

I think adding the theme name to the suggestion is fine and makes sense. But I also think that this is just a suggestion and should not in anyway be mandated by a coder rule. Therefore I think we should do the changes recommended in #39 as we shouldn't be making unnecessary test changes to comply with a Coder rule that core is unlikely to ever adopt.

Reroll the patch #53 with Drupal 9.5.x

#55, please see the status change and justification from #54. As you did not mention any fix to the feedback in #54, moving back to Needs Work.

Following the suggestion in #54. So going back to patch #37 and making the changes mentioned in #39

Hiding patches #53 and #55

#37 was a while ago that the intediff didn't full generate.

Fixed some test failures

Big thanks to @smustgrave for following the issue comments and moving us forward. Per #58 into #61, updating the IS to update where we're at.

This looks good to me.

Re #47 I don't think this changes things - theming using the block ID is probably not a good idea, you should be using a class added by the template.

Committed and pushed to 10.1.x, thanks!