When a views block returns no results a title and empty block body are still rendered.

This is a regression from behavior in views 3.x and 2.x in Drupal 7.

This behavior is often used by themers to avoid showing extra theme elements when no content is available.

Steps to reproduce:
- Enable 'Recent comments' block in the content area on a fresh Drupal 8 'standard profile' site with no comment content.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Sounds a bit like a general bug of the block subsystem.

This is the code in D7, which handled that:


  function execute() {
    // Prior to this being called, the $view should already be set to this
    // display, and arguments should be set on the view.
    $info['content'] = $this->view->render();
    $info['subject'] = filter_xss_admin($this->view->get_title());
    if (!empty($this->view->result) || $this->get_option('empty') || !empty($this->view->style_plugin->definition['even empty'])) {
      return $info;
    }
  }

This is the code in Drupal 8 (\Drupal\views\Plugin\views\display\Block::execute):

  public function execute() {
    // Prior to this being called, the $view should already be set to this
    // display, and arguments should be set on the view.
    $element = $this->view->render();
    if ($this->outputIsEmpty() && $this->getOption('block_hide_empty') && empty($this->view->style_plugin->definition['even empty'])) {
      return array();
    }
    else {
      return $element;
    }
  }

Jaesin’s picture

Part of the problem is that view execution is delayed.

(Drupal\views\Plugin\Block\ViewsBlock->build()) needs to return "array()" if there is nothing to display but since it returns a themeable array for the view (plus a title and contextual links), "execute()" is called after the fact.

  public function build() {
    $this->view->display_handler->preBlockBuild($this);

    if ($output = $this->view->buildRenderable($this->displayID)) {
      // Override the label to the dynamic title configured in the view.
      if (empty($this->configuration['views_label']) && $this->view->getTitle()) {
        $output['#title'] = Xss::filterAdmin($this->view->getTitle());
      }

      // Before returning the block output, convert it to a renderable array
      // with contextual links.
      $this->addContextualLinks($output);
      return $output;
    }

    return array();
  }

Is there a good reason views execution is delayed?

dawehner’s picture

Good catch, indeed the view is executed as late as possible in order to have render caching going on.

*sigh*

Wim Leers’s picture

Title: Block displayed when no results returned » Region displayed when the blocks it contains are rendered to the empty string
Component: views.module » block.module
Priority: Normal » Major

IIRC this is not related to render caching, but to the way the block system has changed — even before render caching was in the picture. I ran into this with the menu block while working on #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees. Basically, if you want to ensure the region HTML (some wrapper DIV e.g.) is not printed, then your block must return FALSE in its access() method.

This is a problem in HEAD too for any block that renders to the empty block. Try putting the "Tools" menu block in the second sidebar. Look at it as the auth user, confirm it appears there. And look at the region HTML. Now load the same page as the anonymous user. Notice that there is no Tools menu block, but the region is there.

This is a general problem.

And IIRC it's even related to the whole "how can templates check if its children are empty?" problem, which has been a thorn for well over a year.

Wim Leers’s picture

emmamaria to the rescue — she immediately knew which one I meant!

Upchuk’s picture

Title: Region displayed when the blocks it contains are rendered to the empty string » Views block displayed when no results returned

Changing back title to reflect the actual issue at hand.

Strongly related to what Wim said in #4 / #5.

dawehner’s picture

IIRC this is not related to render caching, but to the way the block system has changed — even before render caching was in the picture. I ran into this with the menu block while working on #1805054: Cache localized, access filtered, URL resolved, (and rendered?) menu trees. Basically, if you want to ensure the region HTML (some wrapper DIV e.g.) is not printed, then your block must return FALSE in its access() method.

Mh, well, now we can't even return an empty render array anymore, given that we simply can't check it early enough.

Wim Leers’s picture

Title: Views block displayed when no results returned » [PP-1] Views block displayed when no results returned
Status: Active » Postponed

IMO this is simply yet another duplicate of #953034: [meta] Themes improperly check renderable arrays when determining visibility. But currently not marking as a duplicate yet, just to make sure this gets resolved. But #953034: [meta] Themes improperly check renderable arrays when determining visibility should definitely be fixed first, so postponing on that.

