Create a view block, place the block, delete the block, BOOM your site is dead

Let's start with a simple approach, though we need tests and an actual fix :)

This time it works, working on tests right now.

+++ b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.phpundefined
@@ -176,10 +176,23 @@ public function submitOptionsForm(&$form, &$form_state) {
+    foreach (entity_load_multiple('block') as $block_id => $block) {
+      if (strpos($block->get('plugin'), 'views_block:') !== FALSE) {

This would be a great place for the new config entity query!

This would be a great place for the new config entity query!

I totally agree, there are also multiple places in block.module which could use that.

This time with the full test code.

+++ b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.phpundefined
@@ -176,10 +176,23 @@ public function submitOptionsForm(&$form, &$form_state) {
   public function usesExposed() {
-      if ($this->isAJAXEnabled()) {
-        return parent::usesExposed();
+    if ($this->isAJAXEnabled()) {
+      return parent::usesExposed();
+    }
+    return FALSE;

Fixing indentation isn't in scope :)

+++ b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.phpundefined
@@ -176,10 +176,23 @@ public function submitOptionsForm(&$form, &$form_state) {
+    foreach (entity_load_multiple('block') as $block_id => $block) {

We don't use $block_id here

+++ b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.phpundefined
@@ -176,10 +176,23 @@ public function submitOptionsForm(&$form, &$form_state) {
+      if (strpos($block->get('plugin'), 'views_block:') !== FALSE) {
+        $block->delete();

This seems like it would delete all placed blocks of all displays of all views if one block display was removed

+++ b/core/modules/views/lib/Drupal/views/DisplayBag.phpundefined
@@ -92,4 +92,14 @@ protected function initializePlugin($display_id) {
+  public function remove($instance_id) {
+    $this->get($instance_id)->remove();

This is a great idea!

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.phpundefined
@@ -2589,6 +2589,13 @@ public function validate() {
+   * React on deleting a display.

Reacts

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.phpundefined
@@ -2589,6 +2589,13 @@ public function validate() {
+  public function remove() {
+
+  }

Doesn't need the blank line

I'd recommend just using drupalPlaceBlock() in the test, and not providing a default config file.
We should add explicit coverage for:
Adding two instances of the same display, and making sure they are both removed
A view with two block displays, adding one placed block instance of each, removing one display and making sure the other is still there
A second view with a block display with an instance, that shouldn't be affected by the deletion of a display on another view

Thanks for suggestion all this possible combinations.

Fixing indentation isn't in scope :)

This piece of indentation is brutal.

Never fix something right before uploading the patch without rerunning the test.

+++ b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.phpundefined
@@ -182,4 +182,16 @@ public function usesExposed() {
+    $blocks = entity_load_multiple_by_properties('block', array('plugin' => 'views_block:' . $this->view->storage->id() . '-' . $this->display['id']));
+    foreach ($blocks as $block) {

It doesn't really matter, but I'd probably rewrite this as

$plugin_id = 'views_block:' . $this->view->storage->id() . '-' . $this->display['id'];
foreach (entity_load_multiple_by_prperties('block', array('plugin' => $plugin_id)) as $block) {

+++ b/core/modules/block/lib/Drupal/block/Tests/Views/DisplayBlockTest.phpundefined
@@ -0,0 +1,130 @@
+    ViewTestData::importTestViews(get_class($this), array('block_test_views'));

+++ b/core/modules/block/tests/block_test_views/block_test_views.moduleundefined
@@ -0,0 +1 @@
diff --git a/core/modules/block/tests/block_test_views/config/views.view.test_view_block.yml b/core/modules/block/tests/block_test_views/config/views.view.test_view_block.yml

+++ b/core/modules/block/tests/block_test_views/config/views.view.test_view_block.ymlundefined
@@ -0,0 +1,47 @@
diff --git a/core/modules/block/tests/block_test_views/config/views.view.test_view_block2.yml b/core/modules/block/tests/block_test_views/config/views.view.test_view_block2.yml

These should be moved out of the config directory and into the test_views directory.

+++ b/core/modules/block/lib/Drupal/block/Tests/Views/DisplayBlockTest.phpundefined
@@ -0,0 +1,130 @@
+    ViewTestData::importTestViews(get_class($this), array('block_test_views'));
+
+    parent::setUp();

I think that setUp needs to come first, or the modules won't be enabled?

+++ b/core/modules/block/lib/Drupal/block/Tests/Views/DisplayBlockTest.phpundefined
@@ -0,0 +1,130 @@
+   * Check to see whether a block appears on the page.
...
+   * Check to see whether a block does not appears on the page.

Checks

+++ b/core/modules/block/lib/Drupal/block/Tests/Views/DisplayBlockTest.phpundefined
@@ -0,0 +1,130 @@
+  protected function assertBlockAppears(Block $block) {
...
+  protected function assertNoBlockAppears(Block $block) {

These are similar enough, is it worth writing a helper method?

+++ b/core/modules/block/lib/Drupal/block/Tests/Views/DisplayBlockTest.phpundefined
@@ -0,0 +1,130 @@
+    $this->assertFalse((bool) $block_storage_controller->load(array($block_1->id())));

Here, and elsewhere: assertFalse and assertTrue cast the value to (bool) for you.

+++ b/core/modules/block/tests/block_test_views/block_test_views.infoundefined
@@ -0,0 +1,7 @@
+name = Block test views
+description = Provides a view and block to test block displays in views.
+package = Core
+version = VERSION
+core = 8.x
+dependencies[] = block
+dependencies[] = views

Missing hidden = TRUE

These should be moved out of the config directory and into the test_views directory.

One detail which maybe interests you: I had to put that into the config directory, as enabling the block module causes the block entities to be imported but break as the view didn't existed before.

That's also the reason for

I think that setUp needs to come first, or the modules won't be enabled?

There we go.

.

#10: drupal-1898804-10.patch queued for re-testing.

If you use assertTrue with no message, it tries to print out the whole value, which in this case was a block config entity, causing the "Nesting level too deep - recursive dependency?" fatal.

The test coverage looks good and the fix makes sense.

Hey you suggested that assertFalse would already cast to bool :)

This looks like it should be an API function provided by block module, i.e. "delete all instances of this block". I can see lots of modules having to copy and paste this code.

+    foreach (entity_load_multiple_by_properties('block', array('plugin' => $plugin_id)) as $block) {
+      $block->delete();
+    }

Also I'm wondering a bit whether the responsibility here should entirely fall on the Views display plugin, or if it does, it feels like there should be docs in block module pointing out that if you remove a block type you also need to delete the instances yourself.

What happens with aggregator feeds and instances of those blocks? Or menu blocks?

When modules invoke hooks for delete (like hook_menu_delete()), block.module implements the hook.
Other cases, like Views or Aggregator's aggregator_save_category() or aggregator_save_category(), have to handle it themselves.

I think it might be worth coming up with a standard pattern for this, but I'm pretty sure it should be a module's job to handle this.

Basically, derivative discovery is passive, and defined internally by each module, so it should be their job to clean it up. If we do anything, we should put docs on DerivativeDiscoveryDecorator.

It seems the generic issue for this is #1886462: Determine how to clean up plugin instances when the derivative definitions change, but this fixes the bug in a manner consistent with other derivatives.

OK thanks for the link to the generic issue, following that one now. Committed/pushed this to 8.x.