Placing the same custom block, more than once, with a period in the title, results in an Error.

Hi guys,

Just confirming this bug. My steps to reproduce:

- A block named 1.3.1 was placed in a region.
- Set that block to 'none' (so it's placed in 'Disabled').
- Try to place that block in a region using the 'Place block' widget on the /admin/structure/blocks/ page at the desired region
- Error message: 'The website has encountered an unexpected error'.
- Log message: 'Drupal\Core\Entity\EntityStorageException: 'block' entity with ID '1.3.1abovecontent' already exists. in Drupal\Core\Entity\EntityStorageBase->doPreSave() (line 430 of /Users/markconroy/htdocs/adesignforlife/envato/adfl-alpha/docrooot/core/lib/Drupal/Core/Entity/EntityStorageBase.php).'

Note: If I got to the 'Disabled' section on the blocks page and choose the region I want the block to appear, it works fine from there. But I can't enable any other blocks (more than once) that also have a full stop/period in their names.

The issue here is that \Drupal\block\BlockForm::getUniqueMachineName treats the . as a config delimiter.

Simple fix is to disallow the . from block IDs. That makes sense anyway as it would mess with the config system, where the . has significance.

Failing test and fix.

1. +++ b/core/modules/block/src/BlockForm.php
   --- /dev/null
   +++ b/core/modules/block/tests/src/Kernel/BlockMachineNameTest.php
   
   +++ b/core/modules/block/tests/src/Kernel/BlockMachineNameTest.php
   @@ -0,0 +1,87 @@
   +namespace Drupal\Tests\block\Kernel;
   ...
   + * @group block
   ...
   +    $this->installEntitySchema('block_content');
   

   Shouldn't this be block_content, not block?

2. +++ b/core/modules/block/tests/src/Kernel/BlockMachineNameTest.php
   @@ -0,0 +1,87 @@
   +  public function testMachineNamesShouldNotCollideIfBlockTitlesContainPeriod() {
   

   descriptiveTestMethodNamesAreTheBest

3. +++ b/core/modules/block/tests/src/Kernel/BlockMachineNameTest.php
   @@ -0,0 +1,87 @@
   +    $id = $this->container->get('transliteration')->transliterate($block_content->label(), LanguageInterface::LANGCODE_DEFAULT, '_');
   +    $id = Unicode::strtolower($id);
   +    $id = preg_replace('@[^a-z0-9_.]+@', '', $id);
   

   Why not just use a string with the known result of this?

Fix looks good!

1) no, the bug is in BlockForm and BlockBase
2) :)
3) BlockBase::getMachineNameSuggestion is protected, I guess I could use reflection and call it instead of this dance, I have the plugin from $block->getPlugin() - thoughts?

1) Duh, of course. Thanks :)

3) I'm not averse to reflection in tests, so that wfm. I think it will be clearer.

Moved to reflection approach.

Also added user entity for 8.2/8.3 where we have a revision user for block content.

I think that is much clearer. Thanks!

Fixed some unused use statements on commit to 8.3.x

Leaving RTBC against 8.2.x, nice find.

I get nervous when I see support for something removed without an explanation of why it was there to begin with. I remember in the original BAP conversion that there was a whole lot of work specifically around supporting that dot operator. #2043527: Theme name is included in block machine name but should be stored as a key instead removed the pseudo-namespacing-by-theme from block instances and therefore the need for it.

However, what happens if someone has an existing block instance with the dot in its macine name on their site following this patch?

Oops, did not mean to change the branch.

That's a good question, we need an upgrade path for existing blocks at a minimum. Reverted for now.

+

I have checked and able to replicate the issue. I would suggest, if we can add a validation so that it would not allow period to add in Block Title. If you can check any of the contributed blocks placed doesn't have any period.

Rerolled the patch.

Hello

Just tested #26 2685917-block-period-exception.26.patch

1) Installed Drupal Core 8.3.x
2) Applied Patch successfully
3) Created a block called "This.is custom" and added to "Breadcrumb" region
4) Created a second block called "This.is custom2" and added to "Content" Region
5) Clicked Save
6) This saves successfully with no errors shown and no errors shown in logs.

Screen shot of success page

There's still no update path as per #17 and #20. As per #17 I think we need to support dots in custom block titles and work out why this is happening and fix in a different way.

I was also seeing this error in Drupal 8.3.2. There were two breadcrumb blocks created, but they were placed in different regions. One of the block was disabled. And while I was installing a module I was getting

Drupal\Core\Entity\EntityStorageException: 'block' entity with ID 'custom_admin_theme_breadcrumb...' already exists. in Drupal\Core\Entity\EntityStorageBase->doPreSave() (line 430 of /Users/micahjoyner/www/cmm.dev/core/lib/Drupal/Core/Entity/EntityStorageBase.php).

After I deleted the disabled block, the theme installed without any error.

I tested this on Drupal 10.1.x, standard install and was not able to reproduce the problem.

The patch here is fixing this by changing code in /core/lib/Drupal/Core/Block/BlockBase.php. That code was removed in #3091309: Broken context-aware block plugins throw an unexpected exception. I suspect this was fixed by that but can't be sure.

Therefore, closing as outdated. If you are experiencing this problem on a supported version of Drupal reopen the issue, by setting the status to 'Active', and provide complete steps to reproduce the issue (starting from "Install Drupal core").

Thanks!