dawehner’s picture

@wim
Are you sure this is a strict duplicate? In views you can configure whether you want to hide an empty block.

Wim Leers’s picture

It's the same underlying problem, I'm sure. The underlying problem is that a "built" render array does not yet contain the children it *will* contain, because those are added only in #pre_render. See #129 & #131.

Jaesin’s picture

Just to throw up some mud here. Would you guys not agree that returning false in the access callback is semantically incorrect.

dawehner’s picture

Just to throw up some mud here. Would you guys not agree that returning false in the access callback is semantically incorrect.

You could certainly do that for custom code, no question, but its too much overhead for the actual normal case.

Berdir’s picture

Title: [PP-1] Views block displayed when no results returned » Views block displayed when no results returned
Status: Postponed » Active

I don't think this should be postponed on the other issue, fairly sure that's not the same problem and bigger than that.

The problem here is using pre_render. So it's no longer possible to filter out a block without content while rendering them.

*Somehow* we need to detect those kinds of blocks and even bubble that fact up. If this block is in a region that only has that block then we also need to bubble that up and don't show the whole region.

Element::isEmpty() doesn't really help here or at least is not enough because that will still return FALSE for the block and even more for the region.

The only thing I can think of right now is to set a special flag that's set on block render arrays in that case and some sort of callback that runs on the region *after* loading the possibly render-cached blocks that throws out those blocks and then also ensures that the region is considered empty if nothing is left. Which in turn might help #953034: [meta] Themes improperly check renderable arrays when determining visibility, but I think we actually need to start here, not in the other issue.

Wim Leers’s picture

Status: Active » Needs review
FileSize
5.1 KB

