Problem/Motivation

Blocks are some of the most widely used config entities. They should be validatable. This would be very valuable for the Recipes Initiative, especially now that Recipes uses config validation (see #3405328: [meta] Make recipes safer to use in the real world by supporting config validation and rolling back a broken recipe for details).

Proposed resolution

This can easily become a huge scope, so limit the scope to be reviewable:

  1. Make block.block.* validatable.
  2. Introduce type: block.settings.block_content:* to fix the accidental leftovers from when content blocks ("custom blocks" in Drupal 7) were split out — see #5
  3. Make one concrete block plugin with settings fully validatable: the search block (type: block.settings.search_form_block)
  4. .

  5. Open blockers for essential problems that are out of scope:
    1. ✅ Reduction by 11 LoC in this MR: #3426309: Add config validation for weights (blocks, filters, etc. all use weights)
    2. ✅ Reduction by 1 file/11 LoC in this MR: #3426324: ExistsConstraintValidator should ignore NULL values and treat `core` as a valid module
  6. Open follow-ups for non-essential problems encountered along the way — see under "remaining tasks"

Remaining tasks

  1. ✅ Implement proposed solution
  2. ✅ Get tests passing
  3. ✅ Search module must provide update path for search_form_block blocks' page_id setting
  4. Explicit test coverage for each top-level property of a Block config entity in BlockValidationTest.

Follow-ups:

  1. #3426278: Deprecate unused `provider` exported property from Block config entities
  2. #3426279: Deprecate `null` as valid `weight` for Block config entities
  3. #3426302: Deprecate and remove `status` and `info` settings from `block_content` blocks

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

TBD

Issue fork drupal-3379725

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

Title: Fix Block config entity type config schema violations: weight, property » Make Block config entities fully validatable
Assigned: Unassigned » wim leers
Status: Postponed » Active
Issue tags: +Needs issue summary update
Parent issue: #3364109: Configuration schema & required values: add test coverage for `nullable: true` validation support » #2869792: [meta] Add constraints to all config entity types
Related issues: +#3364109: Configuration schema & required values: add test coverage for `nullable: true` validation support

Repurposing this to add validation constraints; the original issue summary no longer makes sense because the scope/approach of #3364109: Configuration schema & required values: add test coverage for `nullable: true` validation support evolved.

wim leers’s picture

Status: Active » Needs work
Issue tags: +Needs update path, +Needs update path tests

Well this is interesting… AFAICT block.block.*:provider is dead/completely unused, but block.block.*:settings.provider is used.

wim leers’s picture

Ah, I finally found the explanation for why status, info and view_mode are part of the generic type: block_settings (and hence apply to ALL block plugins!).

The answer: it's simply an oversight that occurred in #2274175: Block plugin schema should be defined separately from the entity. 😄👍 (\Drupal\block_content\Plugin\Block\BlockContentBlock landed >1 year earlier, but the tight coupling only was removed ~6 months earlier, in #1927608: Remove the tight coupling between Block Plugins and Block Entities. Arguably at least there this should've been rectified, but that was at the height of the "we must get D8 done yesterday!" era, so it totally makes sense.)

So this issue is definitely cleaning up some very old technical debt, that's caused confusion for every person who has ever looked at the exported YAML for blocks! 🚀

The crucial commit: https://git.drupalcode.org/project/drupal/-/merge_requests/6938/diffs?co...

andypost’s picture

Great find! Would be great to have CR for that change somehow as I bet some contrib already using the same pattern

wim leers’s picture

Issue tags: +Needs change record

#6: this will absolutely need a change record — probably multiple!

Wow, with #5 fixed, plus one small fix to the ExtensionExists constraint, we went from 788 failures to 115 failures! Suddenly this became doable 😄

wim leers’s picture

Yay, and after refining the constraints for the sole block (the search block) whose settings I've added validation constraints to: 115 → 56 failures.

wim leers’s picture

Reduced scope by:

  1. Undeprecating the (unused and useless) provider exported property in Block config entities around dropped us to 52 failures
  2. Undeprecating the status and info settings for "content block" blocks dropped us to 51 failures
wim leers’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Rewrote issue summary 👍

wim leers’s picture

Assigned: wim leers » Unassigned
Status: Needs work » Needs review
Issue tags: +Recipes initiative

GREEN!

Update path tomorrow.

Very satisfied that I got this to green with just 18 files, +138, -49 in less than a day 🥳🚀 With this in, we could seriously accelerate config validation adoption, and Recipes indirectly!

wim leers’s picture

wim leers’s picture

Update path pushed.

3rd follow-up created (with MR!).

Two blockers created:

… both with a ready-to-review MR that's green. 😊

Ready for final review! But please help land the two (trivial!) blockers first 😄

wim leers’s picture

Title: [PP-2] Make Block config entities fully validatable » [PP-1] Make Block config entities fully validatable
Issue summary: View changes
needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

wim leers’s picture

Status: Needs work » Needs review
wim leers’s picture

Title: [PP-1] Make Block config entities fully validatable » Make Block config entities fully validatable
Assigned: Unassigned » wim leers
Issue summary: View changes
Status: Needs review » Needs work
borisson_’s picture

I found one tiny nitpick.

wim leers’s picture

Assigned: wim leers » Unassigned
Issue summary: View changes
Issue tags: +Needs tests

I started added explicit validation logic for all of the properties on Block config entities. I just pushed the test coverage for theme.

But then I noticed that block.block.*:region doesn't have a validation constraint just yet! So this definitely needs more work before it is ready for final review. Will continue this next week.

