Issue #3379725 by Wim Leers, phenaproxima, narendraR, alexpott, quietone, borisson_, tim.plunkett: Make Block config entities fully validatable

Author: alexpott

config/schema/core.data_types.schema.yml

@@ -427,12 +427,16 @@ config_entity:
       requiredKey: false
       type: _core_config_info
 
+# This applies to all blocks that have no additional settings of their own.
 block.settings.*:
   type: block_settings
+  constraints:
+    FullyValidatable: ~
 
 block_settings:
   type: mapping
   label: 'Block settings'
+  # This is intentionally not marked as fully validatable: each `type: block.settings.SOMETHING` must do that.
   mapping:
     id:
       type: string
@@ -441,21 +445,17 @@ block_settings:
       type: label
       label: 'Description'
     label_display:
-      type: string
+      type: label
       label: 'Display title'
     provider:
       type: string
-      label: 'Provider'
-    status:
-      type: boolean
-      label: 'Status'
-    info:
-      type: label
-      label: 'Admin info'
-    view_mode:
-      type: string
-      label: 'View mode'
+      label: 'Provider of this block plugin'
+      constraints:
+        NotBlank: []
+        ExtensionName: []
+        ExtensionExists: module
     context_mapping:
+      requiredKey: false
       type: sequence
       label: 'Context assignments'
       sequence:

modules/block/block.post_update.php

@@ -5,6 +5,9 @@
  * Post update functions for Block.
  */
 
+use Drupal\block\BlockInterface;
+use Drupal\Core\Config\Entity\ConfigEntityUpdater;
+
 /**
  * Implements hook_removed_post_updates().
  */
@@ -16,3 +19,18 @@ function block_removed_post_updates() {
     'block_post_update_replace_node_type_condition' => '10.0.0',
   ];
 }
+
+/**
+ * Ensures that all block weights are integers.
+ */
+function block_post_update_make_weight_integer(array &$sandbox = []): void {
+  \Drupal::classResolver(ConfigEntityUpdater::class)
+    ->update($sandbox, 'block', function (BlockInterface $block): bool {
+      $weight = $block->getWeight();
+      if (!is_int($weight)) {
+        $block->setWeight($weight);
+        return TRUE;
+      }
+      return FALSE;
+    });
+}

modules/block/config/schema/block.schema.yml

@@ -3,6 +3,8 @@
 block.block.*:
   type: config_entity
   label: 'Block'
+  constraints:
+    FullyValidatable: ~
   mapping:
     id:
       type: machine_name
@@ -17,13 +19,22 @@ block.block.*:
     theme:
       type: string
       label: 'Theme'
+      constraints:
+        NotBlank: []
+        ExtensionName: []
+        ExtensionExists: theme
     region:
       type: string
       label: 'Region'
+      constraints:
+        NotBlank: []
+        Callback: ['\Drupal\block\Entity\Block', validateRegion]
     weight:
       type: weight
       label: 'Weight'
     provider:
+      # @todo Deprecate this from config schema and remove it from the `config_export` definition in https://www.drupal.org/project/drupal/issues/3426278
+      nullable: true
       type: string
       label: 'Provider'
     plugin:

modules/block/migrations/d6_block.yml

@@ -96,7 +96,14 @@ process:
           right: sidebar
     # If mapping fails, put the block in the content region.
     default_value: content
-  weight: weight
+  weight:
+    -
+      plugin: get
+      source: weight
+    -
+      # Block weights must be integers.
+      plugin: callback
+      callable: intval
   settings:
     plugin: block_settings
     source:

modules/block/migrations/d7_block.yml

@@ -117,7 +117,14 @@ process:
           sidebar_first: 'sidebar_first'
     # If mapping fails, put the block in the content region.
     default_value: content
-  weight: weight
+  weight:
+    -
+      plugin: get
+      source: weight
+    -
+      # Block weights must be integers.
+      plugin: callback
+      callable: intval
   settings:
     plugin: block_settings
     source:

modules/block/src/BlockForm.php

@@ -174,7 +174,7 @@ public function form(array $form, FormStateInterface $form_state) {
     }
 
     // Hidden weight setting.
