Remove unique constraint on block content info field

I recently came to the same conclusion. Why should content blocks be any different than other content entities which don't have such a constraint? Drupal doesn't prevent us from adding duplicate node titles or taxonomy terms.

This was probably from before we had UUIDs

Back in Drupal 7 these were likely using in hook block info

But now we use the uuid

Looks like there some tests for uniqueness that will need updating.

Thanks - it looks like either tests didn't re-run or it's still reporting failures

We'll need an update path test here

You can see an example from another issue here

Thanks for your work on this. This fixed the "A custom block with block description BLOCK NAME already exists." errors that.

For the update path test mentioned in #11

Also MR may need to be updated to point to 9.5

Took a shot. Using a patch off the 9.5 branch.

Tagging for accessibility

If we allow for blocks to have the same now could introduce a 508 issue where links have the same description.

Transferring credit from #3054197: Cannot add a custom block with a block description already used by a non-reusable block

1. +++ b/core/modules/block_content/block_content.install
   @@ -11,3 +11,17 @@
   +  $entity_type = 'block_content';
   +  $field = 'info';
   

   I don't think we need these local variables, lets just pass the strings to the function, they're only used once

2. +++ b/core/modules/block_content/tests/src/Functional/BlockContentCreationTest.php
   @@ -72,12 +72,12 @@ public function testBlockContentCreation() {
        // Check that attempting to create another block with the same value for
   -    // 'info' returns an error.
   +    // 'info' doesn't return an error.
        $this->drupalGet('block/add/basic');
        $this->submitForm($edit, 'Save');
    
        // Check that the Basic block has been created.
   -    $this->assertSession()->pageTextContains('A custom block with block description ' . $edit['info[0][value]'] . ' already exists.');
   +    $this->assertSession()->pageTextContains('basic ' . $edit['info[0][value]'] . ' has been created.');
   

   I think we should just remove these lines now, and end the test with the 'Custom Block found in database' assertion

   We don't need to test for the absence of a constraint

3. +++ b/core/modules/block_content/tests/src/Functional/BlockContentCreationTest.php
   @@ -153,12 +153,12 @@ public function testBlockContentCreationMultipleViewModes() {
   -    // 'info' returns an error.
   +    // 'info' doesn't return an error.
        $this->drupalGet('block/add/basic');
        $this->submitForm($edit, 'Save');
    
        // Check that the Basic block has been created.
   -    $this->assertSession()->pageTextContains('A custom block with block description ' . $edit['info[0][value]'] . ' already exists.');
   +    $this->assertSession()->pageTextContains('basic ' . $edit['info[0][value]'] . ' has been created.');
        $this->assertSession()->statusCodeEquals(200);
   

   Same here, let's just remove these

4. +++ b/core/modules/block_content/tests/src/Functional/BlockContentValidationTest.php
   @@ -2,8 +2,6 @@
    namespace Drupal\Tests\block_content\Functional;
    
   -use Drupal\Component\Render\FormattableMarkup;
   -
    /**
     * Tests block content validation constraints.
     *
   @@ -22,7 +20,7 @@ class BlockContentValidationTest extends BlockContentTestBase {
   
   @@ -22,7 +20,7 @@ class BlockContentValidationTest extends BlockContentTestBase {
      public function testValidation() {
        // Add a block.
        $description = $this->randomMachineName();
   -    $block = $this->createBlockContent($description, 'basic');
   +    $block = $this->createBlockContent($description);
        // Validate the block.
        $violations = $block->validate();
        // Make sure we have no violations.
   @@ -31,15 +29,11 @@ public function testValidation() {
   
   @@ -31,15 +29,11 @@ public function testValidation() {
        $block->save();
    
        // Add another block with the same description.
   -    $block = $this->createBlockContent($description, 'basic');
   +    $block = $this->createBlockContent($description);
        // Validate this block.
        $violations = $block->validate();
   -    // Make sure we have 1 violation.
   -    $this->assertCount(1, $violations);
   -    // Make sure the violation is on the info property
   -    $this->assertEquals('info', $violations[0]->getPropertyPath());
   -    // Make sure the message is correct.
   -    $this->assertEquals(new FormattableMarkup('A custom block with block description %value already exists.', ['%value' => $block->label()]), $violations[0]->getMessage());
   +    // Make sure we have 0 violation.
   +    $this->assertCount(0, $violations);
   

   This isn't really testing anything anymore, I think we should just remove the whole class now

5. +++ b/core/modules/block_content/tests/src/Functional/Update/BlockContentRemoveConstraint.php
   @@ -0,0 +1,37 @@
   +    $entity_type = 'block_content';
   +    $field = 'info';
   +    $definition_update_manager = \Drupal::entityDefinitionUpdateManager();
   +    $field_storage_definition = $definition_update_manager->getFieldStorageDefinition($field, $entity_type);
   +    $constraints = $field_storage_definition->getConstraints();
   

   can we also assert there are two before the update is run?

   thanks

26.1 updated
26.2 updated test case
26.3 updated test case
26.4 removed the class BlockContentValidationTest
26.5 updated to not use those variables. But not sure I follow

Also updated the dump file to drupal-9.4.0.filled.standard.php.gz
And change the install hook number to be 10100

The #15 patch does not apply for core 9.5.0-rc1@RC
I tried doing a reroll to apply it on core version 9.5.0-rc1@RC and it doesnt account for changes on comment #26

This ticket is being targeted for 10.1. If you want to upload a 9.5 patch you still can without changing the ticket. When uploading the patch there is a field "test with" you can select for the 9.5

changing the function block_content_update_9401() to function block_content_update_9501()

sorry for the spam, this is my first time :(

0 problem at all. Some tips

When you upload a new patch it's best to provide an interdiff between the two see https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa... for help. This way we can see the changes between two easily.

Also to avoid CI errors you can run ./core/scripts/dev/commit-code-check.sh --cached with your staged files to have them tested.

Also have a great community involvement on the slack channel if you wished to join there also.

+++ b/core/modules/block_content/tests/src/Functional/Update/BlockContentRemoveConstraint.php
@@ -0,0 +1,35 @@
+    $this->runUpdates();
+
+    $definition_update_manager = \Drupal::entityDefinitionUpdateManager();
+    $field_storage_definition = $definition_update_manager->getFieldStorageDefinition('info', 'block_content');
+    $constraints = $field_storage_definition->getConstraints();
+    $this->assertCount(1, $constraints);

Should we assert the before state (i.e. that the constraint exists)?

Other than that, this looks good to me

in #27 we started doing a D10 patch. If we are switching back to MRs we should open a separate one for D10. There were additional changes in #27 that should be incorporated, mainly additional testing.

Thanks for picking this up!

Per #34

+    $this->runUpdates();
+
+    $definition_update_manager = \Drupal::entityDefinitionUpdateManager();
+    $field_storage_definition = $definition_update_manager->getFieldStorageDefinition('info', 'block_content');
+    $constraints = $field_storage_definition->getConstraints();
+    $this->assertCount(1, $constraints);

Lets add a check before the runUpdates. Count should be 2 before the update.

Was also looking at it the other day and think I see why.

We are using drupal-9.4.0.filled.standard.php.gz but the update hook is 10200() wonder if it's not being called?

I'm not super clear if this change goes under update_hook_n or post_update hook. But if we moved to post_update then it should run fine on D9 and D10 I think.

Haven't tested this myself but just thinking.

Great find!

diff --git a/core/modules/block_content/block_content.install b/core/modules/block_content/block_content.install
index 772d3e35cdb..b75d59b3360 100644
--- a/core/modules/block_content/block_content.install
+++ b/core/modules/block_content/block_content.install
@@ -37,7 +37,7 @@ function block_content_update_10100(&$sandbox = NULL): TranslatableMarkup {
 }
 
 /**
- * Drop UniqueField constraint.
+ * Remove the unique values constraint from block content info fields.
  */
 function block_content_update_10200() {
   $constraint = 'UniqueField';

This message is shown in the UI at /update.php.

Fixed on commit rather than push back on a simple change.

I checked if there were any database implications here, e.g. indexes to drop etc.

Also checked the status report to make sure there were no entity field definition updates listed, and there were not 🙌

But there are none

MariaDB [local]> show keys from block_content_field_data where column_name ='info';
Empty set (0.001 sec)

MariaDB [local]> describe block_content_field_data;
+-------------------------------+------------------+------+-----+---------+-------+
| Field                         | Type             | Null | Key | Default | Extra |
+-------------------------------+------------------+------+-----+---------+-------+
| id                            | int(10) unsigned | NO   | PRI | NULL    |       |
| revision_id                   | int(10) unsigned | NO   | MUL | NULL    |       |
| type                          | varchar(32)      | NO   | MUL | NULL    |       |
| langcode                      | varchar(12)      | NO   | PRI | NULL    |       |
| status                        | tinyint(4)       | NO   | MUL | NULL    |       |
| info                          | varchar(255)     | YES  |     | NULL    |       |
| changed                       | int(11)          | YES  |     | NULL    |       |
| reusable                      | tinyint(4)       | YES  |     | NULL    |       |
| default_langcode              | tinyint(4)       | NO   |     | NULL    |       |
| revision_translation_affected | tinyint(4)       | YES  |     | NULL    |       |
| content_translation_source    | varchar(12)      | YES  |     | NULL    |       |
| content_translation_outdated  | tinyint(4)       | YES  |     | NULL    |       |
| content_translation_uid       | int(10) unsigned | YES  | MUL | NULL    |       |
| content_translation_created   | int(11)          | YES  |     | NULL    |       |
+-------------------------------+------------------+------+-----+---------+-------+
14 rows in set (0.002 sec)

MariaDB [local]> 

Published change record

The name of the function of the hook_update_N should have been:
block_content_update_10101
instead of
block_content_update_10200
because the core version is 10.1