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,infoandview_modeare part of the generictype: 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\BlockContentBlocklanded >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
- ✅ Remove the
statusandinfosettings from the\Drupal\block_content\Plugin\Block\BlockContentBlockblock plugin. - ✅ Remove the
statusandinfosettings from allBlockconfig entities using the above block plugin. - ✅ 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
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:
- 3426302-block_content-status-info
changes, plain diff MR !6953
Comments
Comment #3
wim leersEven 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
statusandinfobelong intype: block.settings.block_content:*, instead of intype: block_settings(that's where they currently reside).Comment #4
wim leersThe 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!
Comment #5
longwaveParent issue landed, so we can fix this now.
Comment #7
acbramley commentedFixed merge conflicts and created a CR, still NW as we need an upgrade path
https://www.drupal.org/node/3499836
Comment #8
acbramley commentedIt looks like a lot of the test failures are now something like this:
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.
Comment #9
acbramley commentedThe 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.
Comment #10
acbramley commentedThis is good to go now
Comment #11
smustgrave commentedIf we remove the status field won’t that preventing unpublishing of a block now
Comment #12
acbramley commentedThat's a top level setting on the block itself. These settings that we are removing do absolutely nothing.
Comment #13
smustgrave commentedLeft 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!
Comment #14
acbramley commentedComment #15
smustgrave commentedMy mistake.
rest of the change appears good to me then
Comment #16
catchLeft some MR feedback about the upgrade path.
Comment #17
acbramley commentedThis is good to go again.
Comment #18
borisson_This looks great, I have no additional remarks on this.
Comment #19
acbramley commentedSquashed and rebased this branch since it wasn't rebasing cleanly.
Comment #21
larowlanCommitted 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