Problem/Motivation

'Delete' on the contextual menu for a custom block deletes the block, not the instance, and then navigating to another page gives the error, "The website has encountered an error. Please try again later".

Perhaps there is a way to fix the error, but I had to reinstall.

To reproduce:

  1. In the Tools block, click 'Add Custom block'
  2. Enter title and text, then click 'Save'
  3. Select Region, then click 'Save Block'
  4. Navigate to Home
  5. Find the new custom block
  6. Hover over block until editing icon appears
  7. Click icon, see screenshot.
  8. customblock.png

  9. Caution, will break your site. Click 'delete'. Navigate to another page. The error msg displays.

Proposed resolution

Remove 'Delete' from the contextual menu.

Remaining tasks

  • Discuss.
  • Create initial patch to remove 'delete' from the contextual links.

User interface changes

See Proposed resolution

API changes

None

#574018: Add "Delete" button to "Configure block" screen
#1956496: Custom block contextual links should differentiate between editing/deleting the block instance and the custom block entity
#1956504: Add block instance delete to block contextual links
#1997262: Convert custom_block_delete_form to new form interface

Files: 
CommentFileSizeAuthor
#23 delete-block-1994146.23.interdiff.txt2.58 KBlarowlan
#23 delete-block-1994146.23.patch5.31 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 55,604 pass(es).
[ View ]
#22 delete-block-1994146.22.interdiff.txt2.59 KBlarowlan
#18 1994146-delete-block-18.patch4.1 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 55,524 pass(es).
[ View ]
#18 1994146-interdiff.txt1.48 KBkim.pepper
#17 1994146-delete-block-17.patch4.15 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#17 1994146-interdiff.txt1.98 KBkim.pepper
#15 1994146-delete-block-15.patch3.63 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#15 1994146-interdiff.txt943 byteskim.pepper
#11 delete-block-1994146.11.interdiff.txt1.85 KBlarowlan
#11 delete-block-1994146.11.patch3.64 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 55,469 pass(es).
[ View ]
#9 delete-block-1994146.fail_.patch2.2 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 55,483 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#9 delete-block-1994146.pass_.patch3.57 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 55,585 pass(es).
[ View ]
#7 Screenshot_6_7_13_10_42_AM.png15.91 KBGábor Hojtsy
#7 Screenshot_6_7_13_10_45_AM.png29.14 KBGábor Hojtsy
customblock.png6.94 KBquietone

Comments

larowlan’s picture

Category:task» bug

We need to remove instances too.

larowlan’s picture

Assigned:Unassigned» larowlan
Priority:Normal» Major
bleen18’s picture

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...

klonos’s picture

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).

Gábor Hojtsy’s picture

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.

larowlan’s picture

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.

Gábor Hojtsy’s picture

Title:Remove delete from custom block contextual links» WSOD after deleting a custom block content entity
Priority:Major» Critical
Issue tags:+Needs issue summary update
StatusFileSize
new29.14 KB
new15.91 KB

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:

Screenshot_6_7_13_10_42_AM.png

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:

Screenshot_6_7_13_10_45_AM.png

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.

larowlan’s picture

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

larowlan’s picture

Assigned:larowlan» Unassigned
Status:Active» Needs review
StatusFileSize
new3.57 KB
PASSED: [[SimpleTest]]: [MySQL] 55,585 pass(es).
[ View ]
new2.2 KB
FAILED: [[SimpleTest]]: [MySQL] 55,483 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Fail and pass

Gábor Hojtsy’s picture

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.

larowlan’s picture

StatusFileSize
new3.64 KB
PASSED: [[SimpleTest]]: [MySQL] 55,469 pass(es).
[ View ]
new1.85 KB

Fixes issues at #10

andypost’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs issue summary update

Nice fix with test, +1 rtbc

alexpott’s picture

+++ 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.'),
    );
  }
alexpott’s picture

Status:Reviewed & tested by the community» Needs work
kim.pepper’s picture

Status:Needs work» Needs review
StatusFileSize
new943 bytes
new3.63 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Changes in #13

alexpott’s picture

+++ 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...

kim.pepper’s picture

StatusFileSize
new1.98 KB
new4.15 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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

kim.pepper’s picture

StatusFileSize
new1.48 KB
new4.1 KB
PASSED: [[SimpleTest]]: [MySQL] 55,524 pass(es).
[ View ]

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.

andypost’s picture

Issue tags:+accessibility, +D8MI, +Twig

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

kim.pepper’s picture

Status:Needs review» Needs work

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()?

larowlan’s picture

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.

larowlan’s picture

StatusFileSize
new2.59 KB

Something like so?

larowlan’s picture

Status:Needs work» Needs review
StatusFileSize
new5.31 KB
PASSED: [[SimpleTest]]: [MySQL] 55,604 pass(es).
[ View ]
new2.58 KB

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

andypost’s picture

Status:Needs review» Reviewed & tested by the community

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!

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Yay!

Committed a74a17a and pushed to 8.x. Thanks!

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

Anonymous’s picture

Issue summary:View changes

Added related issue.