Current implementation of Drupal\panels\Plugin\DisplayBuilder\StandardDisplayBuilder::buildRegions() has a wrong condition which could lead to TypeError (null instead of expected array) in \Drupal\Core\Render\Element::isEmpty().

Example:

          $content = $block->build();
          if ($content !== NULL && !Element::isEmpty($content)) {
            foreach (['#attributes', '#contextual_links'] as $property) {
              if (isset($content[$property])) {
                $block_render_array[$property] += $content[$property];
                unset($content[$property]);
              }
            }
          }

          // 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.
          if (Element::isEmpty($content)) {

The Element::isEmpty($content) on second execution fails with fatal error when $content contains null.

Steps to reproduce described in #2819219: Fatal error when "setMainContent()" method is not called (block module not installed).

@capuleto #8:

This patch also makes Drupal\Core\Render\Element::children() trigger several warnings like

"max-age" is an invalid render array key

@Devin Carlson #9:

This also doesn't check if $content !== NULL, as is done earlier in the function, which causes a fatal error if $content is NULL.

Recoverable fatal error: Argument 1 passed to Drupal\Core\Render\Element::isEmpty() must be of the type array, null given called in panels/src/Plugin/DisplayBuilder/StandardDisplayBuilder.php on line 136.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

lauriii created an issue. See original summary.

lauriii’s picture

Status: Active » Needs review
FileSize
2.83 KB

This should fix both of the issues.

We are now throwing a LogicException if the block builder returns something else than an array. The interface doesn't allow them to return anything else so it seems like a good way to get people fix their code.

tobiberlin’s picture

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

lauriii’s picture

@tobiberlin I have tested the original patch with Views block and I cannot reproduce the patch. Are you sure that the View is fully empty?

tobiberlin’s picture

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

lauriii’s picture

trwill’s picture

Status: Needs review » Reviewed & tested by the community

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.

BR0kEN’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.37 KB

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

BR0kEN’s picture

Title: Followup for #2781897 » Incorrect logic inside of StandardDisplayBuilder could lead to fatal error (followup for #2781897)
Issue summary: View changes
lauriii’s picture

Status: Needs review » Needs work

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

The block builder is broken if in case we thrown an exception. If we don't throw an exception there is no way for people to know that their block builder is broken.

BR0kEN’s picture

Status: Needs work » Needs review

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.

lauriii’s picture

If core do not validate described, in PHPDoc comments, types then module must not do this as well.

I think that core should do this also. Also, we sort of do this for page controllers in Drupal core.

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.

That is not correct. It should be specified that it could return null if it's allowed to return null. See also PHP7 documentation for return type declarations: https://wiki.php.net/rfc/return_types#disallowing_null_on_return_types

I'd let the maintainers decide what is the best approach for this.

BR0kEN’s picture

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.

Xano’s picture

Status: Needs review » Needs work

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.

lauriii’s picture

Status: Needs work » Needs review
FileSize
2.83 KB

Re-uploading patch from #2 based on #14.

Xano’s picture

  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.

lauriii’s picture

#16.1: Fixed
#16.2: Fixed
#16.3: Definitely not. The original code would cause fatal error.
#16.4: The original code is wrong because the render array should be left fully empty.

Xano’s picture

  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?

BR0kEN’s picture

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.

BR0kEN’s picture

BR0kEN’s picture

  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.

sk33lz’s picture

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

jonathanshaw’s picture

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

Xano’s picture

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.

BR0kEN’s picture

Status: Needs review » Reviewed & tested by the community

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

Sebastien @Actualys’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
5.24 KB
724 bytes

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’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
5.17 KB

Sebastien @Actualys: Please see the discussion above. This is the wanted behavior. There's bug in core which needs to be fixed also: #2819219: Fatal error when "setMainContent()" method is not called (block module not installed). You should help that issue move forward instead of making changes for this patch.

I'm re-uploading patch from #19 which was RTBC.

BR0kEN’s picture

@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)) {
BR0kEN’s picture

lauriii’s picture

@BR0kEN The committer can decide the author of the commit and that's why I said I'm re-posting another patch. I was also already credited for this issue since I've posted patches myself so it didn't affect the credits. The reason for re-posting the patch is to avoid any possibility of a wrong patch being committed. :)

BR0kEN’s picture

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

Sebastien @Actualys’s picture

Sorry for making confusion.

joelpittet’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

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?

BR0kEN’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
5.21 KB
Loparev’s picture

Works like a charm

Loparev’s picture

Status: Needs review » Reviewed & tested by the community
welly’s picture

Status: Reviewed & tested by the community » Needs work

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

BR0kEN’s picture

Status: Needs work » Reviewed & tested by the community

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

welly’s picture

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

Cheers.

koffer’s picture

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

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs tests
FileSize
1.08 KB

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.

hamrant’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
45.94 KB

Getting these errors every time the page is reloaded:
errors log

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

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

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work

This issue still needs test coverage.

arosboro’s picture

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.

Li Huang’s picture

Version: 8.x-3.x-dev » 8.x-4.x-dev
FileSize
4.64 KB

Reroll.

praveenkakumanu’s picture

Assigned: Unassigned » praveenkakumanu
praveenkakumanu’s picture

praveenkakumanu’s picture

jiv_e’s picture

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

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

BR0kEN’s picture