Problem/Motivation

It was discovered in #3379725-5: Make Block config entities fully validatable that the config schema for Block config entities (type: block.block.*), and specifically the "base settings" for block plugins (type: block_settings) contains three key-value pairs (status, info and view) that actually are block_content block-specific. And … two of them are completely unused: status and info.

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 has the potential to clean up some very old technical debt, that's caused confusion for every person who has ever looked at the exported YAML for blocks! 🚀

Therefore null never makes sense, and it's only thanks to PHP's automatic typecasting that this has never been a problem 😅

Steps to reproduce

N/A

Proposed resolution

  1. ✅ Remove the status and info settings from the \Drupal\block_content\Plugin\Block\BlockContentBlock block plugin.
  2. ✅ Remove the status and info settings from all Block config entities using the above block plugin.
  3. ✅ Provide an update path that removes those two settings from existing blocks.

Remaining tasks

See above.

User interface changes

None.

API changes

None.

Data model changes

No more useless status and info settings on "content block" blocks.

Release notes snippet

None.

Issue fork drupal-3426302

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: Deprecate and remove `status` and `info` settings from `block_content` blocks » [PP-1] Deprecate and remove `status` and `info` settings from `block_content` blocks
Issue summary: View changes
Status: Active » Postponed
Issue tags: +Needs update path, +Needs update path tests

Even though we could easily land this before #3379725: Make Block config entities fully validatable, I think it's better not to.

Because #3379725 will bring much needed-clarity: that status and info belong in type: block.settings.block_content:*, instead of in type: block_settings (that's where they currently reside).

wim leers’s picture

Because #3379725 will bring much needed-clarity: that status and info belong in type: block.settings.block_content:*, instead of in type: block_settings (that's where they currently reside).

The changes in #3379725 will also cause most (I actually think all) test failures on the current MR to disappear. That's another important reason to postpone this issue: if we wait for that to land, then this issue's MR can stay as simple as it is today!

longwave’s picture

Title: [PP-1] Deprecate and remove `status` and `info` settings from `block_content` blocks » Deprecate and remove `status` and `info` settings from `block_content` blocks
Status: Postponed » Needs work

Parent issue landed, so we can fix this now.

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

acbramley’s picture

Fixed merge conflicts and created a CR, still NW as we need an upgrade path

https://www.drupal.org/node/3499836

acbramley’s picture

It looks like a lot of the test failures are now something like this:

    1) /builds/issue/drupal-3426302/core/lib/Drupal/Core/Config/Schema/SchemaCheckTrait.php:211
    The 'status' setting for content blocks is deprecated in drupal:11.2.0 and is removed from drupal 12.0.0. It was unused, so there is no replacement. See https://www.drupal.org/node/3499836.

Which is coming from an update path test, I'm assuming because the db fixture has those keys set. Not entirely sure how we usually solve this kind of thing.

acbramley’s picture

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

The fix for #8 was glaring me in the face, we needed an update path to fix those deprecations.

I'll work on an update path test next.

acbramley’s picture

Status: Needs work » Needs review
Issue tags: -Needs update path tests

This is good to go now

smustgrave’s picture

If we remove the status field won’t that preventing unpublishing of a block now

acbramley’s picture

That's a top level setting on the block itself. These settings that we are removing do absolutely nothing.

smustgrave’s picture

Status: Needs review » Needs work

Left 1 comment on MR

If you are another contributor eager to jump in, please allow the previous poster at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

acbramley’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

My mistake.

rest of the change appears good to me then

catch’s picture

Status: Reviewed & tested by the community » Needs work

Left some MR feedback about the upgrade path.

acbramley’s picture

Status: Needs work » Needs review

This is good to go again.

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

This looks great, I have no additional remarks on this.

acbramley’s picture

Squashed and rebased this branch since it wasn't rebasing cleanly.

  • larowlan committed 74ba8de4 on 11.x
    Issue #3426302 by acbramley, wim leers: Deprecate and remove `status`...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 11.x

Glad to see this go. I think I was likely the culprit that caused this in the first place 🙈

Published change record

Status: Fixed » Closed (fixed)

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