WSOD after deleting a custom block content entity

We need to remove instances too.

It seems to me that there are a few options:

1. Remove the delete link from contextual filters
2. Make the delete link delete the block AND all its instances
3. Make the delete link delete just the given instance

They are all valid solutions... personally I would expect "3" but who knows...

Whatever it may be (I expect 3 too), it needs to be something less vague than simply "delete". So, either "delete block" (+ a warning in a modal to state that ALL instances are going to be deleted) or "delete this block instance" (which is pretty much self-explanatory).

In fact I believe we can make a better distinction of the two by perhaps using "delete" for the more destructive block deletion and "remove" (that implies it can be added back again) for the instance. Although I cannot know how that would translate to other languages (in some languages both words might be the same or have less distinction between them).

I don't see why its a problem that you can delete the block directly from there. You can also do the same on a node. I think its a minority case, that you'd use the same block elsewhere as well, the delete page actually can inform you about that if we want to. That an error is displayed then is a different question, not related to the Delete operation being there.

I'm wondering how this is a major.

If you create and place a block and then delete it using the delete link, it doesn't remove the block instance config entity before deleting and your site is then hosed.
I think that warrants major but happy for it to be knocked down to normal.

Ok, I agree that is at least major (I think critical).

But the bug then is not that Delete is a contextual link, but that deleting a custom block will not result in all placements removed as well, right? If you go to edit the block and then go to Delete on the edit screen, it will do the same, no? I tried (DID NOT use the contextual delete link), and first I got this "ok" message:

But then after trying to submit another block, I indeed got into this for all my site pages with no way to get out of this:

If it is so easy to WSOD a site as to create a custom block, place it and then remove the content entity, then this is a critical.

In which case this just became my top priority :)
Will try and look at it over the weekend

Fail and pass

Looks generally pretty good! Also quick work! :)

+++ b/core/modules/block/custom_block/custom_block.pages.incundefined
@@ -99,6 +99,18 @@ function custom_block_delete_form($form, &$form_state, CustomBlock $block) {
+      '#markup' => format_plural(count($instances), 'This will also remove 1 placed block instance', 'This will also remove @count placed block instances'),

Make this a full sentence IMHO. Add a period.

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Tests/CustomBlockCreationTest.phpundefined
@@ -102,4 +102,49 @@ public function testFailedBlockCreation() {
+    $this->assertText('This will also remove 1 placed block instance');

Should assert format_plural(1, '...', '...'), this may look odd, but it would find the exact string translation that way.

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Tests/CustomBlockCreationTest.phpundefined
@@ -102,4 +102,49 @@ public function testFailedBlockCreation() {
+    $this->assertRaw(format_string('Custom block %name has been deleted.', array('%name' => $edit['info'])));

Should assert this with t() as well.

Fixes issues at #10

Nice fix with test, +1 rtbc

+++ b/core/modules/block/custom_block/custom_block.pages.incundefined
@@ -99,6 +99,18 @@ function custom_block_delete_form($form, &$form_state, CustomBlock $block) {
+  $instances = entity_load_multiple_by_properties('block', array('plugin' => 'custom_block:' . $block->uuid->value));
+  if (!empty($instances)) {
+    $form['instances'] = array(
+      '#type' => 'value',
+      '#value' => $instances,
+    );
+    $form['message'] = array(
+      '#type' => 'markup',
+      '#markup' => format_plural(count($instances), 'This will also remove 1 placed block instance.', 'This will also remove @count placed block instances.'),
+    );
+  }

To prevent a PHP warning in the submit I think this should be... so that we always have $form_state['values']['instances']

  $instances = entity_load_multiple_by_properties('block', array('plugin' => 'custom_block:' . $block->uuid->value));
  $form['instances'] = array(
    '#type' => 'value',
    '#value' => $instances,
  );
  if (!empty($instances)) {
    $form['message'] = array(
      '#type' => 'markup',
      '#markup' => format_plural(count($instances), 'This will also remove 1 placed block instance.', 'This will also remove @count placed block instances.'),
    );
  }

Changes in #13

+++ b/core/modules/block/custom_block/custom_block.pages.incundefined
@@ -99,6 +99,19 @@ function custom_block_delete_form($form, &$form_state, CustomBlock $block) {
+  if (!empty($instances)) {
+    $form['message'] = array(
+      '#type' => 'markup',
+      '#markup' => format_plural(count($instances), 'This will also remove 1 placed block instance.', 'This will also remove @count placed block instances.'),
+    );
+  }

Oops... If the following works then this should be...

  $form['message'] = array(
    '#type' => 'markup',
    '#markup' => format_plural(count($instances), 'This will also remove 1 placed block instance.', 'This will also remove @count placed block instances.'),
    '#access' => !empty($instances),
  );

If we avoid conditionals in form builders alters become much simpler...

Changes in #16 plus additional test for not showing a message if there are no instances.

Discussed the new test with IRC with @alexpott and agreed to move it into testBlockDelete() and make it more clear that we are testing the confirm form message.

I think we need this element in form array. It was broken. So filed a better to have count and plurals are separate.

avoid conditionals in form builders alters become much simpler...

anyway we should decide on a language-aware renderable element be a source for markup

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Tests/CustomBlockCreationTest.phpundefined
@@ -102,4 +102,49 @@ public function testFailedBlockCreation() {
+    $langcode = Language::LANGCODE_NOT_SPECIFIED;
...
+    $langcode = Language::LANGCODE_NOT_SPECIFIED;

Language tested but is there injection?
Also - do this a unit test for form and UI of delete as a part of testBlockDelete() makes sense from follow-up

Discussed with @alexpott and @andypost in IRC last night, and the suggestion was to move the deleting of instances behind CustomBlock::delete(). This would mean displaying the additional message about how many instances are going to be removed, more complex, as we'd need to do a query first. Something like CustomBlock::countInstances()?

How about ::getInstances, that would be more useful. But +1 for making it part of the delete method, decoupling from forms is definitely better design.

Something like so?

#22 plus new test for getInstances method, plus remove unneeded 'instances' form key.

So about #d8mi we have separate issue now and everything addressed about the scope. RTBC

also the ability to implement unit test would be found in follow-up #2015543: Decide on a language-aware renderable element in Twig templates

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockInterface.phpundefined
@@ -38,4 +38,12 @@ public function setTheme($theme);
+   * Gets the configured instances of this custom block.
...
+  public function getInstances();

Awesome!

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Tests/CustomBlockCreationTest.phpundefined
@@ -102,4 +102,64 @@ public function testFailedBlockCreation() {
+    // Test getInstances method.
+    $this->assertEqual(1, count($block->getInstances()));

Makes sense here too!

Yay!

Committed a74a17a and pushed to 8.x. Thanks!