Broken context-aware block plugins throw an unexpected exception

Reformatted issue to suit the issue template & corrected one typo.
Also tagged with "DrupalSouth 2019" as per the advice on the #australia-nz slack channel.

Hmmm... I replied to this issue last night and the updated time, 24 Feb 2020 at 01:39 CET, seems to confirm this, but today my reply is nowhere to be seen....

Anyway, let me try again:

I encountered this issue yesterday, but the field I deleted is not used anywhere in my layout. Trying to view the node with the layout or edit the layout to fix the issue throws the exception mentioned in the original post making it impossible to fix without commenting out the line throwing the exception as proposed in the original post.

After debugging this it seems that one of my layout sections contains references to blocks or fields that I've never used even though the section is completely empty. My entire layout is completely empty apart from containing 2 visually identically layout sections.

This is the serialized data for the empty working layout section from my database:
O:29:"Drupal\layout_builder\Section":4:{s:11:"*layoutId";s:18:"layout_onecol_wide";s:17:"*layoutSettings";a:1:{s:5:"label";s:6:"Banner";}s:13:"*components";a:0:{}s:21:"*thirdPartySettings";a:0:{}}

And this is from the broken layout section causing the exception to be thrown:
O:29:"Drupal\layout_builder\Section":4:{s:11:"*layoutId";s:13:"layout_onecol";s:17:"*layoutSettings";a:1:{s:5:"label";s:0:"";}s:13:"*components";a:7:{s:36:"dd8cd7a7-afca-4936-8a3f-208621a0336f";O:38:"Drupal\layout_builder\SectionComponent":5:{s:7:"*uuid";s:36:"dd8cd7a7-afca-4936-8a3f-208621a0336f";s:9:"*region";s:7:"content";s:16:"*configuration";a:3:{s:13:"label_display";s:1:"0";s:15:"context_mapping";a:1:{s:6:"entity";s:21:"layout_builder.entity";}s:2:"id";s:39:"extra_field_block:node:case_study:links";}s:9:"*weight";i:0;s:13:"*additional";a:0:{}}s:36:"3f52075a-1d1d-4164-981f-f64027cd7540";O:38:"Drupal\layout_builder\SectionComponent":5:{s:7:"*uuid";s:36:"3f52075a-1d1d-4164-981f-f64027cd7540";s:9:"*region";s:7:"content";s:16:"*configuration";a:4:{s:13:"label_display";s:1:"0";s:15:"context_mapping";a:1:{s:6:"entity";s:21:"layout_builder.entity";}s:2:"id";s:46:"field_block:node:case_study:field_teaser_image";s:9:"formatter";a:4:{s:5:"label";s:5:"above";s:8:"settings";a:2:{s:11:"image_style";s:0:"";s:10:"image_link";s:0:"";}s:20:"third_party_settings";a:0:{}s:4:"type";s:5:"image";}}s:9:"*weight";i:1;s:13:"*additional";a:0:{}}s:36:"ce395b44-37b8-4d05-8088-ba9fd28ac6a6";O:38:"Drupal\layout_builder\SectionComponent":5:{s:7:"*uuid";s:36:"ce395b44-37b8-4d05-8088-ba9fd28ac6a6";s:9:"*region";s:7:"content";s:16:"*configuration";a:4:{s:13:"label_display";s:1:"0";s:15:"context_mapping";a:1:{s:6:"entity";s:21:"layout_builder.entity";}s:2:"id";s:40:"field_block:node:case_study:field_slogan";s:9:"formatter";a:4:{s:5:"label";s:5:"above";s:8:"settings";a:1:{s:14:"link_to_entity";b:0;}s:20:"third_party_settings";a:0:{}s:4:"type";s:6:"string";}}s:9:"*weight";i:2;s:13:"*additional";a:0:{}}s:36:"88d609f8-2c3e-4ba4-bc73-c1d13d8ec82f";O:38:"Drupal\layout_builder\SectionComponent":5:{s:7:"*uuid";s:36:"88d609f8-2c3e-4ba4-bc73-c1d13d8ec82f";s:9:"*region";s:7:"content";s:16:"*configuration";a:4:{s:13:"label_display";s:1:"0";s:15:"context_mapping";a:1:{s:6:"entity";s:21:"layout_builder.entity";}s:2:"id";s:48:"field_block:node:case_study:field_project_leader";s:9:"formatter";a:4:{s:5:"label";s:5:"above";s:8:"settings";a:1:{s:4:"link";b:1;}s:20:"third_party_settings";a:0:{}s:4:"type";s:22:"entity_reference_label";}}s:9:"*weight";i:3;s:13:"*additional";a:0:{}}s:36:"1e756e5f-9259-49f7-ac15-dc058e30374e";O:38:"Drupal\layout_builder\SectionComponent":5:{s:7:"*uuid";s:36:"1e756e5f-9259-49f7-ac15-dc058e30374e";s:9:"*region";s:7:"content";s:16:"*configuration";a:4:{s:13:"label_display";s:1:"0";s:15:"context_mapping";a:1:{s:6:"entity";s:21:"layout_builder.entity";}s:2:"id";s:40:"field_block:node:case_study:field_client";s:9:"formatter";a:4:{s:5:"label";s:5:"above";s:8:"settings";a:1:{s:4:"link";b:1;}s:20:"third_party_settings";a:0:{}s:4:"type";s:22:"entity_reference_label";}}s:9:"*weight";i:4;s:13:"*additional";a:0:{}}s:36:"5ebda3b3-5e0a-4d49-b10b-d2f673c747ed";O:38:"Drupal\layout_builder\SectionComponent":5:{s:7:"*uuid";s:36:"5ebda3b3-5e0a-4d49-b10b-d2f673c747ed";s:9:"*region";s:7:"content";s:16:"*configuration";a:4:{s:13:"label_display";s:1:"0";s:15:"context_mapping";a:1:{s:6:"entity";s:21:"layout_builder.entity";}s:2:"id";s:38:"field_block:node:case_study:field_logo";s:9:"formatter";a:4:{s:5:"label";s:5:"above";s:8:"settings";a:1:{s:4:"link";b:1;}s:20:"third_party_settings";a:0:{}s:4:"type";s:22:"entity_reference_label";}}s:9:"*weight";i:5;s:13:"*additional";a:0:{}}s:36:"e6eed493-02c9-4beb-872f-4662f48b3150";O:38:"Drupal\layout_builder\SectionComponent":5:{s:7:"*uuid";s:36:"e6eed493-02c9-4beb-872f-4662f48b3150";s:9:"*region";s:7:"content";s:16:"*configuration";a:4:{s:13:"label_display";s:1:"0";s:15:"context_mapping";a:1:{s:6:"entity";s:21:"layout_builder.entity";}s:2:"id";s:45:"field_block:node:case_study:field_website_url";s:9:"formatter";a:4:{s:5:"label";s:5:"above";s:8:"settings";a:5:{s:11:"trim_length";i:80;s:8:"url_only";b:0;s:9:"url_plain";b:0;s:3:"rel";s:0:"";s:6:"target";s:0:"";}s:20:"third_party_settings";a:0:{}s:4:"type";s:4:"link";}}s:9:"*weight";i:6;s:13:"*additional";a:0:{}}}s:21:"*thirdPartySettings";a:0:{}}

