Deletion of view used by layout builder breaks page with custom layout built by layout builder.

Problem/Motivation

I did following.

  1. create some simple view block
  2. used it for layouting of single page (so configuration will be saved in field)
  3. delete view block
  4. open previously layouted page

And following exception is thrown:

TypeError: Argument 1 passed to Drupal\views\ViewExecutableFactory::get() must implement interface Drupal\views\ViewEntityInterface, null given, called in /var/www/html/core/modules/views/src/Plugin/Block/ViewsBlockBase.php on line 69 in Drupal\views\ViewExecutableFactory->get() (line 70 of core/modules/views/src/ViewExecutableFactory.php).
Drupal\views\ViewExecutableFactory->get(NULL) (Line: 69)
Drupal\views\Plugin\Block\ViewsBlockBase->__construct(Array, 'views_block:test-block_1', Array, Object, Object, Object) (Line: 84)

Proposed resolution

Maybe this is a bit conceptual problem with saving layout builder information as a serialized blob.

This is one case where the problem occurs, another possible case is when some module provides block and wants to update its configuration. How I see it, update hook should go over all layout fields -> load config -> check if there is a config for that particular block ->update it -> serialize again.

CommentFileSizeAuthor
#15 2965973-15.patch9.78 KBtedbow
#15 interdiff-12-15.txt0 bytestedbow
#15 2965973-15-TEST_ONLY.patch7.65 KBtedbow
#12 2965973-12.patch9.78 KBtedbow
#12 2965973-12-test_only.patch2.25 KBtedbow
#7 2965973-7.patch9.79 KBtedbow
#7 2965973-7_TEST_ONLY.patch7.66 KBtedbow
#5 2965973-4-delete_view_block.patch2.13 KBtedbow
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mtodor created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Priority: Normal » Major
Issue tags: +Layout Builder stable blocker

This issue is at least major for layout builder and blocks a stable release.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented

It seems like since basically we now have content that dependent on config then Layout Builder should handle missing plugins gracefully.

When a View block is deleted there could be thousands of nodes or other content entities that references this via serialized value.

Looking at how that would work.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented

I can't reproduce this. Instead, the views block derivative goes missing and I get this message where the view was located:

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

which is not as ideal as being able to find and remove the view. But it's way less broken than the IS implies.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
FileSize
2965973-4-delete_view_block.patch2.13 KB

One way to solve the hard error would be for Views to clear the block definitions cache in \Drupal\views\Plugin\views\display\Block::remove

Instead you would see

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

Which seems better.

But it would also solve another problem which I don't think has another issue

  1. Create view block
  2. Goto "Place block" on block page. You don't actually have to place the definitions just have to loaded/cached once.
  3. Delete the View or remove the block display.
  4. goto "Place block" see block in list.
  5. Click block to place
  6. Nothing happens
  7. No error because modals don't show errors but error in logs the same as in this issue.
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented

RE #4 I am able to reproduce problem in the IS.

I will try to write test that proves it.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Active » Needs review
FileSize
2965973-7_TEST_ONLY.patch7.66 KB
2965973-7.patch9.79 KB

Here is rough test to prove that it fails.

Probably don't need
$assert_session->pageTextNotContains('Argument 1 passed to Drupal\views\ViewExecutableFactory::get() must implement interface Drupal\views\ViewEntityInterface');
but this proves the particular error for the issue.

The last submitted patch, 7: 2965973-7_TEST_ONLY.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented

Nice! Also thought about #2966969: Trigger a warning whenever the "Broken" block plugin is instantiated

lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya at Babson College commented

What is the step is to reproduce the error manually? I also just see:

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

The test shows the error, but I can't see what's different about the automated test.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented

On my local, I have all caching disabled, which prevented me from reproducing since the fix is to clear caches at the right time :)

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
FileSize
2965973-12-test_only.patch2.25 KB
2965973-12.patch9.78 KB

Cleaning up the test.

The last submitted patch, 12: 2965973-12-test_only.patch, failed testing. View results

lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya at Babson College commented

Yes, after turning on cache, I was able to reproduce the error.

+++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
@@ -349,4 +353,40 @@ public function testLayoutBuilderChooseBlocksAlter() {
+    // Node can be loaded after deleting the View.
+    $assert_session->pageTextContains(Node::load(1)->getTitle());

I like the updated test better because it is more durable; it doesn't rely on the error message. Although the error message was helpful in the rough test for proving the problem.

  • The view should have been part of the test_only patch.
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
FileSize
2965973-15-TEST_ONLY.patch7.65 KB
interdiff-12-15.txt0 bytes
2965973-15.patch9.78 KB

The view should have been part of the test_only patch.

@lokapujya thanks for catching that adding a new test only patch.

  1. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
@@ -349,4 +353,40 @@ public function testLayoutBuilderChooseBlocksAlter() {
+      'administer node fields',

    This permission is not needed so removing 

  2. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
@@ -349,4 +353,40 @@ public function testLayoutBuilderChooseBlocksAlter() {
+    $this->drupalGet('node/1');
+    // Node can be loaded after deleting the View.
+    $assert_session->pageTextContains(Node::load(1)->getTitle());

    Adding an assertion at the end to confirm the View is not on the page.

The last submitted patch, 15: 2965973-15-TEST_ONLY.patch, failed testing. View results

lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya at Babson College commented
Status: Needs review » Reviewed & tested by the community

  • larowlan committed ce1f1fc on 8.6.x 
    Issue #2965973 by tedbow: Deletion of View Block used by Layout Builder
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Fixed

As this adds a new argument to the constructor of the views Block display plugin. I think it will be disruptive, I have sub-classed that plugin in custom projects a few times.

So I think that makes this 8.6 only, especially given it only surfaces in layout builder (experimental) and it can be resolved with a cache clear

Committed as ce1f1fc and pushed to 8.6.x.

Status: Fixed » Closed (fixed)

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

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Issue tags: +Blocks-Layouts

Retroactively tagging