Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#54 | 2443457-54.patch | 9.89 KB | giancarlosotelo |
#54 | interdiff-47-54.txt | 2.42 KB | giancarlosotelo |
#47 | interdiff.txt | 3.84 KB | dawehner |
#47 | 2443457-47.patch | 9.76 KB | dawehner |
#40 | 2443457-fail.patch | 2.08 KB | dawehner |
Comments
Comment #1
dawehnerSounds a bit like a general bug of the block subsystem.
This is the code in D7, which handled that:
This is the code in Drupal 8 (\Drupal\views\Plugin\views\display\Block::execute):
Comment #2
Jaesin CreditAttribution: Jaesin commentedPart 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.
Is there a good reason views execution is delayed?
Comment #3
dawehnerGood catch, indeed the view is executed as late as possible in order to have render caching going on.
*sigh*
Comment #4
Wim LeersIIRC 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.
Comment #5
Wim Leersemmamaria to the rescue — she immediately knew which one I meant!
Comment #6
Upchuk CreditAttribution: Upchuk commentedChanging back title to reflect the actual issue at hand.
Strongly related to what Wim said in #4 / #5.
Comment #7
dawehnerMh, well, now we can't even return an empty render array anymore, given that we simply can't check it early enough.
Comment #8
Wim LeersIMO 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.
Comment #9
dawehner@wim
Are you sure this is a strict duplicate? In views you can configure whether you want to hide an empty block.
Comment #10
Wim LeersIt'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.Comment #11
Jaesin CreditAttribution: Jaesin commentedJust to throw up some mud here. Would you guys not agree that returning false in the access callback is semantically incorrect.
Comment #12
dawehnerYou could certainly do that for custom code, no question, but its too much overhead for the actual normal case.
Comment #13
BerdirI 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.
Comment #14
Wim LeersSo there are a few problems here:
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.\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.No, instead, take a look at
MenuLinkTree::build()
(which generates the render array forSystemMenuBlock::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 inBlockViewBuilder::buildBlock()
, you'll see that it's always called from a Twig template).ViewsBlock::build()
: it should not return yet another layer of lazy rendering, but instead just provide the final render array. So, stop doing: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.
Comment #15
Wim LeersHere's a vastly simplified version, that works generically, for any block. It just happens to fix Views' blocks as well.
Comment #16
dawehnerwait, 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
Comment #17
Wim LeersYeah 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 :)Comment #19
BerdirTried 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)
Comment #21
BerdirThis 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.
Comment #23
dawehnerIt is really kinda sad that we have to hardcode it here, but there seems to be no way around that?
Comment #24
BerdirYes, 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.
Comment #25
BerdirSo 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.
Comment #26
Wim LeersDiscussed 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'sbuild()
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 itsBlockPluginInterface::build()
method, so that it can return the final render array.Berdir & dawehner: what do you think? :)
(No interdiff because new approach.)
BlockPluginInterface::build()
. (But I think it could be made clearer.)Comment #27
Wim Leers#26 incorporates #2549719: View::preRenderViewElement should use $element['view_build'] always too.
Comment #29
Wim LeersLOL.
Comment #30
BerdirI'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.
Comment #31
Wim Leers#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).
Comment #33
Wim LeersFixed the test failures.
Comment #34
dawehnerI promised to wrote tests for the evening
Comment #35
Wim LeersAddressed #30.
Comment #36
BerdirThis looks good to me and works in my project. RTBC once we have a test. And thanks @dawehner in advance for writing them :)
Comment #37
Wim Leers#2549719: View::preRenderViewElement should use $element['view_build'] always landed, so this needed a reroll. No interdiff, because it's just one less change this patch needs to make :)
Comment #38
Wim LeersNW for #36.
Comment #39
dawehnerThere we go.
Comment #40
dawehnerHere is a test only patch.
Comment #42
BerdirGreat, test looks good to me and passed/failed as expected.
Comment #43
BerdirI think this is just a views bug now, so moving.
Comment #44
alexpottDo we have test coverage of this?
Comment #45
BerdirWe clearly don't. That code there is fine.. the corresponding logic in BlockViewBuilder, however, not so much....
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():
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.
Comment #46
BerdirForget 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.
Comment #47
dawehnerLet's expand the test coverage ...
Comment #48
Wim LeersNit: comma can be removed, or should be moved to before the "even". Can be fixed on commit.
Comment #50
Wim LeersThis failure also is caused by #2552687: Test failures in ConfigFormOverrideTest and ContainerRebuildWebTest on newly spun up testbot instances.
Comment #53
Wim LeersThis needs a reroll because of #2552013: Follow-up for #2481453: ensure the 'url.query_args': MainContentViewSubscriber::WRAPPER_FORMAT cache context is set having landed in the mean time.
Comment #54
giancarlosotelo CreditAttribution: giancarlosotelo at MD Systems GmbH commentedThis should works.
Comment #55
LKS90 CreditAttribution: LKS90 commentedLooks fine, the testbot likes it, I'd say good to go.
Comment #56
gaurav.goyal CreditAttribution: gaurav.goyal at Innoraft commentedPatch #54 cleanly applies. Removing the needs reroll tag.
Comment #57
webchickCommitted and pushed to 8.0.x. Thanks!
Comment #61
Chi CreditAttribution: Chi commentedI 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?
Comment #62
BerdirYes, it is. There is an option somewhere in your view that you need to enable for this to work.
Comment #63
Chi CreditAttribution: Chi commentedThe option is "Hide block if the view output is empty".
It works fine. Thanks.
Comment #64
didaka CreditAttribution: didaka commentedI 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:
Comment #65
didaka CreditAttribution: didaka commentedComment #66
dawehner@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 :(
Comment #67
muash CreditAttribution: muash commentedsame problem here as of #64
Comment #69
afoster CreditAttribution: afoster commentedI 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
Comment #70
3eidoz CreditAttribution: 3eidoz commentedI'm having the same issue in D8.0.5
Comment #71
diogo_plta CreditAttribution: diogo_plta commentedI'm having the same issue in Drupal 8.1.1 : (
Comment #72
diogo_plta CreditAttribution: diogo_plta commentedI'm having the same issue in Drupal 8.1.1
Comment #73
LendudePlease 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.
Comment #75
jwilson3Just 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: