Incorrect logic inside of StandardDisplayBuilder could lead to fatal error (followup for #2781897)

This still is not a solution for views generated blocks as I described in #2781897

It's a mess... it seems to be a more general problem with blocks/ render arrays. The point is that I use a views block with context filter: term id from url as I placed the block in a panel used for term pages. See #2443457: Views block displayed when no results returned and the following #953034: [meta] Themes improperly check renderable arrays when determining visibility - quite strange

Reviewed and tested. Patch works well and does fix render array errors for empty blocks, including empty view blocks, as I posted in https://www.drupal.org/node/2812617 (which was closed as duplicate of this issue). Setting to RTBC.

This patch is nicer because it will not throw any exceptions and will not lead to broken site, because the exception is not caught.

For instance, one of blocks from core can break the things. Read more here: #2819219: Fatal error when "setMainContent()" method is not called (block module not installed).

If core do not validate described, in PHPDoc comments, types then module must not do this as well. Did you read my comment from referenced issue? I don't want to get broken site and a state which is not depends on my code. By the way, null - is more or less acceptable replacer for array which could mean that result is empty (does not contain nothing). That's how it works, for instance, as default value for typed arguments for methods/functions.

Fair point. Let them decide. We can interpret situation in both ways: your and mine. My only concern to not break site in any case.

null is never an acceptable value if that value is documented as array. One could discuss nullable types, but an empty list MUST still be a list. In our case, we document the return value as array, so any code not following that is wrong and MUST be fixed by a developer. If this breaks site, go fix the root cause of the problem, or support someone else in doing so.

@BR0kEN: Your patch unfortunately still allows sites to be broken, as it does nothing to help fix the root cause of the problem. Instead, it just tries not to fail in this one particular case.

The approach in #2 is an excellent one, as it fails as early as possible, and clearly instructs developers how to fix the root problem (useful for other usages of BlockPluginInterface as well, as opposed to #8). I do propose not to use the plugin ID, but the class and method name instead, and perhaps reference the interface as well, because that's the documentation we are talking about.

1. +++ b/src/Plugin/DisplayBuilder/StandardDisplayBuilder.php
   @@ -117,33 +118,36 @@ class StandardDisplayBuilder extends DisplayBuilderBase implements PluginWizardI
   +            throw new \LogicException(new FormattableMarkup('Block %block build method has to return an array.', ['%block' => $block_id]));
   

   Showing the class and method name, with potentially a reference to the interface method (which comes with the type documentation) makes it much easier to find the code that needs to be fixed.

   Why is this an object? As exceptions are development tools, we usually just use plain strings for their messages.

2. +++ b/src/Plugin/DisplayBuilder/StandardDisplayBuilder.php
   @@ -117,33 +118,36 @@ class StandardDisplayBuilder extends DisplayBuilderBase implements PluginWizardI
   -          // If the block is empty, instead of trying to render the block
   -          // correctly return just #cache, so that the render system knows the
   -          // reasons (cache contexts & tags) why this block is empty.
   

   What is the reason this comment was moved away from the code it documents?

3. +++ b/src/Plugin/DisplayBuilder/StandardDisplayBuilder.php
   @@ -117,33 +118,36 @@ class StandardDisplayBuilder extends DisplayBuilderBase implements PluginWizardI
   +              $block_render_array['#cache'] = $content['#cache'];
   

   Out of scope?

4. +++ b/src/Plugin/DisplayBuilder/StandardDisplayBuilder.php
   @@ -117,33 +118,36 @@ class StandardDisplayBuilder extends DisplayBuilderBase implements PluginWizardI
   -          $block_render_array['content'] = $content;
   

   Out of scope?

#3 reports that SystemMainBlock is compromised. I'm not entirely sure why the tests do not catch this (maybe ::setMainContent() is called always in core), but it may be worth fixing this by giving the ::$mainContent property an empty array for its default value as it serves as an example of how to fix the root cause of this problem when people analyze the diff for this issue in the future.

1. +++ b/src/Plugin/DisplayBuilder/StandardDisplayBuilder.php
   @@ -117,32 +118,36 @@ class StandardDisplayBuilder extends DisplayBuilderBase implements PluginWizardI
   +            throw new \LogicException(sprintf('BlockPluginInterface::build method has to return an array. Class returning non-array value is: %s', $class));
   

   Great! If we use BlockPluginInterface::class, we get the full namespace as well, and easy refactorability.

2. +++ b/src/Plugin/DisplayBuilder/StandardDisplayBuilder.php
   @@ -117,32 +118,36 @@ class StandardDisplayBuilder extends DisplayBuilderBase implements PluginWizardI
   +          // @see \Drupal\block\BlockViewBuilder::preRender()
   

   Docs++.

Thanks for explaining why those other lines were not out of scope! Do or don't we want to update SystemMainBlock as an example fix?

Okay, @Xano, there is some sense in your words from #14. But very bad when core is in use almost year and has issues like this. I'll add a separate patch, based on @lauriii's approach, and make a review of changes from it.

1. +++ b/src/Plugin/DisplayBuilder/StandardDisplayBuilder.php
   @@ -2,19 +2,15 @@
   -use Drupal\Component\Render\FormattableMarkup;
   

   Removed since not used, as the similar uses below.

2. +++ b/src/Plugin/DisplayBuilder/StandardDisplayBuilder.php
   @@ -2,19 +2,15 @@
   +use Drupal\Core\Block\BlockPluginInterface;
   

   Added, to not use class name as string because, potentially, namespace could be changed.

3. +++ b/src/Plugin/DisplayBuilder/StandardDisplayBuilder.php
   @@ -91,37 +87,31 @@
   +    /** @var \Drupal\Core\Block\BlockPluginInterface[] $blocks */
   

   Moved here, to let developers know about the type where variable appears.

4. +++ b/src/Plugin/DisplayBuilder/StandardDisplayBuilder.php
   @@ -137,6 +127,17 @@
   +            $block_render_array = [
   

   No need to call methods of block instance because, if build result will be empty, the $block_render_array variable will be set to empty array and all above methods execution will be redundant.

#20 resolves the errors for me. It also applies cleanly to 3.0-beta5.

Have we identified a core issue to fix the doc comment?

Would it not break backwards compatibility if core now changed what it actually returned? And therefore isn't it necessary in reality for contrib to be able to handle the null?

#20 works for me, fixes the issue with an empty views block that uses the setting "Hide block if the view output is empty".

Would it not break backwards compatibility if core now changed what it actually returned? And therefore isn't it necessary in reality for contrib to be able to handle the null?

Core documents it returns arrays, but not all implementations do so, as they return NULL instead. To make debugging easier, the patch throws an exception if no array is returned. The actual fix is that the patch no longer makes the code return NULL, but an array instead.

Set it to RTBC after two positive comments. Feel free to change if needed.

Hi all,

Once this #20 patch applied I still have an issue when $content = $block->build(); is null.

First I tried to add a check to avoid throwing exception if null, but later a new error is thrown due to the Element::isEmpty function which requires an array as the first parameter.

Here is my interdiff patch:

        if ($block->access($this->account)) {
          $content = $block->build();
+
+          if (is_null($content)) {
+            $content = [];
+          }

          if (!is_array($content)) {
            throw new \LogicException(sprintf(
              '%s::build() method has to return an array. Class returning non-array value is: %s',
              BlockPluginInterface::class,
              get_class($block)
            ));
          }

BR

@lauriii, and for what you've re-uploaded it? To be the author of commit? It's funny.

@Sebastien @Actualys, don't you see anything strange in the lines below (they're from your patch)?

+          if (is_null($content)) {
+            $content = [];
+          }
+
           if (!is_array($content)) {

@lauriii, you can just show/hide patches as I did. No need to re-upload the same because it's could confuse people.

Sorry for making confusion.

This patch no longer applies against 3.x-dev

patching file src/Plugin/DisplayBuilder/StandardDisplayBuilder.php
Hunk #1 succeeded at 7 (offset 4 lines).
Hunk #2 FAILED at 91.
1 out of 2 hunks FAILED -- saving rejects to file src/Plugin/DisplayBuilder/StandardDisplayBuilder.php.rej

Can someone re-roll against 8.x-3.x?

Works like a charm

It's not working for me. The initial error I was getting was:

TypeError: Argument 1 passed to Drupal\Core\Render\Element::isEmpty() must be of the type array, null given, called in /var/www/drupalvm/web/modules/contrib/panels/src/Plugin/DisplayBuilder/StandardDisplayBuilder.php on line 140 in Drupal\Core\Render\Element::isEmpty()

I applied the patch at https://www.drupal.org/node/2812721#comment-11952030 and now I get:

LogicException: Drupal\Core\Block\BlockPluginInterface::build() method has to return an array. Class returning non-array value is: Drupal\system\Plugin\Block\SystemMainBlock in Drupal\panels\Plugin\DisplayBuilder\StandardDisplayBuilder->buildRegions() (line 110 of /var/www/drupalvm/web/modules/contrib/panels/src/Plugin/DisplayBuilder/StandardDisplayBuilder.php).

@welly, I know that it's almost impossibly to read whole thread by please, try to start from the #27.

Thanks @BR0kEN, appreciate you pointing me to that comment which I missed. Not sure snide comments are called for or needed though.

Cheers.

A site with drupal 8.3 broke with this error

Recoverable fatal error: Argument 1 passed to Drupal\Core\Render\Element::isEmpty() must be of the type array, null given, called in /Applications/MAMP/htdocs/escalaexp/modules/panels/src/Plugin/Dis$
#1 /Applications/MAMP/htdocs/escalaexp/core/lib/Drupal/Core/Render/Element.php(198): _drupal_error_handler(4096, 'Argument 1 pass...', '/Applications/M...', 198, Array)
#2 /Applications/MAMP/htdocs/escalaexp/modules/panels/src/Plugin/DisplayBuilder/StandardDisplayBuilder.php(136): Drupal\Core\Render\Element::isEmpty(NULL)
#3 /Applications/MAMP/htdocs/escalaexp/modules/panels/src/Plugin/DisplayBuilder/StandardDisplayBuilder.php(162): Drupal\panels\Plugin\DisplayBuilder\StandardDisplayBuilder->buildRegions(Array, Array)
#4 /Applications/MAMP/htdocs/escalaexp/modules/panels/src/Plugin/DisplayVariant/PanelsDisplayVariant.php(329): Drupal\panels\Plugin\DisplayBuilder\StandardDisplayBuilder->build(Object(Drupal\panels\Plugin\DisplayVariant\PanelsDisplayVa$
#5 /Applications/MAMP/htdocs/escalaexp/modules/page_manager/src/Entity/PageVariantViewBuilder.php(34): Drupal\panels\Plugin\DisplayVariant\PanelsDisplayVariant->build()
#6 /Applications/MAMP/htdocs/escalaexp/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php(96): Drupal\page_manager\Entity\PageVariantViewBuilder->view(Object(Drupal\page_manager\Entity\PageVariant), 'full')

There is no reason to be calling CacheableMetadata::createFromObject($block_render_array);

1) It is not an object, it is an array.
2) It is an empty array that we create on the line directly above this.

#2825497: SystemMainBlock only functions when used by block.module is a separate issue, and while it is an easy way to reproduce this bug, is separate.

Here is my proposed fix. No interdiff since it is from scratch.
This also needs test coverage.

Getting these errors every time the page is reloaded:

Tried the patch from #41 but it didn't help.

Patch from #34 fixed problem, thanks @BR0kEN, great job!

This issue still needs test coverage.

Patch #34 re-directs this error by referencing to the block that didn't pass an array from its build method. Regarding the updated error message, in my case it was the system content block, because no content had been set by the HTMLRender class.

Reroll.

@arosboro, when the following fix is in core, the patch #45 should work also for empty SystemMainBlock.

https://www.drupal.org/node/2885370

@jiv_e, those won't be in core. See #2819219: Fatal error when "setMainContent()" method is not called (block module not installed).

thanks its working fine for me

protected $mainContent = [];

I could not go through all the comments. But #45 works for me. It is not fixed yet only due to add some test coverage or something else we are waiting for ? Can any one clearly mention what is going here and what to do next ?

Reroll.

Previous patch had a unclosed tag. Fixed it.

Is there a patch for 9.3x?

Simple reroll...