narendraR made their first commit to this issue’s fork.

narendrar’s picture

Status: Needs work » Needs review

Marked NR for initial review.

phenaproxima’s picture

Status: Needs review » Needs work

This is looking good but still needs some changes...

narendrar’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record, -Needs tests
phenaproxima’s picture

Status: Needs review » Needs work

A few questions but overall I think this looks pretty good.

narendrar’s picture

Status: Needs work » Needs review
phenaproxima’s picture

Status: Needs review » Needs work

Two very minor things (basically just rewordings), otherwise I think this is OK.

narendrar’s picture

Status: Needs work » Needs review

Feedback addressed

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

No objections here.

quietone’s picture

Status: Reviewed & tested by the community » Needs work

I read the issue summary, the comments and the MR (not a code review). The find in #5 is great!

I resolved the low hanging comments, leaving 7 to look at. I did find one unanswered question in the MR, I have the same question, so let's get that answered. Setting to NW.

I think it would help if the issue summary explained why BlockInvalidRegionTest, testRebuildInvalidBlocks() are deleted

I also question the need to shout at the reader in comments. I am referring to the use of 'TRICKY', a word that also have negative connotations. If we need to draw people's attention to something there is always the use of "Note that ...". There may be other options. This is not meant to block progress on this issue. This conversation may be more suitable for a separate issue.

quietone’s picture

Forgot to add, that all the followup are made, with complete issue summaries and the links in the MR are correct.

phenaproxima’s picture

Status: Needs work » Reviewed & tested by the community

I removed the unnecessary test coverage, but as far as I can tell, this is otherwise good to go. I'm going to go ahead and boldly restore RTBC here, and if there's anything further to fix, let's get it over the line. This issue is even more imperative since #3448131: Create flexible config actions to place a block in the admin or default themes is close to ready.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I don't think that config validation issues shoudl just in a type widening for used values. Weight is always an integer once a block has been saved because config casts the value.

phenaproxima’s picture

Status: Needs work » Needs review

Alright, all done here AFAIK. I did the deprecation dance around non-integer block weights.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new10.18 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

phenaproxima’s picture

Status: Needs work » Needs review
alexpott’s picture

Status: Needs review » Needs work

Added some more review comments. Nice work on fixing up the int-ness of weight @phenaproxima

phenaproxima’s picture

Status: Needs work » Needs review

I think I've addressed all of @alexpott's feedback.

tim.plunkett’s picture

Status: Needs review » Needs work

Reviewed

phenaproxima’s picture

Status: Needs work » Needs review

Okay, I agree with you @tim.plunkett; did the deprecation dance around page_id and added a change record.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Great work, all!

borisson_’s picture

I reviewed it again today as well, and the one remark I had was answered. RTBC++

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 1c21864 and pushed to 11.x. Thanks!

Given where we are at in the 11.0.0 release cycle we can't commit this to 11.0.0 and have to target 11.1.0 instead. I've updated the deprecation messages and tests on commit.

diff --git a/core/modules/block/src/Entity/Block.php b/core/modules/block/src/Entity/Block.php
index 2effc409b7..68582e48f0 100644
--- a/core/modules/block/src/Entity/Block.php
+++ b/core/modules/block/src/Entity/Block.php
@@ -349,7 +349,7 @@ 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.0.0 and removed in drupal:12.0.0. See https://www.drupal.org/node/3462474', E_USER_DEPRECATED);
+      @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);
     }
 
diff --git a/core/modules/block/tests/src/Kernel/BlockValidationTest.php b/core/modules/block/tests/src/Kernel/BlockValidationTest.php
index 9a1a2412d6..2456e7fb57 100644
--- a/core/modules/block/tests/src/Kernel/BlockValidationTest.php
+++ b/core/modules/block/tests/src/Kernel/BlockValidationTest.php
@@ -176,7 +176,7 @@ public function testWeightValidation(): void {
   public function testWeightCannotBeNull(): void {
     $this->entity->set('weight', NULL);
     $this->assertNull($this->entity->getWeight());
-    $this->expectDeprecation('Saving a block with a non-integer weight is deprecated in drupal:11.0.0 and removed in drupal:12.0.0. See https://www.drupal.org/node/3462474');
+    $this->expectDeprecation('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');
     $this->entity->save();
   }
 
diff --git a/core/modules/search/search.module b/core/modules/search/search.module
index 8d5b1e330d..0944c53dcf 100644
--- a/core/modules/search/search.module
+++ b/core/modules/search/search.module
@@ -428,7 +428,7 @@ function search_block_presave(BlockInterface $block) {
   if ($block->getPluginId() === 'search_form_block') {
     $settings = $block->get('settings');
     if ($settings['page_id'] === '') {
-      @trigger_error('Saving a search block with an empty page ID is deprecated in drupal:11.0.0 and removed in drupal:12.0.0. To use the default search page, use NULL. See https://www.drupal.org/node/3463132', E_USER_DEPRECATED);
+      @trigger_error('Saving a search block with an empty page ID is deprecated in drupal:11.1.0 and removed in drupal:12.0.0. To use the default search page, use NULL. See https://www.drupal.org/node/3463132', E_USER_DEPRECATED);
       $settings['page_id'] = NULL;
       $block->set('settings', $settings);
     }

  • alexpott committed 1c218641 on 11.x
    Issue #3379725 by Wim Leers, phenaproxima, narendraR, alexpott, quietone...

Status: Fixed » Closed (fixed)

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

wim leers’s picture

larowlan’s picture