Undefined array key "view_mode" in block_content_theme_suggestions_block_alter

The attached patch currently applies to the 11.0.x and 10.3.x and 10.4.x branches.

The patch attached to this comment is a re-roll based on the MR. The original patch in #2 does not pass tests because it reorders the suggestions.

Possible to get test coverage around this issue?

Added a test. The test passes with the MR and fails when run using the "test only" pipeline, as desired.

Made a small change directly. Don't think we need to mention the ticket in the comment since git blame can show that. But did leave the comment that this is solving a php warning.

Test-only failure does indeed show coverage and rest of the changes appear fine.

Makes sense, thank you.

The steps to reproduce say:

Add a field block or extra field block to a layout builder enabled page. Then view the page.

But the function does this:

function block_content_theme_suggestions_block_alter(array &$suggestions, array $variables) {
  $suggestions_new = [];
  $content = $variables['elements']['content'];

  $block_content = $variables['elements']['content']['#block_content'] ?? NULL;

  if ($block_content instanceof BlockContentInterface) {

So it shouldn't run for field or extra field blocks at all, and I don't see how a block content block can be rendered without a view mode set.

The test coverage is quite low-level so doesn't confirm the steps to reproduce either. It's not clear to me how we end up in this situation, and if so, whether the test is fixing the right thing or not - if this alter is running for the wrong blocks, then that's the thing that should be changed.

Thanks for that analysis which is spot on. I've updated the 'steps to reproduce' to a much narrower use case, which is when the extra field builds a render array containing a block content entity. The layout_builder_entity_view_alter() function calls ExtraFieldBlock::replaceFieldPlaceholder() when it finds an extra field in its layout, and that function does a simple replacement of the content for the plugin. If that content contains a render array generated by a call like \Drupal::entityTypeManager()->getViewBuilder('block_content')->view($block_content_entiy, 'default'), the block content suggestions logic will trigger this issue.

Creating a test that instantiates the above would be doable but non-trivial, and more work than I have time for today. I'll put this back into "needs review" to get your thoughts on whether that complicated test is worth it. My thought is that the suggestions logic should be defensive in its approach, and the existing test covers that, but put this back into "needs work" if you feel the complicated test is a requirement to proceed.

I am also seeing this issue when using the Fixed Block Content → contrib module.

The module renders a block_content entity directly as its only content, the same situation as described by OP. This situation does seem perfectly reasonable to me. There's any number of reasons why a block, e.g. a custom block, might only directly render a block_content entity as its content.

MR !9426 attempts to resolve this issue by looking to $variables['elements']['content']['#view_mode'] to determine the view mode of the block_content entity rather than $variables['elements']['#configuration']['view_mode'].

Hide patches since we are now using MRs.

There are now 2 MRs up with different approaches and 1 not having test coverage.

john.oltman → closed merge request !9232

I can confirm that the approach in MR 9426 from @sonfd handles my use case as well as regular block plugins, so I closed my MR and updated the existing test so MR 9426 passes.

The existing test is low level as @catch points out, and ideally there would be a more complex test that instantiates various blocks in a more realistic way, and then checks the suggestions. In this way one could see that the suggestions for the standard block plugin case are the same as they were and no regression is introduced by the MR, and the extra field case is handled in the MR but wasn't previously. I'll try and work on this additional test, so for now leaving this issue in "needs work".

Added tests and moving to "needs review". The TEST ONLY failed, as expected, while the MR passes. Portions of the new tests are modeled after the suggestions testing in the core layout_builder module.

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.

False positive

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.

MR updated + missing type hints.

Believe feedback on this one has been addressed.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. 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.

@smustgrave should the hooks in the test module be converted to the new Hook system as well - I can do that if needed

Yes, they should be, but wow do you guys get an award for confusing module / hook names.

At first glance I thought block_content_theme_suggestions_test_entity_extra_field_info was theme suggestion until I saw the end.

Theme suggestions are not converted yet, but to be clear all three hooks in this module should be able to be converted.

The MR has been updated, moving hooks in the test module from procedural to OOP. Ready for review.

Refactor for move to hooks seems good.