I don't recognize any of these blocks. I only use custom blocks in my layouts and these seem to be autogenerated blocks based on individual fields or something.

The field I deleted is field_logo which I can see referenced in the broken layout section.

And just to reiterate, both of these layout sections contain no blocks, they are completely empty.

What confuses me is that nothing looks out of the ordinary neither when viewing the layout frontend nor when editing the layout backend either logged in as an administrator or when viewing the layout as an anonymous user, so why is this exception being thrown? The only thing this exception is doing for me is making the site unusable for no good reason. If this exception wasn't thrown I wouldn't have known anything was wrong because everything would have continued working as before.

And if removing this exception is out of the question then I at least feel that layout builder should make sure layout sections are synced when you save the layout, ie. remove any block references that aren't used or have never been used. If I save an empty layout section I expect there to be an empty layout section saved in my database and not a layout section containing references to a lot of blocks i have no knowledge of and have never used.

As for fixing my broken layout section nothing seemed to work apart from deleting it. I tried saving the layout, updating the layout section, adding/removing blocks to it, but this made no difference. The reference to all the unknown blocks, including the missing field_logo block, remained. After deleting the broken layout section I wasn't able to reproduce the issue so I have no idea how it appeared in the first place. But without commenting out the line throwing the exception I wouldn't have been able to do any of this, meaning if I didn't have code access I wouldn't be able to fix an issue that wasn't actually a visible issue to begin with.

