Make Block config entities fully validatable

Created on 7 August 2023, 11 months ago
Updated 18 June 2024, 10 days ago

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: ๐Ÿ“Œ Add config validation for weights (blocks, filters, etc. all use weights) Fixed
    2. โœ… Reduction by 1 file/11 LoC in this MR: ๐Ÿ› ExistsConstraintValidator should ignore NULL values and treat `core` as a valid module Needs review
  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. ๐Ÿ“Œ [PP-1] Deprecate unused `provider` exported property from Block config entities Postponed
  2. ๐Ÿ“Œ [PP-1] Deprecate `null` as valid `weight` for Block config entities Postponed
  3. ๐Ÿ“Œ [PP-1] Deprecate and remove `status` and `info` settings from `block_content` blocks Postponed

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

TBD

๐Ÿ“Œ Task
Status

Needs work

Version

11.0 ๐Ÿ”ฅ

Component
Blockย  โ†’

Last updated about 11 hours ago

Created by

๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @Wim Leers
  • Assigned to Wim Leers
  • Status changed to Active 4 months ago
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

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

  • Merge request !6938Resolve #3379725 "Validatable block" โ†’ (Open) created by Wim Leers
  • Status changed to Needs work 4 months ago
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

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

  • Pipeline finished with Failed
    4 months ago
    Total: 533s
    #112765
  • Pipeline finished with Failed
    4 months ago
    Total: 551s
    #112849
  • Pipeline finished with Failed
    4 months ago
    Total: 487s
    #112863
  • Pipeline finished with Failed
    4 months ago
    Total: 489s
    #112873
  • Pipeline finished with Failed
    4 months ago
    Total: 546s
    #112879
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    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! ๐Ÿš€

  • ๐Ÿ‡ซ๐Ÿ‡ทFrance andypost

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

  • Pipeline finished with Failed
    4 months ago
    Total: 616s
    #112902
  • Pipeline finished with Failed
    4 months ago
    Total: 491s
    #112930
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    #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 ๐Ÿ˜„

  • Pipeline finished with Failed
    4 months ago
    Total: 603s
    #112942
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

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

  • Pipeline finished with Canceled
    4 months ago
    Total: 190s
    #112949
  • Pipeline finished with Failed
    4 months ago
    Total: 530s
    #112953
  • Pipeline finished with Failed
    4 months ago
    #112972
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    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
  • Pipeline finished with Failed
    4 months ago
    Total: 504s
    #113007
  • Pipeline finished with Failed
    4 months ago
    Total: 492s
    #113042
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Rewrote issue summary ๐Ÿ‘

  • Pipeline finished with Failed
    4 months ago
    Total: 482s
    #113066
  • Pipeline finished with Failed
    4 months ago
    Total: 202s
    #113103
  • Pipeline finished with Failed
    4 months ago
    Total: 604s
    #113112
  • Pipeline finished with Success
    4 months ago
    Total: 572s
    #113167
  • Issue was unassigned.
  • Status changed to Needs review 4 months ago
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    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!

  • Pipeline finished with Failed
    4 months ago
    Total: 559s
    #113542
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    2 of the 3 follow-ups created.

    Update path test pushed.

  • Pipeline finished with Failed
    4 months ago
    Total: 491s
    #113565
  • Pipeline finished with Success
    4 months ago
    Total: 567s
    #113610
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    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 ๐Ÿ˜„

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  • Status changed to Needs work 4 months ago
  • 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.

  • Status changed to Needs review 4 months ago
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  • Pipeline finished with Failed
    4 months ago
    Total: 579s
    #114423
  • Assigned to Wim Leers
  • Status changed to Needs work 4 months ago
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium borisson_ Mechelen, ๐Ÿ‡ง๐Ÿ‡ช

    I found one tiny nitpick.

  • Pipeline finished with Success
    4 months ago
    Total: 491s
    #115005
  • Issue was unassigned.
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    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.

  • Pipeline finished with Success
    4 months ago
    Total: 499s
    #115023
  • First commit to issue fork.
  • Pipeline finished with Success
    3 months ago
    Total: 601s
    #140766
  • Pipeline finished with Canceled
    3 months ago
    Total: 302s
    #143046
  • Pipeline finished with Failed
    3 months ago
    Total: 641s
    #143056
  • Pipeline finished with Failed
    about 2 months ago
    Total: 613s
    #160686
  • Pipeline finished with Failed
    about 2 months ago
    Total: 4446s
    #162198
  • Pipeline finished with Failed
    about 2 months ago
    Total: 491s
    #162441
  • Pipeline finished with Success
    about 2 months ago
    Total: 504s
    #162505
  • Pipeline finished with Success
    about 2 months ago
    Total: 572s
    #162523
  • Status changed to Needs review about 2 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

    Marked NR for initial review.

  • Pipeline finished with Success
    about 2 months ago
    Total: 518s
    #163087
  • Status changed to Needs work 30 days ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts

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

  • Pipeline finished with Success
    23 days ago
    Total: 617s
    #191718
  • Pipeline finished with Canceled
    22 days ago
    Total: 67s
    #192777
  • Pipeline finished with Failed
    22 days ago
    Total: 515s
    #192780
  • Pipeline finished with Success
    10 days ago
    Total: 551s
    #201921
  • Status changed to Needs review 10 days ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India
  • Status changed to Needs work 10 days ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts

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

  • Pipeline finished with Failed
    9 days ago
    Total: 596s
    #202677
  • Pipeline finished with Success
    9 days ago
    Total: 566s
    #202764
  • Pipeline finished with Success
    2 days ago
    Total: 510s
    #208440
Production build 0.69.0 2024