Log error + visual warning for missing or broken block

FWIW here's an 8.7 patch for the same flaw. These didnt apply for me.

Yes, I got this too. For some reason, the inline block was deleted but the reference to it is still on the page. No idea how that happened.

Here's a minor change that adds logging (without dependency injection). This needs tests and properly injected logger, but I just needed this quickly.

Keeps it in needs review to let tests work.

Patch replace logger static call by injection.

Does someone can describe steps to reproduce please ? This will help to write tests.

Replace logger service by logger.channel.layout_builder

I reproduced this by manually deleting the inline block in the database. Somehow that happened on a production site, but I'm not sure how. Only thing I can think of at the moment is that maybe two people were modifying a layout at the same time and one person deleted the block while the other kept it in the layout?

Isn't this masking potential data loss issues by not failing hard and explicitly? The underlying problem is that somehow an inline block was deleted when it shouldn't have been. Figuring out the root cause of why should probably be the first step.

Having this fail silently with a log message seems a bit dangerous, given in all future tests, the system as a whole will continue to work.

(core 8.7.7) – I configured a layout for a content type using layout builder on dev, exported the config to Git, imported it to a production site I'm currently building (along with core.extension.yml to enable the layout_builder modules), cleared the cache, and:

- the nodes were showing both the layout builder content and the default 'manage display'-style fields simultaneously
- on clicking the 'Manage Layout' button in Manage Display, I got the InlineBlock.php:build() method getEntityTypeId() error above - NB: it's now line 223

I was able to disable layout builder, so I did that.

Then, I tried reimporting the dev synchronised config a second time.
Now the node displays as intended, but the error when trying to access the layout builder itself still persists.

Anyone else had this / suggest a solution or anything obvious I'm missing?

…now I've had time to test:

(a) the latest patch (#11) applies and fixes (bypasses) the problem ( @bkosborne / Goz - thanks for adding the logging )
(b) it was my own fault for forgetting during the process I'd actually created a custom block, which I hadn't exported (and of course blocks, along with taxonomies and menus, don't get synced).

re: #13, personally I find the patch preferable, given that it at least allows you to view the Layout Builder UI, and that says "Placeholder" where the block content would be, so you can see what's wrong (maybe there's a case for using CSS to make that more prominent).

NW for tests

This is how I get the error:
- Created a page using Page Manager and Layout Builder (Required patch: #2960739)
- Add inline blocks to the page
- Exported config
- Pushed changes to production/staging
- Import config
- Navigate to the edit layout page and you will see the error (Because the block doesn't exist)

This patch fixed the issue for me.

Confirmed patch working on 8.9.1.

Reroll patch needed as not apply on 9.1.x

Reroll patch created, Please review.

@Sam152 #13:
> Having this fail silently with a log message seems a bit dangerous, given in all future tests, the system as a whole will continue to work.

I strongly agree with this. Rerolled the patch to output a visible error message rather than a silent log message.

Setting NR to trigger bot, but still needs tests.

NW for tests.

Nope, #23 was the wrong patch. Here we go.

Tested it manually, the last patch was missing the t(). Sorry for the noise.

Tested manually and worksforme.
Note that, apart from the error message, the block is shown as "Placeholder for the 'Foo' block".

IMHO the best would be if we show the block in error-red with text "'Foo' block (broken)".

NW for deciding this & automated tests.

Thank you for the patch
It's working with default content

I can create this very easily without a cross env config sync.

• site builder creates global template for basic page - "You are editing the layout template for all Basic page content items."
• sets up the layout to have any number of predefined blocks in it.
• user creates a bunch of nodes from this template
• site builder then deletes any number of the predefined blocks from the original template
• any node that was created while those blocks were present can no longer be edited by clicking the "Layout" tab. Error occurs.

I basically broke it within like 4 hours of playing with the builder. :) whoops.

I applied the patch in #26 and it's working very nicely. Thank you for the patch. The error is obvious.

Let's hope #29 is enough to write some tests from.

+++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlock.php
@@ -215,6 +215,10 @@ protected function blockAccess(AccountInterface $account) {
     $block = $this->getEntity();
+    if (!$block) {

As currently documented, InlineBlock::getEntity() is always supposed to return a block. But RevisionableStorageInterface::loadRevision() can return a NULL, and this code doesn't account for it.

So getEntity() needs to be adjusted to either:

1. allow for returning NULL (in which case all callers must account for that)
2. throw an exception when the block cannot be loaded
3. ALWAYS return a block (not sure that it's possible)

Aha! I was able to reproduce with one key addition:

1. site builder creates global template for basic page - "You are editing the layout template for all Basic page content items."
2. sets up the layout to have any number of predefined blocks in it.
3. user creates a bunch of nodes from this template
4. user clicks "Layout" tab on a node, but does not save changes
5. site builder then deletes any number of the predefined blocks from the original template
6. any node that was created while those blocks were present can no longer be edited by clicking the "Layout" tab. Error occurs.

Step 4 is the key here. It's the tempstore that is storing the old block revision ID.
Now we can write tests for this.

I still think the fix should shift to getEntity(), but in the meantime here's a test with the patch from #26.

I'm also not sure about the logger vs message part. Now that we have steps to reproduce, this isn't as mysterious as it was (and definitely not a data loss problem).
I think a middle ground would be to mimic Broken block by returning a render array with the broken message, and logging it.

In Slack, there was a discussion of the 3 options in the issue summary.
@jibran pointed to #3007424: Multiple usages of FieldPluginBase::getEntity do not check for NULL, leading to WSOD which is essentially the same problem.

So, conforming to that approach (aka #1 from the IS) but switching from a message to logging.

The same issue exists even without override layouts, when using configuration synchronization.

Steps to reproduce:

1. Create a new node type on dev/staging environment
2. Enable layout builder for this bundle (no override layouts)
3. Create a default layout with inline blocks
4. drush config:export, transfer config to live environment, drush config:import
5. visit /admin/structure/types/manage/NODE_TYPE/display/default/layout
6. WSOD due missing inline blocks, even without override layouts

Applied 3049332-inline-33-PASS.patch to Drupal 8.9.3 site. Overridden layout indeed had unsaved changes and a broken block ID.

Patch worked to get me past error message and able to edit and save page!

I have found similar types of issues with Layout Builder so will report back any additional blockers.

The fix and test look great.

+++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlock.php
@@ -81,14 +89,17 @@ class InlineBlock extends BlockBase implements ContainerFactoryPluginInterface,
+   * @param \Psr\Log\LoggerInterface $logger
+   *   A logger instance.
...
-  public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityTypeManagerInterface $entity_type_manager, EntityDisplayRepositoryInterface $entity_display_repository, AccountInterface $current_user) {
+  public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityTypeManagerInterface $entity_type_manager, EntityDisplayRepositoryInterface $entity_display_repository, LoggerInterface $logger, AccountInterface $current_user) {

This needs to be the last argument and trigger a deprecation notice like the $current_user argument... and then in D10 both with be required.

EDIT: The reason $logger was not added last is because $current_user was optional in D8 - because it was added and triggered a deprecation notice. So we need to do the same for this.

Confirmed Patch in #11 works on 8.9.6

Restoring version

I hope this is required here.

Rolled out #45 for Drupal 8.9

Whoops... flipped around parameters to address #42

Fixing tests for 8.9.x.

This is the interdiff, Forgot to upload.

Looks like this needs a re-roll for both 9.1.x and 9.2.x. First is a re-roll for 9.1.x. No need for an interdiff, since nothing was changed (I tried making one regardless though, and it failed for some reason).

And here is the re-roll for 9.2.x

Fixing tests for 9.2.x.

The patch #56 works fine on 9.2.8

Here's a re-rolled patch that should apply on 9.2.x, 9.3.x, and 9.4.x. I'm changing the target branch for this fix to 9.4.x.

This is really close to being done, but the feedback from #42 has not been fully addressed yet. I will follow with a patch that addresses that feedback after confirming that the re-roll passes.

Seeing as the re-roll applied and passed, I am adding a new patch that fully addresses the comments in #42. We have to move the optional parameter to the end of the list of arguments. Note that there is typically NOT any test coverage added fo these kinds of deprecations, so we don't need to add any test coverage.

My understanding is that we've missed the time for deprecating for removal in 10.0.0. I've updated the deprecation notice to reflect the timeline as I understand it.

I'll also unassign this because I suspect @tim.plunkett isn't actively working on it. :)

Patch #56 worked on 9.2.9. Thank you!

Patch #60 working on 9.3.6 as well. Thank you!!

Just in case anyone is looking for it... Here is a combination of the patch in #60, plus additions to make it work with 9.3.7 that is also patched with the synchronized layout override translations patch here.

Patch #60 fixed my issue. Drupal 9.3.9.

I haven't used Layout Builder much.

My issue was that I created and inserted a block on my local, exported config and pushed it to a remote but then couldn't edit the layout on the remote. Removing the block from my local, exporting the config again and pushing to the remote still resulted in a broken layout on the remote.

This patch allows me to edit the config on the remote.

#60 seems to be a very elegant solution here. It has most of the features that I'd hope for:

1. Prevents a WSOD :)
2. Provides a simple and user-friendly front-facing message.
3. Provides a more in depth diagnostic in the logs.

This solution even handles uncommon scenarios / irresponsible use cases like people deleting database rows directly.

One improvement that I'd love to see is for there to be some sort of classing applied to the broken block message. This would give themes the ability to:

1. Hide the broken block from anonymous users?
2. Add some sort of visual indication for staff that indicates "Hey!! Something isn't right here!!"

Thoughts?

Applied patch #60 fixed my issue Drupal 9.3

I have the following configuration:

Drupal: 9.3.12
PHP: 8.0.10
MySQL: 5.7.29

Webserver: Apache/2.4.38 (Debian)

I had the same issue

Error: Call to a member function getEntityTypeId() on null

and applied patch #60.

After that, I got another issue. I clicked on the 'Edit' button of the node and clicked on the 'Layout' tab. As soon as I clicked on the 'Layout' tab, I see the white blank page spinning for a long time.

I stop the loading page and checked the Drupal log message. The following two messages were generated:

1. Unable to load inline-block content entity with revision ID 1.
2. Recursive rendering detected when rendering entity user:2, using the uid field on the node:1 entity. Aborting rendering.

My Finding:
I had enabled the Revison option for the specific Content Type where layoutbuilder was implemented.
Once removed the Revison, it worked.

Though it was a temporary fix, we still need revision on the content type.

Appreciate your help.

re-roll of #63 for 9.5.x which is also patched with the synchronized layout override translations patch here.

Patch #60 works on Drupal 9.4.1. Thanks !

This patch doesn't seem to apply with the MR from #2942975: [PP-1] Expose Layout Builder data to REST and JSON:API.

Not managed to work out which lines are conflicting yet.

#60 is still the current patch. I have reviewed it. The fix looks sound, and has sufficient test coverage.

It still applies cleanly to 9.4.x up to 10.1.x. In the meantime 10.0.x and 10.1.x are in development so I started test rounds. What is missing is executing the tests on their own. We should see the original error message appearing in the tests. I am uploading the test-only version of patch #60. I have made no changes.

Test proves that an exception was being thrown (even though the test output doesn't include the exact exception), so the test is correctly detecting the failure.

1) Drupal\Tests\layout_builder\FunctionalJavascript\InlineBlockTest::testDeletion
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'Unable to load inline block content entity with revision ID %vid.'
+'%type: @message in %function (line %line of %file) @backtrace_string.'

The patch in #60 looks RTBC. Thanks everyone!

The deprecation message needs to be updated for 9.5.x since we won't change a constructor in 9.4.x and the issue summary needs to be updated to describe why logging is the correct solution.

The patch in #60 works for me on Drupal 9.4.7. Thank you!

The patch in #60 works for me on Drupal 9.5.5

Updated patch to move this forward. moved deprecation to 10 as the latest release this would go in. Updated deprecation to match standard format. Needs Change Record to link to in the deprecation.

I don't know how to address the logging concern in #75 to update the issue summary. Why wouldn't you log the an error? Without logging something it would be one of those unknoweable unfixable errors right?

Here's a small adjustment to #60 to allows admins to see the broken blocks, instead of suppressing the error "This block is broken or missing..."

Attaching patch and interdiff

Having this error on Layout builder of an entity in my case the taxonomy term page in Core 10.0.10, so this patch is against 10.0.10

Re-role for the patch from #82 to apply for Drupal 10.2.1

The issue is in
@trigger_error('The logger service must be passed to InlineBlock::__construct(). It was added in drupal:9.4.0 and will be required before drupal:11.0.0.', E_USER_DEPRECATED);

as it is not in correct format.
%thing% is deprecated in %deprecation-version% and is removed from %removal-version%. %extra-info%. See %cr-link%
https://www.drupal.org/about/core/policies/core-change-policies/drupal-deprecation-policy

Attempt tp fix PHPSTAN.

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

I created an MR with code from patch #87 as recommended by the "needs-review-queue-bot"

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Still getting the PHPCS error on trigger_error. However, I'm wondering if that trigger_error is needed. Is there an intention to make $logger required in a future Drupal version? If yes, do we need a Change Record to link to?

Correct yeah we would need a CR to be made for this as the logger is now required by InlineBlock

In my opininon, the trigger_error message should be formatted like existing examples. This example also includes a CR that could be used as a starting point.

I've updated the message and created... 🥁 my first ever change record (please be gentle).

Thanks to @DanielVeza for the helpful example. I tried to follow that model, although I couldn't find any good examples of the constructor being called. I instead opted to use [#3436810] as an example of a constructor change. I'm very much open to suggestions for better examples there.

Test failures seem to be unrelated:

    There was 1 failure:
    
    1)    Drupal\Tests\Composer\Plugin\Scaffold\Functional\ManageGitIgnoreTest::testUnmanagedGitIgnoreWhenGitNotAvailable

Hiding patches for clarity.

Left some comments on the MR.

Leaving issue summary tag if that could be cleaned up please. Proposed solution seems like a question and there are some TBD spots. If there aren't, for example, UI changes it's fine to just N/A there.

Thanks for the feedback @smustgrave. I've resolved the MR comments and taken a pass at updating the issue summary.

Thanks MR appears to have phpcs errors

I've satisfied phpcs, but it looks like Drupal\Tests\Composer\Plugin\Scaffold\Functional\ManageGitIgnoreTest is still failing.

Would recommend rebasing the branch.

Sorry... took a couple tries but I successfully rebased and tests are now passing.

Believe feedback has been addressed

This needs a title update so it makes sense in the git log.

Done.

I've asked a couple of questions on the MR.