TL;DR: Why is this exception being thrown? It seems to break sites with no visible errors and at the very least it makes it impossible to rectify the problem causing the exception to be thrown in the first place.

After debugging this some more this seems to be 100% reproducible for me without ever using the fields in a layout.

Steps I used to reproduce:

1. Create a new content type. Just give it a name, you don't have to change anything else. This new content type will have a default body field (which is the field that will cause this exception to be thrown later on)
2. Click "Manage display" and enable "Use Layout Builder" and "Allow each content item to have its layout customized."
3. Create a new node for your content type.
4. Go to the "Layout" tab of your new node and just save the layout without changing anything. You should see that you have a default empty layout section here.
5. Now go and delete the body field from your content type.
6. Your node will now give you a WSOD due to this exception being thrown. Even though you didn't use the body field anywhere.

What seems to be happening is that the default layout section provided seems to be broken. Even though it's visually empty and identical to any other empty layout section it has hidden references to every field on your content type and breaks if any field that existed when you created the node is later deleted. The only way to fix it is to delete this broken section. Any new sections you manually create won't have this issue and creating new nodes after the field is deleted also works fine, the default broken layout section seems to snapshot all available fields at the moment you first save the layout, so if you later delete a field it will break again, unless you've already replaced the broken layout section beforehand.

Edit: The above was tested on a site running a lot of custom code. I will need to test this on a default install as well.

Step 4 is curious. When you go to the Layout tab of your new node and save the layout without changing anything, you should see the same thing as before, which was all of your fields. Not an empty layout.
Perhaps something custom is causing that behavior, but it's definitely that step where things are going off the rails.

What step 4 does is save to the database that you've overridden the default layout for this node. This saves all layout sections along with all the blocks to the database which saves the field reference to the body field which breaks when you later delete the field. But after testing on a clean install I see what's going on.

I tested this on a clean installation just now and the exact same thing happens apart from one very noticeable difference and that is that the default "field blocks" are actually visible, making this more in line with the original description of the issue. On the site I was testing with initially I have this module https://www.drupal.org/project/layout_builder_restrictions which was probably hiding these default blocks making it seem like the layout section was completely empty when it was in fact containing the default field blocks.

On the clean install I can see that I have a block for "Links" and a block for "Body" which are the fields you get by default when you create a new content type. And this all makes sense now. When you convert a content type to use layout builder for the display it takes all fields not marked as being "Hidden" and inserts them as blocks in the default layout section. And then when you create a node of this content type and save the layout this default layout section along with the default blocks are saved as the overridden layout for the node even though you never added them yourself. So when you later delete the body field your node has a reference to that field and causes the exception to be thrown.

Either way the exception seems to be the root of the problem here. If it wasn't thrown everything would work as expected. Having a default layout section with the fields that were previously set to be visible as blocks seem like the expected behaviour to me. The only problem here is the exception making it impossible for the user to fix a layout causing a WSOD if he forgot to do things in the correct order. Commenting out the exception shows me this message where the block used to be:

This block is broken or missing. You may be missing content or you might need to enable the original module.

and lets me remove the block thus fixing the problem.

I feel like just removing the exception would fix this issue, but for all I know this exception serves multiple purposes, but throwing it when you delete a field seems like an unintended one.

Lastly the steps to reproduce this exception on a clean Drupal installation are the same as the ones above with a few differences:

1. Install a clean Drupal installation.
2. Enable the Layout builder module.
3. Create a new content type. Just give it a name, you don't have to change anything else. This new content type will have a default body field (which is the field that will cause this exception to be thrown later on)
4. Click "Manage display" and enable "Use Layout Builder" and "Allow each content item to have its layout customized."
5. Create a new node for your content type.
6. Go to the "Layout" tab of your new node and just save the layout without changing anything. You will see that the "Body"-field as well as the "Links"-field was automatically added as blocks to this layout.
7. Now go and delete the body field from your content type.
8. Your node will now give you a WSOD when you try to view it or edit the layout for it due to this exception being thrown. Nothing here is actually broken, but the exception makes it impossible for the user to fix the problem by removing the block referencing the deleted body field.

#9 addresses my confusion, thanks for the update. See also #3115759: Prevent auto-adding new fields to LB layouts.

We could try to find a way to catch this exception just for field blocks, and log the error. I'm hesitant to always catch it for all blocks.
Maybe for all derivative blocks?

