Block name collision on theme creation

+++ 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
   ✨ Strip useless "_" at beginning and end of JS-transliterated machine names Fixed 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 ✨ Strip useless "_" at beginning and end of JS-transliterated machine names Fixed 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.

As the latest test results mention the accessCheck() function is not required here https://www.drupal.org/node/3201242 → , so about the last changes I remove this but kept the comment for BlockMachineNameGenerator class

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 ✨ Prefix block machine name suggestions with the theme machine name Fixed .

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 📌 Possibly consolidate machine name generation API Active 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?

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.

The last submitted patch, 93: 2858897_93.patch, failed testing. View results →

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

The last submitted patch, 101: 2858897_101.patch, failed testing. View results →

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.

Automatically closed - issue fixed for 2 weeks with no activity.