-    $weight = $entity->isNew() ? $this->getRequest()->query->get('weight', 0) : $entity->getWeight();
+    $weight = $entity->isNew() ? $this->getRequest()->query->getInt('weight') : $entity->getWeight();
     $form['weight'] = [
       '#type' => 'hidden',
       '#default_value' => $weight,

modules/block/src/Entity/Block.php

@@ -11,6 +11,7 @@
 use Drupal\Core\Config\Entity\ConfigEntityInterface;
 use Drupal\Core\Entity\EntityWithPluginCollectionInterface;
 use Drupal\Core\Entity\EntityStorageInterface;
+use Symfony\Component\Validator\Context\ExecutionContextInterface;
 use Drupal\Core\StringTranslation\TranslatableMarkup;
 
 /**
@@ -89,7 +90,7 @@ class Block extends ConfigEntityBase implements BlockInterface, EntityWithPlugin
    *
    * @var int
    */
-  protected $weight;
+  protected $weight = 0;
 
   /**
    * The plugin instance ID.
@@ -323,7 +324,7 @@ public function setRegion($region) {
    */
   #[ActionMethod(adminLabel: new TranslatableMarkup('Set weight'), pluralize: FALSE)]
   public function setWeight($weight) {
-    $this->weight = $weight;
+    $this->weight = (int) $weight;
     return $this;
   }
 
@@ -347,6 +348,11 @@ public function createDuplicateBlock($new_id = NULL, $new_theme = NULL) {
   public function preSave(EntityStorageInterface $storage) {
     parent::preSave($storage);
 
+    if (!is_int($this->weight)) {
+      @trigger_error('Saving a block with a non-integer weight is deprecated in drupal:11.1.0 and removed in drupal:12.0.0. See https://www.drupal.org/node/3462474', E_USER_DEPRECATED);
+      $this->setWeight((int) $this->weight);
+    }
+
     // Ensure the region is valid to mirror the behavior of block_rebuild().
     // This is done primarily for backwards compatibility support of
     // \Drupal\block\BlockInterface::BLOCK_REGION_NONE.
@@ -358,4 +364,23 @@ public function preSave(EntityStorageInterface $storage) {
     }
   }
 
+  /**
+   * Validates that a region exists in the active theme.
+   *
+   * @param null|string $region
+   *   The region to validate.
+   * @param \Symfony\Component\Validator\Context\ExecutionContextInterface $context
+   *   The validation context.
+   */
+  public static function validateRegion(?string $region, ExecutionContextInterface $context): void {
+    if ($theme = $context->getRoot()->get('theme')->getValue()) {
+      if (!array_key_exists($region, system_region_list($theme))) {
+        $context->addViolation('This is not a valid region of the %theme theme.', ['%theme' => $theme]);
+      }
+    }
+    else {
+      $context->addViolation('This block does not say which theme it appears in.');
+    }
+  }
+
 }

modules/block/tests/src/Functional/BlockInvalidRegionTest.php

@@ -21,6 +21,16 @@ class BlockInvalidRegionTest extends BrowserTestBase {
    */
   protected static $modules = ['block', 'block_test'];
 
+  /**
+   * {@inheritdoc}
+   */
+  protected static $configSchemaCheckerExclusions = [
+    // This block is intentionally put in an invalid region, so it will violate
+    // config schema.
+    // @see ::testBlockInvalidRegion()
+    'block.block.invalid_region',
+  ];
+
   /**
    * {@inheritdoc}
    */
@@ -45,7 +55,7 @@ protected function setUp(): void {
    */
   public function testBlockInInvalidRegion(): void {
     // Enable a test block and place it in an invalid region.
-    $block = $this->drupalPlaceBlock('test_html');
+    $block = $this->drupalPlaceBlock('test_html', ['id' => 'invalid_region']);
     \Drupal::configFactory()->getEditable('block.block.' . $block->id())->set('region', 'invalid_region')->save();
     $block = Block::load($block->id());
 

modules/block/tests/src/Functional/BlockRenderOrderTest.php

@@ -46,17 +46,17 @@ public function testBlockRenderOrder(): void {
     $region = 'header';
     $test_blocks = [
       'stark_powered' => [
-        'weight' => '-3',
+        'weight' => -3,
         'id' => 'stark_powered',
         'label' => 'Test block A',
       ],
       'stark_by' => [
-        'weight' => '3',
+        'weight' => 3,
         'id' => 'stark_by',
         'label' => 'Test block C',
       ],
       'stark_drupal' => [
-        'weight' => '3',
+        'weight' => 3,
         'id' => 'stark_drupal',
         'label' => 'Test block B',
       ],

modules/block/tests/src/Functional/BlockWeightUpdateTest.php

@@ -0,0 +1,51 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Drupal\Tests\block\Functional;
+
+use Drupal\block\Entity\Block;
+use Drupal\FunctionalTests\Update\UpdatePathTestBase;
+
+/**
+ * @covers block_post_update_make_weight_integer
+ * @group block
+ */
+class BlockWeightUpdateTest extends UpdatePathTestBase {
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function setDatabaseDumpFiles() {
+    $this->databaseDumpFiles = [
+      __DIR__ . '/../../../../system/tests/fixtures/update/drupal-10.3.0.filled.standard.php.gz',
+    ];
+  }
+
+  /**
+   * Tests update path for blocks' `weight` property.
+   */
+  public function testRunUpdates() {
+    // Find a block and change it to have a null weight.
+    /** @var \Drupal\Core\Database\Connection $database */
+    $database = $this->container->get('database');
+    $block = $database->select('config', 'c')
+      ->fields('c', ['data'])
+      ->condition('name', 'block.block.claro_content')
+      ->execute()
+      ->fetchField();
+    $block = unserialize($block);
+    $block['weight'] = NULL;
+    $database->update('config')
+      ->fields([
+        'data' => serialize($block),
+      ])
+      ->condition('name', 'block.block.claro_content')
+      ->execute();
+
+    $this->assertNull(Block::load('claro_content')->get('weight'));
+    $this->runUpdates();
+    $this->assertSame(0, Block::load('claro_content')->get('weight'));
+  }
+
+}

modules/block/tests/src/Functional/Rest/BlockResourceTestBase.php

@@ -82,7 +82,7 @@ protected function getExpectedNormalizedEntity() {
     $normalization = [
       'uuid' => $this->entity->uuid(),
       'id' => 'llama',
-      'weight' => NULL,
+      'weight' => 0,
       'langcode' => 'en',
       'status' => TRUE,
       'dependencies' => [

modules/block/tests/src/Kernel/BlockRebuildTest.php

@@ -23,6 +23,17 @@ class BlockRebuildTest extends KernelTestBase {
    */
   protected static $modules = ['block', 'system'];
 
+  /**
+   * {@inheritdoc}
+   */
+  protected static $configSchemaCheckerExclusions = [
+    // These blocks are intentionally put into invalid regions, so they will
+    // violate config schema.
+    // @see ::testRebuildInvalidBlocks()
+    'block.block.invalid_block1',
+    'block.block.invalid_block2',
+  ];
+
   /**
    * {@inheritdoc}
    */
@@ -71,8 +82,12 @@ public function testRebuildNoInvalidBlocks(): void {
    */
   public function testRebuildInvalidBlocks(): void {
     $this->placeBlock('system_powered_by_block', ['region' => 'content']);
-    $block1 = $this->placeBlock('system_powered_by_block');
-    $block2 = $this->placeBlock('system_powered_by_block');
+    $block1 = $this->placeBlock('system_powered_by_block', [
+      'id' => 'invalid_block1',
+    ]);
+    $block2 = $this->placeBlock('system_powered_by_block', [
+      'id' => 'invalid_block2',
+    ]);
     $block2->disable()->save();
     // Use the config API directly to bypass Block::preSave().
     \Drupal::configFactory()->getEditable('block.block.' . $block1->id())->set('region', 'INVALID')->save();

modules/block/tests/src/Kernel/BlockStorageUnitTest.php

@@ -93,7 +93,7 @@ protected function createTests() {
       'id' => 'test_block',
       'theme' => 'stark',
       'region' => 'content',
-      'weight' => NULL,
+      'weight' => 0,
       'provider' => NULL,
       'plugin' => 'test_html',
       'settings' => [