The config entity system provides a mechanism for dependency resolution. LB uses that to ensure that if you try to delete a field that is in use by any *default* layouts, it prevents you from doing so.
Content entities, which are the underlying storage mechanism for *overridden* layouts, do not provide such a mechanism.

Without inventing a new storage mechanism, right now in order to accomplish #12 we'd have to load every revision in every language of every entity on the site, and check it for that field.

I'm up for other suggestions on how to proceed, of course!

And this is a good thing, a layout that references no-existent content is a corrupted layout and that needs to be handled.

I don't agree at all with this. Imagine if entity reference fields worked this way: "Sorry, you can't delete this entity without first removing it from the x amounts of entities referencing it.". It would be unusable. Instead you're allowed to delete any entity even if it is referenced from other entities and the other entities just clean themselves up so they continue working, invisible to the user.

The behavior for layouts with blocks that reference deleted fields is nearly perfect in my opinion, but only when the exception is commented out, with it in place the behavior is broken or "corruped".

The only thing I thing I really don't like about the current behavior when you comment out the exception is that the "This block is broken or missing." message is shown frontend to everyone as well. This is completely uneccesary. Just show nothing. Print a warning/error message to the admin potentially instead or just show the message when you're editing the layout the way it is currently.

And if we were to mimick the behavior of deleting fields as it is without layout builder then the blocks referencing the deleted fields would just remove themselves from any layouts they're in automatically, again entirely invisible to the user.

If I tried deleting a field and Drupal told me to remove it from the potentially hundreds of layouts using it before I was allowed to delete it (a problem compounded by the fact that fields are automatically added to layouts at the moment), I would ask myself why Drupal couldn't just do this for me automatically.

WSOD'ing the site the way it is now is the worst way to handle this in my opinion.

That's a false equivalency. You're comparing deleting a single field value with deleting the entire field itself. Content vs config.

As I said, I'm happy to find a way to more gracefully handle this exception. It was written this way long before LB allowed fields to be placed, and should be revisited.

#2980299: Layout Builder cannot be uninstalled while overrides exist; no easy way to revert all overrides originally started as an addition to admin/reports, but took a different approach in the end.

Restoring the metadata.

Since Layout Builder is a tool for site builders, we should provide a way for site builders to resolve the problem. WOD gets you completely stuck.

Would it be possible to work with the concept "this item is broken"? Thinking along the lines that layout builder can handle a views block as "Broken/Missing block" when the view has been disabled. That way site builders can still access layout builder to remove that broken block. Would that work for fields, too?

#18 got me thinking... why does this work for views blocks but not field blocks?

The answer is because the field block expects a context, and tries to assign that context to the broken block.
The block plugin system has already determined that the field is not able to be displayed, yet Layout Builder attempts to set the contexts on the broken block.

Why? Because due to BlockBase, the Broken block declares itself as context-aware, something it is very much not.

Unfortunately, even though BlockPluginInterface does not enforce context-aware-ness, BlockBase implicitly guarantees it via its inheritance. By moving the block-specific logic to a new trait and having Broken use that directly, it no longer triggers this bug.

Thanks to all for their patience with me on this issue so far!

I can confirm the problem still exists and #19 fixed it for us in several distinct projects. Would be nice to get further feedback! :)

Did someone of the others already try the patch?

We've run in the same problem again after we've deleted an entity field what was embeded in layout builder.

Patch #19 fixed this for us!

I can confirm the patch #19 solved the problem for us in one project. Otherwise is impossible to even remove the missing block from the layout by user interface.

I'm glad the patch is working! Now it needs automated tests that expose the bug and prove that the patch fixes it.

Just another confirmation that the patch in #19 allowed us to recover a node with a broken field reference. Thanks!

I have problems with installing this patch. I have a site running under 8.8.6 and both, running this patch via terminal and via composer, failed.
I updated my site to 8.9.0-beta 1 and 8.9.0-rc1 afterwards and still have no luck.

After checking I found that b/core/lib/Drupal/Core/Block/BlockPluginTrait.php wasn't created and content from a/core/lib/Drupal/Core/Block/BlockBase.php wasn't copied. The other files got patched successfully.

Am I missing anything? Thanks for any help.

The patch is written for Drupal 9, but also applies cleanly to Drupal 8.9 via git apply.