So there are a few problems here:

  1. The STR in the IS are wrong (Enable 'Recent comments' block in the content area on a fresh Drupal 8 'standard profile' site with no comment content), because that ships with default "empty results text". So, you want to remove that and also change the Hide block if the view output is empty setting from "No" to "Yes". Only then, we can reasonably expect this block to not be shown at all.
  2. But, this is just a simple distraction. The real problem is elsewhere.
  3. Look at \Drupal\block\BlockViewBuilder::buildBlock(). It checks whether the block content is empty. This is where things matter, for all blocks, so also for Views blocks.
  4. Note that menu blocks whose links are all inaccessible to the current user do not show up. So it works for menu blocks. And NOT anymore using the crazy technique in #4, that used to be the case until before #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees landed.
    No, instead, take a look at MenuLinkTree::build() (which generates the render array for SystemMenuBlock::build()). You can see there that it does all calculations right then and there, to determine the final render array. So that if no links are accessible, all that is returned for the block content is ['#cache' => …], i.e. a render array with only cacheability metadata.
    It now actually is okay to do the building of the final render array in your block's build() method, because all blocks are rendered lazily already anyway (step through it by setting a breakpoint in BlockViewBuilder::buildBlock(), you'll see that it's always called from a Twig template).
  5. So, to fix Views' blocks' "emptiness problem", we just need to fix ViewsBlock::build(): it should not return yet another layer of lazy rendering, but instead just provide the final render array. So, stop doing:
    if ($output = $this->view->buildRenderable($this->displayID, [], FALSE)) {
    

    Because that just returns a '#type' => 'view' render array with its own #pre_render callback. Instead, provide the final render array.

So, attached patch is a first stab at fixing that. It works, but uses some hacks for now to work around some bugs.

With this patch in place, the "Recent comments" block does NOT show up anymore when it shouldn't.

Wim Leers’s picture

FileSize
2.21 KB

Here's a vastly simplified version, that works generically, for any block. It just happens to fix Views' blocks as well.

dawehner’s picture

+++ b/core/modules/views/src/Element/View.php
@@ -88,7 +88,8 @@ public static function preRenderViewElement($element) {
+      if (empty($plugin_definition['returns_response']) && empty($plugin_definition['uses_hook_block'])) {

wait, so we never add views-element-container on there? This breaks contextual links, doens't it? This also sounds wrong to have to special case blocks here. There is also no docs explaining why we need that here

Wim Leers’s picture

Yeah I'm not at all sure about that. But that's how I can get tests to pass (AFAICT — because the tests only look at non-block Views) and fix the bug reported in the IS.

AFAICT it's actually impossible to have #theme_wrappers not be output if the thing they're supposed to be wrapping is empty. Perhaps we could fix that instead (i.e. fix #theme_wrappers, and not hack Views).

In any case, those #theme_wrappers being always output effectively means that the Views block NEVER renders to the empty string, therefore making it impossible to fix this issue. This is the temporary work-around I came up with. I'd very much welcome your help on fixing that :)

Status: Needs review » Needs work

The last submitted patch, 15: 2443457-15.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.17 KB
542 bytes

Tried this, doesn't seem to work yet.

One problem that I see is that the views template is called anyway by that render() call even if there are no rows. And that prints out an empty div wrapper and even if I add a check to prevent that, I still get twig debug output if that's enabled. So relying on the rendered markup doesn't seem very reliable?

What I tried now seems to be working for me but I'm sure needs more checks for empty headers and things like that. Also no idea if e.g a table with headers but no rows should be considered empty or not? ;) And of course it still relies on rendering but maybe isEmpty() should check access too? (it is empty if there are no accessible children)

Status: Needs review » Needs work

The last submitted patch, 19: 2443457-19.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.19 KB
580 bytes

This behaves as expected again with blocks with an empty text. Still seeing some test fails in my install profile that might be related to this.

Status: Needs review » Needs work

The last submitted patch, 21: 2443457-21.patch, failed testing.

dawehner’s picture

+++ b/core/modules/views/src/Element/View.php
@@ -88,10 +88,14 @@ class View extends RenderElement {
+      if (empty($plugin_definition['returns_response']) && empty($plugin_definition['uses_hook_block'])) {

It is really kinda sad that we have to hardcode it here, but there seems to be no way around that?

Berdir’s picture

Yes, also something there is not right. Somehow a combination of those changes ate my block titles in some cases, still trying to figure out why.

Also had a look at the test fails, but some seem a bit tricky, the render() call breaks some things, like render() calls in test that don't get any output then.

Berdir’s picture

So my problem that I mentioned above was that I'm doing something to the content render array structure in preprocess_block(), but the content was already rendered at that point, so my change had now effect. So that call changes the behavior, the same problem is also visible in BlockViewBuilder and possibly other tests, which are then again calling render() on it which doesn't return anything as it is already printd.

I still think that using render() is not an option for this, what I did for now is do it on a copied $content array that is then thrown away. That's obviously bad for performance, especially on views but I don't see another way right now.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
3.71 KB

Discussed with Berdir. The root cause of the problem here is the nested #pre_render: all blocks already are built with a #pre_render callback. i.e. the Block module is specifically designed to ensure that Block developers do not need to know about #pre_render. So, a block's build() method can just return the final render array. This has worked well so far.

But… Views is a very, very advanced module, and so applies a single rendering pattern to all of its output, no matter whether it's a Page display, an Attachment display, …, or a Block display.

So, the solution is actually fairly simple: ensure that Views' Block already runs the #pre_render as part of its BlockPluginInterface::build() method, so that it can return the final render array.

Berdir & dawehner: what do you think? :)

(No interdiff because new approach.)


  • Yes this is less than ideal, but so far it looks like only the Views module has to do this sort of advanced thing. If Views, the most complex module we have, has to do something special: I think that's acceptable.
  • I've also documented this explicitly on BlockPluginInterface::build(). (But I think it could be made clearer.)
Wim Leers’s picture

Status: Needs review » Needs work

The last submitted patch, 26: 2443457-26.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
3.74 KB
724 bytes

LOL.

Berdir’s picture

+++ b/core/lib/Drupal/Core/Block/BlockPluginInterface.php
@@ -63,6 +63,13 @@ public function access(AccountInterface $account, $return_as_object = FALSE);
+   * This must be the final renderable array. Otherwise Block module will not be
+   * able to detect whether this block has no content. (Blocks without content
+   * are not rendered, but if a non-final renderable array is returned — usually
+   * a render array that has #pre_render callbacks defined, to further build the
+   * render array — then Block module cannot determine whether the block has no
+   * content.)

I'm not sure if the concept of "final renderable array" really is a thing.

Isn't our requirement actually even simpler?

What we require is that if the block is empty, is that you return an empty renderable array (excluding cacheability metadata).

If you actually have content, then you can return #pre_render and #lazy_builders and whatever you heart desires. Nothing wrong with that.

Wim Leers’s picture

#30: excellent, excellent points. This is why I said I thought the docs could be made clearer :P

dawehner is writing a test for this right now, I'll address #30 after he posts that (unless he already does).

Status: Needs review » Needs work

The last submitted patch, 29: 2443457-28.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
5.94 KB
2.99 KB

Fixed the test failures.

dawehner’s picture

I promised to wrote tests for the evening

Wim Leers’s picture

Addressed #30.

Berdir’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

This looks good to me and works in my project. RTBC once we have a test. And thanks @dawehner in advance for writing them :)

Wim Leers’s picture

Wim Leers’s picture

Status: Needs review » Needs work

NW for #36.

dawehner’s picture

dawehner’s picture

Here is a test only patch.

Status: Needs review » Needs work

The last submitted patch, 40: 2443457-fail.patch, failed testing.

Berdir’s picture

Status: Needs work » Reviewed & tested by the community

Great, test looks good to me and passed/failed as expected.

Berdir’s picture

Component: block.module » views.module

I think this is just a views bug now, so moving.

alexpott’s picture

+++ b/core/modules/views/src/Plugin/Block/ViewsBlock.php
@@ -40,6 +41,20 @@ public function build() {
+      // When view_build is empty, the actual render array output for this View
+      // is going to be empty. In that case, return just #cache, so that the
+      // render system knows the reasons (cache contexts & tags) why this Views
+      // block is empty, and can cache it accordingly.
+      if (empty($output['view_build'])) {
+        $output = ['#cache' => $output['#cache']];
+      }

Do we have test coverage of this?

Berdir’s picture

Status: Reviewed & tested by the community » Needs work

We clearly don't. That code there is fine.. the corresponding logic in BlockViewBuilder, however, not so much....

$content = $build['#block_plugin']->build();
    // Remove the block plugin from the render array.
    unset($build['#block_plugin']);
    if ($content !== NULL && !Element::isEmpty($content)) {
      $build['content'] = $content;
    }
    else {
      // Abort rendering: render as the empty string and ensure this block is
      // render cached, so we can avoid the work of having to repeatedly
      // determine whether the block is empty. E.g. modifying or adding entities
      // could cause the block to no longer be empty.
      $build = [
        '#markup' => '',
        '#cache' => $build['#cache'],
      ];
    }

That, uhm, never merges cacheability metadata of $content, which is what the views block there returns?

While debugging this (which caused my behat tests to fail, turns out this didn't fix it after all in my project, just didn't test it properly), I also stumbled over the following code in DisplayPluginBase::render():


  /**
   * {@inheritdoc}
   */
  public function render() {
    $rows = (!empty($this->view->result) || $this->view->style_plugin->evenEmpty()) ? $this->view->style_plugin->render($this->view->result) : array();

    $element = array(
      '#theme' => $this->themeFunctions(),
      '#view' => $this->view,
      '#pre_render' => [[$this, 'elementPreRender']],
      '#rows' => $rows,
      // Assigned by reference so anything added in $element['#attached'] will
      // be available on the view.
      '#attached' => &$this->view->element['#attached'],
    );

    $this->applyDisplayCachablityMetadata($this->view->element);

    return $element;
  }

Note the discrepancy between $element and $this->view->element. Why would it do that, doesn't make sense to me. But it's unrelated, since $element is set to an empty array anyway later on.

Setting to needs work to decide what exactly we want to fix here. Arguably, this introduces the loss of cacheability of empty views blocks by fixing empty views blocks to actually be empty :( Since the fix is pretty easy, we might want to just fix it here. If not, we need to open a follow-up.

Berdir’s picture

Status: Needs work » Needs review

Forget that, I was confused, BlockViewBuilder does have that. I was looking at the corresponding code in page_manager that needs it too.

Still, I think we should have explicit test coverage for that.

dawehner’s picture

Let's expand the test coverage ...

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/block/src/Tests/Views/DisplayBlockTest.php
@@ -282,6 +287,10 @@ public function testBlockEmptyRendering() {
+    // Ensure that the view cachability metadata is propagated even, for an

Nit: comma can be removed, or should be moved to before the "even". Can be fixed on commit.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 47: 2443457-47.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

Wim Leers queued 47: 2443457-47.patch for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 47: 2443457-47.patch, failed testing.

Wim Leers’s picture

giancarlosotelo’s picture

Status: Needs work » Needs review
FileSize
2.42 KB
9.89 KB

This should works.

LKS90’s picture

Status: Needs review » Reviewed & tested by the community

Looks fine, the testbot likes it, I'd say good to go.

gaurav.goyal’s picture

Issue tags: -Needs reroll

Patch #54 cleanly applies. Removing the needs reroll tag.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 9b768d7 on 8.0.x
    Issue #2443457 by Wim Leers, dawehner, Berdir, giancarlosotelo, Jaesin:...

  • alexpott committed 31a65da on 8.0.x
    Issue #2561757 by Wim Leers, borisson_: Follow-up for #2443457: improve...

Status: Fixed » Closed (fixed)

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

Chi’s picture

Status: Closed (fixed) » Fixed

I just faced the same issue. I was able to reproduce the bug on simpletest.me with Drupal 8.0.1, RC1, Beta 15. Can anyone confirm that this is really fixed?

Berdir’s picture

Status: Fixed » Closed (fixed)

Yes, it is. There is an option somewhere in your view that you need to enable for this to work.

Chi’s picture

The option is "Hide block if the view output is empty".
It works fine. Thanks.

didaka’s picture

I have just faced this issue again in Drupal 8.0.2.

Steps to replicate on a boilerplate Drupal 8.0.2. with Bartik on simplytest.me:

  1. Create a view with a block that accepts nid as default argument from url.
  2. Add argument validation that hides the view. Basic validation will be enough.
  3. Set "Hide block if the view output is empty: Yes"
  4. Display the block in the empty sidebar_second (Left sidebar) on Bartik.
  5. Create an article node. The secondary sidebar should show the node title.
  6. Go to frontpage. The secondary sidebar is rendered, but empty. Body classes are also added.
didaka’s picture

Status: Closed (fixed) » Active
dawehner’s picture

Status: Active » Fixed

@didaka
Please don't reopen already fixed issues but rather add new one.
Regarding your bug, there is another issue which fixes the issue, but I cannot find it anymore at the moment :(

muash’s picture

same problem here as of #64

Status: Fixed » Closed (fixed)

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

afoster’s picture

I can confirm I'm having the same issue #64 in Drupal 8.0.5 - As requested by @dawehner I've opened a new issue https://www.drupal.org/node/2685157

3eidoz’s picture

I'm having the same issue in D8.0.5

diogo_plta’s picture

I'm having the same issue in Drupal 8.1.1 : (

diogo_plta’s picture

Version: 8.0.x-dev » 8.1.1
Status: Closed (fixed) » Active

I'm having the same issue in Drupal 8.1.1

Lendude’s picture

Status: Active » Fixed

Please leave this fixed, see #66.

A new issue has been opened for the problems described in #64, see #2685157: Empty region rendered when a Views block with no results and 'block_hide_empty' set is in the region. Please comment there if you still have issues.

Status: Fixed » Closed (fixed)

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

jwilson3’s picture

Just following up here that the current workaround until a solution to #953034: [meta] Themes improperly check renderable arrays when determining visibility lands is to render the output of the region the block is placed in to determine whether to render the region.

Eg. in page.html.twig in your theme:

{# render and strip out everything except images, iframes, objects and svgs that don't have any inner html #}
{% set has_sidebar_right = page.sidebar_right|render|striptags('<img><iframe><object><svg>')|trim|length %}

...

{% if has_sidebar_right %}
  {{ page.sidebar_right }}
{% endif %}

...