I was having issues applying #19 in my project via composer (against 8.9), which I think has to do with the copy from/copy to on l. 288/289. I re-rolled against 9.1 without those and it seems to apply in my project now. Attaching here in case someone else has similar issues.

#29 I forget, but does composer use patch or git apply? The patch I posted is only compatible with git.

Here's a first pass on test coverage. The FAIL patch is equivalent to an interdiff.

#30 From what I can tell, composer-patches should try git apply first before falling back to patch, but for whatever reason #19 would not apply via composer while it worked just fine if I manually ran git apply

After applying the patch from 30 (the passed one), I am getting the following error.

Fatal error: Uncaught Error: Class 'Drupal\layout_builder\Cache\ExtraFieldBlockCacheTagInvalidator' not found in /var/www/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 269

Error: Class 'Drupal\layout_builder\Cache\ExtraFieldBlockCacheTagInvalidator' not found in /var/www/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 269

#33 That error is unrelated to this issue. That class does not exist and has never existed in core. My guess is that you were using a patch that added that class, stopped using the patch, and did not clear the cache afterwards. EDIT: Maybe #3056135: Viewing Layout Builder enable node fails after Workflow enabled on bundle if another node was created before enabling?

Patch still needs review

After applying the patch #30 i m getting 500 error
When I was applying the patch i got some errors

patching file core/lib/Drupal/Core/Block/BlockBase.php
patching file core/lib/Drupal/Core/Block/BlockBase.php
Hunk #1 FAILED at 2.
Hunk #2 FAILED at 83.
Hunk #3 FAILED at 168.
3 out of 3 hunks FAILED -- saving rejects to file core/lib/Drupal/Core/Block/BlockBase.php.rej
patching file core/lib/Drupal/Core/Block/Plugin/Block/Broken.php
patching file core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php

The patch cleanly applies to 8.8 and 9.1, sorry for the trouble you are having.

#30 applies cleanly to a production 8.9.1 site and a new 9.1.x test site and seems to work great!

+++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
@@ -67,6 +67,40 @@ protected function setUp(): void {
     ]);
   }
 
+  /**
+   * Tests deleting a field in-use by an overridden layout.
+   */
+  public function testDeleteField() {
...
+    // Enable layout builder overrides.
+    $this->drupalGet('admin/structure/types/manage/bundle_with_section_field/display/default');
+    $page->checkField('layout[enabled]');
+    $page->pressButton('Save');
+    $page->checkField('layout[allow_custom]');
+    $page->pressButton('Save');

Nit: this seems to be at least the third different way that layout builder overrides are enabled in this test class, so maybe one of the other ones could be reused?

The other two being:
in testOverridesWithoutDefaultsAccess():

    LayoutBuilderEntityViewDisplay::load('node.bundle_with_section_field.default')
      ->enableLayoutBuilder()
      ->setOverridable()
      ->save();

and in testOverrides() and elsewhere:

    $this->drupalGet('admin/structure/types/manage/bundle_with_section_field/display/default');
    $this->drupalPostForm(NULL, ['layout[enabled]' => TRUE], 'Save');
    $this->drupalPostForm(NULL, ['layout[allow_custom]' => TRUE], 'Save');

Very good point. I'm fixing the addition here, and opened #3158916: Enable Layout Builder via code in tests when that UI is not being tested to fix the rest later.

Looks good. RTBC.

Updated the issue summary.

RTBC +1
The patch works fine!

Before apply patch:

After apply patch:

+++ b/core/lib/Drupal/Core/Block/BlockPluginTrait.php
@@ -88,7 +83,7 @@ protected function baseConfigurationDefaults() {
-      'label_display' => static::BLOCK_LABEL_VISIBLE,
+      'label_display' => BlockPluginInterface::BLOCK_LABEL_VISIBLE,

@@ -173,8 +168,8 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
-      '#default_value' => ($this->configuration['label_display'] === static::BLOCK_LABEL_VISIBLE),
-      '#return_value' => static::BLOCK_LABEL_VISIBLE,
+      '#default_value' => ($this->configuration['label_display'] === BlockPluginInterface::BLOCK_LABEL_VISIBLE),
+      '#return_value' => BlockPluginInterface::BLOCK_LABEL_VISIBLE,

I think this is a subtle API change. It's possible but very unlikely and it'd be incorrect but they can be overridden no? I'm not sure that this matters but are these changes actually necessary? Can't we leave as is. I'm not sure. I think anything that uses this trait will have to implement the interface.

#42:
Constants defined in interfaces can't be overridden by any class or interface that inherits them. https://www.php.net/manual/en/language.oop5.interfaces.php#language.oop5...

@godotislate thanks! that's v good to know. It would have been nice if there was progress on https://wiki.php.net/rfc/traits-with-interfaces

1. +++ b/core/lib/Drupal/Core/Block/BlockPluginTrait.php
   @@ -2,31 +2,26 @@
    /**
   - * Defines a base block implementation that most blocks plugins will extend.
   + * Provides the base implementation of a block plugin.
     *
   - * This abstract class provides the generic block configuration form, default
   - * block settings, and handling for general user-defined block visibility
   - * settings.
   + * @see \Drupal\Core\Block\BlockPluginInterface
     *
     * @ingroup block_api
     */
   -abstract class BlockBase extends ContextAwarePluginBase implements BlockPluginInterface, PluginWithFormsInterface, PreviewFallbackInterface {
   +trait BlockPluginTrait {
   

   I think we need a bit more documentation as to when you should use this over BlackBase. If the answer is never then we might consider making this @internal.

2. +++ b/core/lib/Drupal/Core/Block/Plugin/Block/Broken.php
   @@ -14,7 +17,10 @@
   +class Broken extends PluginBase implements BlockPluginInterface {
   +
   +  use BlockPluginTrait;
   +  use CacheableDependencyTrait;
   

   I've looked at the rest of this and it looks great. And the bit that took some time to unpick was why we needed to add the use CacheableDependencyTrait.

   And it's because BlockPluginInterface extends CacheableDependencyInterface. In BlockBase this is implemented by the \Drupal\Core\Plugin\ContextAwarePluginBase class which BlockBase extends from.

   And so this is where it gets a bit awkward because BlockPluginTrait does not fully implement BlockPluginInterface. I think in order to be complete the trait should implement abstract methods for these methods.

   In fact here are all the methods you'd need to implement:

     public function build() {}
   
     public function getCacheContexts() {}
   
     public function getCacheTags() {  }
   
     public function getCacheMaxAge() {}
   
     public function getBaseId() {}
   
     public function getDerivativeId() {}
   
     public function getPluginId() {}
   
     public function getPluginDefinition() {}
   

   If you extend PluginBase - which I think is a reasonable requirement for using the trait you'll get implementations of getBaseId, getDerivativeId, getPluginId and getPluginDefinition. So I think we should consider adding abstract methods in the trait for build, getCacheContexts, getCacheTags, getCacheMaxAge - or documenting that it is an incomplete implementation and that CacheableDependencyTrait can be used or something like that.

@alexpott: Yeah, the constant thing was very much "TIL ..." for me.

#45
1) Good point. There's no real reason to use BlockPluginTrait that I can see, let's make it @internal
2) I think making it @internal removes the need for this. But had I not gone this way, I would have added docs pointing to CacheableDependencyTrait and not added any abstract methods.

Looks good.

The only thing that makes me slightly uncomfortable is the methods on the trait that are not part of the interface. I.e. \Drupal\Core\Block\BlockPluginTrait::setTransliteration() and \Drupal\Core\Block\BlockPluginTrait::transliteration() - they are there for \Drupal\Core\Block\BlockPluginTrait::getMachineNameSuggestion() which is meaningless for the Broken block - this method is always going to return missingbroken.

However all this said we're marking the new trait @internal so maybe all of that does not matter.

I hope this is my final question - but one thing I'm now wondering about this...

Why? Because due to BlockBase, the Broken block declares itself as context-aware, something it is very much not.

#19

Currently the only context-aware block plugins are FieldBlocks, used by Layout Builder in core and various places in contrib.

(issue summary)
So this means all plugins are marked as context aware but we're saying that only FieldBlocks are. Will this cause other problems in the future? I.e. will we have to make other block plugins not pretend context aware?

Your point about getMachineNameSuggestion is fair, but I don't think it's worth splitting up the trait between it and BlockBase more. And it is @internal if we choose to do so later.

Broken plugins are a manager-level concern.
See \Drupal\Core\Block\BlockManager::getFallbackPluginId() and \Drupal\Component\Plugin\PluginManagerBase::handlePluginNotFound()
This is the only runtime case where plugins are swapped out for each other, and where the contexts of one plugin would be applied to another.
I cannot envision another way for this to happen (though because I just said that, it will now happen...)

If that does come to pass, then the solution is straightforward: Make BlockPluginTrait the new Correct Way™ to write block plugins, and deprecate BlockBase, forcing those who want to continue to be context aware to explicitly opt-in to that.

Oh noes... now I have more questions after sleeping on this... Given this is such a special case - was doing something like:

diff --git a/core/modules/layout_builder/src/SectionComponent.php b/core/modules/layout_builder/src/SectionComponent.php
index c09492e3ab..91d82748d7 100644
--- a/core/modules/layout_builder/src/SectionComponent.php
+++ b/core/modules/layout_builder/src/SectionComponent.php
@@ -3,6 +3,7 @@
 namespace Drupal\layout_builder;
 
 use Drupal\Component\Plugin\Exception\PluginException;
+use Drupal\Core\Block\Plugin\Block\Broken;
 use Drupal\Core\Plugin\ContextAwarePluginInterface;
 use Drupal\layout_builder\Event\SectionComponentBuildRenderArrayEvent;
 
@@ -241,7 +242,7 @@ public function getUuid() {
    */
   public function getPlugin(array $contexts = []) {
     $plugin = $this->pluginManager()->createInstance($this->getPluginId(), $this->getConfiguration());
-    if ($contexts && $plugin instanceof ContextAwarePluginInterface) {
+    if ($contexts && $plugin instanceof ContextAwarePluginInterface && !($plugin instanceof Broken)) {
       $this->contextHandler()->applyContextMapping($plugin, $contexts);
     }
     return $plugin;

Considered?

As the Broken block is part of core I think this is okay from a dependency point-of-view. And makes it plain that we have to deal with broken plugins. I guess a disadvantage of this approach is anything doing something similar has to make the same change. But OTOH I think thinking about the Broken block and its effects and why it's there is useful because it's the sign of others things being wrong....

... according to the issue summary this whole problem comes about from deleting a field. This implies to me that we've got a dependency problem. If you delete a field the field block should be automatically removed from the layout. Yes there are other ways you can end up with the Broken block but in really Broken is a legacy of the old way of handling configuration dependencies. Uninstalling a module that provides a block that is used in a layout should remove the block too.

@alexpott: I put in a patch similar what you're suggesting in a duplicate issue: #3150080: "Assigned contexts were not satisfied" ContextException when rendering 'broken' section components expecting context

I don't have a preference between either approach.

Removing blocks from overrides storage when their dependencies are uninstalled would be quite the herculean challenge, though. The blocks do get removed from defaults layouts when their module dependencies are uninstalled.

On the first point of #51:
@godotislate's patch on the other issue is the correct implementation of that approach (plugin classes are not guaranteed, plugin ID are).

But either way, Broken plugin shouldn't be context-aware, and this addresses that.

On the second point:
We have no mechanism for loading every override of every entity in every language (and what about revisions?) to remove old plugins. As mentioned, defaults (entity displays) are handled just fine.

Thanks for all the replies @tim.plunkett and @godotislate.

Committed 9f4e556 and pushed to 9.1.x. Thanks!

diff --git a/core/lib/Drupal/Core/Block/BlockPluginTrait.php b/core/lib/Drupal/Core/Block/BlockPluginTrait.php
index a4fce4b413..c7c47660f9 100644
--- a/core/lib/Drupal/Core/Block/BlockPluginTrait.php
+++ b/core/lib/Drupal/Core/Block/BlockPluginTrait.php
@@ -210,7 +210,8 @@ public function validateConfigurationForm(array &$form, FormStateInterface $form
   /**
    * {@inheritdoc}
    */
-  public function blockValidate($form, FormStateInterface $form_state) {}
+  public function blockValidate($form, FormStateInterface $form_state) {
+  }
 
   /**
    * {@inheritdoc}
@@ -233,7 +234,8 @@ public function submitConfigurationForm(array &$form, FormStateInterface $form_s
   /**
    * {@inheritdoc}
    */
-  public function blockSubmit($form, FormStateInterface $form_state) {}
+  public function blockSubmit($form, FormStateInterface $form_state) {
+  }
 
   /**
    * {@inheritdoc}

Fixed coding standards on commit.

Will backport to 9.0.x and 8.9.x if tests are green there.

Cherry-picked back to 8.9.x - I think this one was important to fix there given the number of reports / WSOD / duplicate issues.