Problem/Motivation

Determination of visibility for a given region's content is done by checking:

<?php
if ($page['region']) {...
?>

This results in wrapping divs showing regardless of content existing, which prevents other modules from suppressing various regions through hook_page_alter() w/o resorting to unsetting given regions entirely. (i.e. no using #access = FALSE)

Proposed resolution

Several approaches have been considered:

  1. Use render() before determining visibility. Patch in #16 implements this solution.
    <?php
    $region_name = render($page['region']);
    if ($region_name) {...
    ?>
    

    Disadvantages

    • Confusing for themers (see #1158090: Separate usage of hide(), render(), and other template features from print statements to enable non-developers to create themes )
    • From #23: hide() will not work once the content is rendered, e.g. in:
      <?php if ($comment_form = render($content['comment_form'])): ?>
          <h2 class="title comment-form"><?php print t('Add new comment'); ?></h2>
          <?php hide($content['comment_form']['subject']); // This will not work. ?>
          <?php print $comment_form; ?>
        <?php endif; ?>
      
    • See #20 and #32. Many preprocess functions make decisions based on the unrendered state of the variable, which would not be consistent with this approach. The core issue is that each each time an element is themed, only an HTML string is returned and all the info used to render it is discarded.
    • There is a misunderstanding here however as the following works perfectly:
      <?php if ($comment_form = render($content['comment_form'])): ?>
          <h2 class="title comment-form"><?php print t('Add new comment'); ?></h2>
          <?php hide($content['comment_form']['subject']); // This will work, though it has been rendered already. ?>
          <?php print render($content['comment_form']); ?>
        <?php endif; ?>
      

      The trick is that drupal_render() itself keeps state of what has been rendered, but not #printed already. And if you do it like this:

      <?php if (is_printable($content['comment_form'])): ?>
          <h2 class="title comment-form"><?php print t('Add new comment'); ?></h2>
          <?php hide($content['comment_form']['subject']); // This will work, though it has been rendered already. ?>
          <?php print render($content['comment_form']); ?>
        <?php endif; ?>
      

      It will even be simple to understand ...

  2. Add a helper function.

    See #36 and #38. A helper function that checks whether the element should be empty or not (based on the element's values for #access, #printed, and #children) could be used both in templates and preprocess functions.

  3. Solution 3 (D8 only): Remove all show()/hide()/render() from templates.

Remaining tasks

Determine which solution to use.

Related issues:

User interface changes

Solution 1 would change tpl files as seen in the proposed code.

API changes

Original report by EclipseGc

I'm filing this against bartik, but in truth, garland, and system module both suffer from the exact same problem.

Currently determination of visibility for a given region's content is done by checking:

if ($page['region']) {...

instead of:

$region_name = render($page['region']);
if ($region_name) {...

This results in wrapping divs showing regardless of content existing, which prevents other modules from suppressing various regions through hook_page_alter() w/o resorting to unsetting given regions entirely. (i.e. no using #access = FALSE)

Eclipse

Files: 
CommentFileSizeAuthor
#109 953034-109.patch45.6 KBmariancalinro
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,636 pass(es), 133 fail(s), and 3,428 exception(s). View
#105 953034-105.patch45.15 KBmariancalinro
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 953034-105.patch. Unable to apply patch. See the log in the details link for more information. View
#97 953034-97.patch44.01 KBmariancalinro
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,560 pass(es), 133 fail(s), and 3,502 exception(s). View
#89 drupal-render-array-visibility--953034-89.patch3.03 KBsteveoliver
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-render-array-visibility--953034-89.patch. Unable to apply patch. See the log in the details link for more information. View
#85 Screen Shot 2013-07-30 at 10.10.00 AM.png223.14 KBtim.plunkett
#57 clean_up_template_files-953034-56.patch17.02 KBc4rl
PASSED: [[SimpleTest]]: [MySQL] 33,753 pass(es). View
#55 clean_up_template_files-953034-52.patch17.74 KBjenlampton
PASSED: [[SimpleTest]]: [MySQL] 33,756 pass(es). View
#53 clean_up_template_files-953034-52.txt17.71 KBbleen
#52 clean_up_template_files-953034-52.patch17.74 KBjenlampton
PASSED: [[SimpleTest]]: [MySQL] 33,755 pass(es). View
#16 drupal-953034-16.patch16.38 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 31,489 pass(es). View
#10 953034-template-files-empty-render-2.patch19.01 KBtstoeckler
PASSED: [[SimpleTest]]: [MySQL] 27,374 pass(es). View
#7 953034-template-files-empty-render.patch17.91 KBtstoeckler
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 953034-template-files-empty-render.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View

Comments

EclipseGc’s picture

Title: Themes improperly check page array to determine visibility » Themes improperly check page array to determine region visibility

better title.

Damien Tournoud’s picture

Yes, we need a new pattern here.

Instead of:

    <?php if ($page['sidebar_first']): ?>
      <div id="sidebar-first" class="column sidebar"><div class="section">
        <?php print render($page['sidebar_first']); ?>
      </div></div> <!-- /.section, /#sidebar-first -->
    <?php endif; ?>

I suggest we do:

    <?php if ($sidebar_first = render($page['sidebar_first'])): ?>
      <div id="sidebar-first" class="column sidebar"><div class="section">
        <?php print $sidebar_first; ?>
      </div></div> <!-- /.section, /#sidebar-first -->
    <?php endif; ?>
Jeff Burnz’s picture

Component: Bartik theme » theme system
Priority: Normal » Major

Perhaps we should bump this up to critical to get this fixed immediately. Raising to major and shifting to proper queue.

Indeed this is being done in all 4 core page.tpl.php files - system, bartik, garland and seven.

effulgentsia’s picture

Priority: Major » Critical
Issue tags: +markup
Perhaps we should bump this up to critical to get this fixed immediately.

Yeah. I agree it should receive attention before RC1, even if the answer ends up being "won't fix". I don't know if #2 is the best approach though. It makes sense, but is it themer friendly? Any themers want to chime in?

John Pitcairn’s picture

From a keep-it-simple standpoint for inexperienced themers, I'd expect the theme to handle the visibility logic and populate simple $sidebar_first (etc) variables in the appropriate template.php functions, rather than calling render() in a .tpl file.

tstoeckler’s picture

Assigned: Unassigned » tstoeckler

Working on this now. Will post a patch soon.

tstoeckler’s picture

Status: Active » Needs review
FileSize
17.91 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 953034-template-files-empty-render.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View

Here we go.

Just to make a case for this (as someone who came to Drupal by copying PHP snippets into my custom theme, without knowing a line of PHP):
This only effects places where you need to call render() anyway.
So instead of copying:

if ($page['sidebar_first']) {
  // ...
  print render($page['sidebar_first']);
}

you copy:

if ($sidebar_first = render($page['sidebar_first'])) {
  // ...
  print $sidebar_first;
}

If you don't know PHP, either is cryptic to you, and I don't think you can clearly say the latter is more cryptic. Either way, if you don't know PHP, you probably don't care and just copy&paste anyway.
If you know PHP, either is trivial.

Also from a markup standpoint, this is a bugfix not an aesthetical choice (see also issue category).

Let's see what Dries or webchick say.

claar’s picture

Status: Needs review » Needs work
+++ modules/comment/comment-wrapper.tpl.php	16 Nov 2010 22:16:47 -0000
@@ -37,16 +37,16 @@
+  <?php if ($comments = render($content['comments']) && $node->type != 'forum'): ?>

Bug if I'm not mistaken -- '=' has a lower precedence than '&&' http://php.net/manual/en/language.operators.precedence.php

Needs parens either way.

+++ themes/bartik/templates/comment-wrapper.tpl.php	16 Nov 2010 22:16:49 -0000
@@ -37,16 +37,16 @@
+  <?php if ($comments = render($content['comments']) && $node->type != 'forum'): ?>

Same

+++ themes/bartik/templates/page.tpl.php	16 Nov 2010 22:16:50 -0000
@@ -209,36 +209,41 @@
+  <?php if ($triptych_first = render($page['triptych_first'])

Same.

+++ themes/bartik/templates/page.tpl.php	16 Nov 2010 22:16:50 -0000
@@ -209,36 +209,41 @@
+    <?php if ($footer_firstcolumn = render($page['footer_firstcolumn'])

Same.

If we weren't so late in the D7 cycle, this would be a big -1 from me -- the creation of all of these extra variables like $help just smells bad -- it's not hard to imagine them causing head-scratching when they overwrite other same-named variables. Additionally, using an assignment operator inside if()'s is familiar to us coders, but seems certain to confuse themers who aren't strong on PHP.

Unfortunately, I don't have a better pattern to suggest, and I agree with tstoeckler's evaluation of copy-paste coders not caring what the syntax looks like. Thanks for the patch, tstoeckler.

Powered by Dreditor.

claar’s picture

I just re-read and completely agree with comment #5 -- that may be a D8 thing, though.

tstoeckler’s picture

FileSize
19.01 KB
PASSED: [[SimpleTest]]: [MySQL] 27,374 pass(es). View

What was I thinking? Thanks claar!
The latter two examples you give in #8 are correct, though.
I found two other examples where I was doing it wrong, though.

tstoeckler’s picture

Status: Needs work » Needs review

Setting to "needs review".

bleen’s picture

Status: Needs review » Needs work

I think that this approach is (a) a hair too complex for the average themer (b) the least complex option at this late stage.

That said... re the patch in #10

+++ modules/update/update.fetch.inc	16 Nov 2010 23:08:23 -0000
@@ -374,6 +374,16 @@ function update_parse_xml($raw_xml) {
+      if (isset($release->files)) {
+        $data['releases'][$version]['files'] = array();
+        foreach ($release->files->children() as $file) {
+          $file_data = array();
+          foreach ($file->children() as $k => $v) {
+            $file_data[$k] = (string) $v;
+          }
+          $data['releases'][$version]['files'][] = $file_data;
+        }

huh? Is this supposed to be part of the current patch? What am I missing

Powered by Dreditor.

webchick’s picture

Version: 7.x-dev » 8.x-dev

Uh?!? No. Way. Those templates have had that markup in them since 2009. They're staying put.

Moving to D8, but this is probably even won't fix. That logic doesn't belong in tpl.php files.

tstoeckler’s picture

Just for the record: no #12 doesn't belong in there.

tim.plunkett’s picture

Ran into this today in Bartik with $tabs, during a demo.
I added dpr($variables['tabs']); to bartik_process_page(), and went to the home page:

Array
(
    [#theme] => menu_local_tasks
    [#primary] => 
    [#secondary] => 
)

The output of render($tabs) is nothing.
In the markup, right after <a id="main-content"></a> was <div class="tabs"></div>

In response to #13, in Garland it was if ($tabs): print '<ul class="tabs primary">'. $tabs .'</ul></div>'; endif;. There were no renderable arrays, so the if() statement actually did something.

I'll roll another patch soon, but #10 is along the right line.

tim.plunkett’s picture

Assigned: tstoeckler » Unassigned
Status: Needs work » Needs review
FileSize
16.38 KB
PASSED: [[SimpleTest]]: [MySQL] 31,489 pass(es). View

Here we go.

tim.plunkett’s picture

Title: Themes improperly check page array to determine region visibility » Themes improperly check renderable arrays when determining visibility
Version: 8.x-dev » 7.x-dev
Priority: Critical » Major

In D6, variables like $tabs contained the actual HTML output, so using if ($tabs) was valid.
With the introduction of renderable arrays, the $tabs variable doesn't contain the HTML output until render() is called.
Calling render() in preprocess/process is not an option, because it removes the ability for a tpl.php to use hide().

Therefore, this patch is only updating the templates to work with renderable arrays. The current behavior is a regression.

In D8 this should probably be revisited, but in the meantime this is the easiest way to fix it for D7.

tstoeckler’s picture

Patch looks great. You'll have to convince webchick, though, see #13

EclipseGc’s picture

Tim, Thanks for hopping into this issue. That was kind of my point from the get-go. The logic in the tpl files is insufficient, and regardless of how long it's been there, you CAN and WILL end up with regions that render, but have no content (say, if all the content in a region is set to #access => false). So we need a fix, IN D7. *end rant*

David_Rothstein’s picture

I don't think this patch is going to work, since we have code in a variety of preprocess functions that makes decisions based on the unrendered state of the variable also. For example, in template_preprocess_html():

  // Add information about the number of sidebars.
  if (!empty($variables['page']['sidebar_first']) && !empty($variables['page']['sidebar_second'])) {
    $variables['classes_array'][] = 'two-sidebars';
  }
  elseif (!empty($variables['page']['sidebar_first'])) {
    $variables['classes_array'][] = 'one-sidebar sidebar-first';
  }
  elseif (!empty($variables['page']['sidebar_second'])) {
    $variables['classes_array'][] = 'one-sidebar sidebar-second';
  }
  else {
    $variables['classes_array'][] = 'no-sidebars';
  }

I think the checks have to be consistent, or else couldn't we wind up with pages that have the "two-sidebars" class applied to the body, but no trace of two sidebars anywhere in the HTML? That would be even worse than the current situation.

It seems very difficult to fix this in Drupal 7. Maybe the best we could hope for is to add a special case for situations where #access was specifically set to FALSE, and make sure the variable is forced to be empty then. Those are easier to detect since we know exactly what to look for. (It might even be considered a security hardening improvement, to make sure the tpl.php file doesn't have any chance to use variables that the user is explicitly denied access to.)

bfroehle’s picture

Jacine’s picture

sub.

sun’s picture

Hm. This is a bit mind-boggling. With the suggested approach, you cannot do the following anymore:

  <?php if ($comment_form = render($content['comment_form'])): ?>
    <h2 class="title comment-form"><?php print t('Add new comment'); ?></h2>
    <?php hide($content['comment_form']['subject']); ?> <---------------------- #FAIL
    <?php print $comment_form; ?>
  <?php endif; ?>

unless you know that you have to hide() before you render(); i.e., before the entire wrapping condition — and that said, performing the hide() as in this example in case there is no comment form will trigger a PHP notice/warning, since the array key doesn't exist.

tim.plunkett’s picture

Status: Needs review » Needs work

How do we prevent empty markup from being printed, without removing the usefulness of render arrays?

tstoeckler’s picture

@#24: In light of #20/#23:
I think it's pretty clear that, ideally, this patch is the way forward, because no one wants to output empty markup. The question, though, is whether we can get away with potentially breaking existing templates (#23) or theme functions (#20, although that seems to be a bit more difficult, because there's no real workaround, I think) for existing D7 themes.
On the other hand, empty markup can also break layout, styling, etc. in themes. Except that that's been the case for the last year or whenever render arrays got in. And, since it's all templates and theme functions, it's not like people are actually stuck with empty markup on their sites or in their themes.
So in light of #20/#23 I would vote to punt this to D8 unless someone comes up with a way to remove empty markup and retain backwards compatibility.

moshe weitzman’s picture

I'm not too bothered by #23. there are lots of other ways to hide the comment subject. A themer can use existing technique if she really wants.

As for David's #20 - template_preprocess_html() runs after sidebar_first has been rendered so it should be able to check #children and see if it is empty. So, if (!empty($variables['page']['sidebar_first']['#children']). All I did was append a #children to the existing check.

Problem is, #children (and #printed) is never present in $variables during preprocess and is also missing when we do subsequent drupal_render() calls. I added EXTR_REFS to theme_render_template() but there must still be more copies of $elements that are losing this data.

Sorry i can't be more helpful.

claar’s picture

Can someone explain the code change that caused this render() call in the template to be necessary?

Adding render() in the template like this makes Drupal templates one step harder to understand. For Drupal 8, where we have a nice new blank slate, can we not work toward a solution that doesn't make templates less readable?

Like Webchick said, the old syntax has been in Drupal a looong time, and it's a shame to change it to something less understandable.

tim.plunkett’s picture

@claar, take a look at core's node.tpl.php around line 100: http://drupalcode.org/viewvc/drupal/drupal/modules/node/node.tpl.php?rev...

Being able to use show() and hide() before render() is awesome.

JohnAlbin’s picture

This issues shows the fundamental flaw in the idea of combining show/hide/render with the desire to be able to conditional flow HTML in the template.

Dealing with wrapper divs that only wrap a single variable can still be done relatively easily. You just have to move the wrapper out of your current template and into the theme hook that the variable is using. e.g. Instead of adding a wrapper div around the page.tpl's $sidebar_first variable, add the wrapper div to region--sidebar-first.tpl.

But dealing with wrapper divs that wrap multiple variables can't be solved that way. The "easiest" solution for d7 is to use the ugly-ish if ($foo = render($bar)):

We either need to solve this problem better in D8 or remove all the show/hide/render stuff from templates.

jenlampton’s picture

subscribing

nicl’s picture

subs. Just to say, as a relative newbie to Drupal theming (or at least I was so fairly recently), the addition of render() is very useful indeed but does confuse new people from my experience. I'm talking here of the hobbyist/web-amateur, not experienced professionals who will immediately look up the API.

moshe weitzman’s picture

I looked a little more into this, and especially David's problem from #20.

The core problem is that we have lost all information about what was empty and what was not (i.e. #children) by the time we exit page,tpl.php. So we can't definitively say if any region was empty or not during template_preprocess_html(). While we are in page.tpl.php, $page carefully keeps a history of what stuff was #printed and what each elements contribution to the HTML was. But thats all tossed out when theme_render_template() finishes. Each time we theme an element, we discard this important info and only return a string of HTML. I would love for each element's #children and #printed to persist inside of drupal_render(). Though we might have to be satisfied with a #empty boolean instead of #children in order to conserve memory ... I don't immediately know how to accomplish this persistence though.

effulgentsia’s picture

Version: 8.x-dev » 7.x-dev
Issue tags: -needs backport to D7

Re #31: Thanks for your feedback. We do need to get a better handle on how many themers are finding difficulty with render(), and whether the solution is documentation, rethinking some architecture, or something else. Please join that discussion on #1158090: Separate usage of hide(), render(), and other template features from print statements to enable non-developers to create themes , especially if you have ideas on how we can move that discussion from just anecdotal guesses to real "themer experience" data.

If a significant change gets committed in that issue, that may change this issue's direction. But in the meantime, let's keep this issue focused not on whether render() in templates is in itself confusing, but whether something along the lines of #16 makes sense, given the problem in #20, summarized well in #25, and getting dug into a little more in #32.

effulgentsia’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +needs backport to D7
dvessel’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +needs backport to D7

subscribing

dvessel’s picture

What's wrong with using a helper function? Was this considered already?

This function might need more conditions but you get the idea.


/**
 * Check if the render array element has content.
 */
function has($element) {

  if (isset($element['#access']) && !$element['#access']) {
    $element = FALSE;
  }
  elseif (!empty($element['#printed']) && isset($element['#children'])) {
    $element = $element['#children'];
  }

  return !empty($element);
}

  // Add information about the number of sidebars.
  if (has($variables['page']['sidebar_first']) && has($variables['page']['sidebar_second'])) {
    $variables['classes_array'][] = 'two-sidebars';
  }
  elseif (has($variables['page']['sidebar_first'])) {
    $variables['classes_array'][] = 'one-sidebar sidebar-first';
  }
  elseif (has($variables['page']['sidebar_second'])) {
    $variables['classes_array'][] = 'one-sidebar sidebar-second';
  }
  else {
    $variables['classes_array'][] = 'no-sidebars';
  }
<?php if (has($content['comment_form'])): ?>
  <h2 class="title comment-form"><?php print t('Add new comment'); ?></h2>
  <?php hide($content['comment_form']['subject']); ?>
  <?php print render($content['comment_form']); ?>
<?php endif; ?>

This doesn't complicate things when you compare it with !empty(). It's actually easier to read IMO.

moshe weitzman’s picture

@dvessel - did you actually run that code? when i looked into the problem described in #20, #children has vanished by the time we enter into template_preprocess_html(). the reason why is #32.

dvessel’s picture

No, but maintaining persistence would need to happen in two places.

theme_render_template is the easy one and doesn't maintain references.

  extract($variables, EXTR_SKIP); // Extract the variables to a local namespace

That can be fixed with the EXTR_REFS + EXTR_SKIP flags.

Not sure how theme() will behave when the "$variables" parameter is referenced and that may be trickier.

c4rl’s picture

Subscrizzle

Devin Carlson’s picture

subscribing

klonos’s picture

...subscribing.

sachbearbeiter’s picture

subscribing

xjm’s picture

Tagging.

marcoka’s picture

Status: Needs work » Needs review
marcoka’s picture

Issue summary: View changes

issue summary

marcoka’s picture

Issue summary: View changes

markup fixes

marcoka’s picture

Issue summary: View changes

syntax fix

marcoka’s picture

Issue summary: View changes

another syntaxfix, fixing liked comments

xjm’s picture

Status: Needs review » Needs work
Issue tags: -Needs issue summary update

Setting back to NW (patches are in old format). First summary added by e-anima.

xjm’s picture

Issue summary: View changes

added related issues

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Corrected and clarified the summary a bit.

xjm’s picture

Issue summary: View changes

Updated issue summary.

Jacine’s picture

Hey all, can we please figure out a way forward for this soon? We're running into issues with this while converting templates to HTML5. I've been using the approach Damien recommends in #2, but don't want to implement that across all these templates until a decision has been reached here. We've only really attempted to address Drupal 7 here. What's the plan for Drupal 8?

moshe weitzman’s picture

IMO, #2 is the solution until someone has a working alternative. I'd say that improvements to templates should not wait on this issue.

Jacine’s picture

Alright, thanks Moshe. :)

Anyone else have an issue with this? If everyone is alright with #2 then perhaps we should fix it all in one patch here.

webchick’s picture

#2 is basically fine for D8, as long as the markup maintainers are fine with more PHP logic being introduced into template files. The only complaint raised against it is #23, which is a bit of an edge case.

We need another, less every-template-changes-as-a-result solution for D7 though.

jenlampton’s picture

I still think we should get all the render calls out of all of the template files in core.

Let's move them into preprocess, and then we can have clean and simple logic only in the template files.

function whatever_preprocess_page(&$variables) {
 $variables['sidebar_first'] = render($page['sidebar_first']);
}

and then the templates are NOT SCARY anymore and any HTML person can understand them

<?php if ($sidebar_first): ?>
  <div id="sidebar-first" class="column sidebar"><div class="section">
    <?php print $sidebar_first; ?>
  </div></div> <!-- /.section, /#sidebar-first -->
<?php endif; ?>

Don't get me wrong, I like render. But it does not belong in core template files. It's an advanced tool, and advanced people will not be using core themes anyway. Beginners will be looking at core themes, and these files need to make sense to them.

jenlampton’s picture

Status: Needs work » Needs review
FileSize
17.74 KB
PASSED: [[SimpleTest]]: [MySQL] 33,755 pass(es). View

Here's a patch that updates all of bartik's template files. Take a look and let me know what you think.

bleen’s picture

This is the same as the patch in #52 with all the whitespace issues removed

webchick’s picture

Damn, #52 looks sexy. Certainly makes our template files look a lot more like standard (and Googleable) HTML + PHP again, which is great for "normal" people who just want to take a theme and tweak it. It does mean a bit more work for theme developers, though, but I think those folks are largely used to muddling around in template.php and preprocess functions already.

Obviously we can't backport that approach to D7 unforutnately, though. :(

jenlampton’s picture

FileSize
17.74 KB
PASSED: [[SimpleTest]]: [MySQL] 33,756 pass(es). View

Here's #53 again with .patch, for testbot.

I think we should go this route for D8. I'll write a patch for D7 that follows #2 if we can't find a more elegant solution.

xjm’s picture

I like this approach. Much better for #1158090: Separate usage of hide(), render(), and other template features from print statements to enable non-developers to create themes .

Note that #55 has some trailing whitespace and some inline comments that should probably be wrapped at 80 chars, so we'll want to clean that up.

c4rl’s picture

FileSize
17.02 KB
PASSED: [[SimpleTest]]: [MySQL] 33,753 pass(es). View

This is a re-roll of #55 that I believe fixes the whitespace issues.

moshe weitzman’s picture

The justification given so far for removing the render() calls is that they are SCARY. Lets balance that against the benefits provided by render() in templates. I think these benefits are pretty compelling:

  1. Maintainability: Customized templates still work. That is, templates when you decide to output field_foo separately from the rest. Once you do that, new contrib modules like fivestar stop printing their bits and and new Fields will not be output.
  2. Security: Many folks were printing unsafe data from $node->field_foo in the template. render() is does this safely
  3. Performance: Only stuff that actually prints gets themed. Moving this work to preprocess means that we do extra work. Further, folks will often print theme everything into $content AND theme the bits they need separately. Even worse performance.
  4. Empowerment: Template author has one workplace. This technique forces template owners to dive into preprocess
  5. Backwards Compat: You don’t have to use render(). I'm totally fine if one theme in core uses preprocess like this patch proposes. I don't think it should be the default theme though.
EclipseGc’s picture

Basically we have to weigh Moshe's very legitimate set of objections against the simple problem that we can't run a clean if statement against un-rendered arrays. That's the bottom line. Our two primary objections are summed up thus:

  1. That fix is UUUUUGLY and designers aren't going to understand.
  2. Moshe's argument.

Someone's got to give here... personally I favor the ugly fix cause it's not THAT complicated, and designers are used to copying and pasting stuff they know works.

c4rl’s picture

Responses to Moshe's objections list of benefits in #58:

1. Maintainability: Customized templates still work. That is, templates when you decide to output field_foo separately from the rest. Once you do that, new contrib modules like fivestar stop printing their bits and and new Fields will not be output.

I don't think moving render() to preprocess changes this principle, but correct me if I am wrong.

2. Security: Many folks were printing unsafe data from $node->field_foo in the template. render() is does this safely

render() doesn't guarantee that people aren't going to still write bad code. If people don't understand how to sanitize their data, that is not our problem. Giving them render() is like giving them a slap-chop instead of teaching them how to use a knife.

3. Performance: Only stuff that actually prints gets themed. Moving this work to preprocess means that we do extra work. Further, folks will often print theme everything into $content AND theme the bits they need separately. Even worse performance.

I don't see how there is extra work? Either we are rendering in a tpl and checking for an empty string or we are rendering in template.php and then checking for an empty string in the tpl. The argument that "folks will often print theme everything into $content AND theme the bits" with regards to performance follows the same response I had for #2: If people don't understand how to render variables separately, that is not our problem. It *is* our problem to have an inconsistent theming API, imho. Also see my response to #4.

4. Empowerment: Template author has one workplace. This technique forces template owners to dive into preprocess

This argument has always been troublesome to me. Anyone who is serious about theming is going to have to learn preprocess functions someday; It's like wanting to be a race car driver without learning how to drive manual transmission. As someone who has given several Drupalcon sessions about preprocessor functions, I have had direct feedback from attendees about how useful the concepts are and how attendees felt like they now "understand" theming in Drupal. We should stop fooling ourselves that themers and designers aren't smart enough to write PHP and dive into template.php -- it is inevitable that they will have to enter this world someday if they are going to be writing custom themes in Drupal.

5. Backwards Compat: You don’t have to use render(). I'm totally fine if one theme in core uses preprocess like this patch proposes. I don't think it should be the default theme though.

Saying "you don't have to use render()" is an argument for having render() in tpls? Do I misunderstand? :)

jenlampton’s picture

I do not think we should use render() in Bartik's template files. I believe the opposite of Moshe's #5.

Bartik is the default theme for Drupal, and that means that people who don't know any better are going to be changing it directly, copying it to make their own theme, or doing who-knows-what else do it - as beginners will do. These are people who will not understand how to properly use render(). These template files are the ones that need to be the most inviting - because this is where people will start.

Sure - let's leave render in stark. We can assume that people who will be starting from scratch (or at least think they want to) can figure it out. But in Bartik? no way.

Moving the render calls to preprocess in Bartik solves this issue, and is a start to the bigger problem in #1158090: Separate usage of hide(), render(), and other template features from print statements to enable non-developers to create themes

webchick’s picture

Hm. I wonder if a lot of Moshe's concerns could be mitigated by changing:

+      <?php print $content; ?>

to

+      <?php print render($content); ?>

Just $content (and things inside $content, like $content['links']), no other template variables. That would still remarkably clean up the template output while also exposing to themers to the power of the render API, thus hopefully alleviating their tendency to do a D6-like "blow away this crappy $content variable and start printing raw field values directly" behaviour that I think Moshe is concerned about.

One thing:

"As someone who has given several Drupalcon sessions about preprocessor functions, I have had direct feedback from attendees about how useful the concepts are and how attendees felt like they now "understand" theming in Drupal."

Yeeeeahhh. That's actually a problem that sort of proves Moshe's point. :) Not everyone has the ability to go to DrupalCon and hear the almighty Carl Wiedemann explain preprocess functions to them. :) The .tpl.php file is the more natural workspace of a novice-level themer/designer, as evidenced by the unholy things you will find in there with themes coded by novices (SQL queries, 70,000-line functions, and the like). Exposing hints that there are more flexibe ways to render content in the tpl.php file itself will likely lead to more people making their way to template.php without "outside" help.

jenlampton’s picture

Hm. This is an interesting idea.

I still think that calling functions like hide() are going to scare people away - and I don't want that in Bartik's tpl.php files - but maybe there's a way we can render($content) after rendering links and other parts of $content in preprocess.

I'd be willing to compromise on something like this.

Crell’s picture

I am no fan of render(), but "pre-render everything we may possibly need just in case" is exactly the core bug we need to get away from.

c4rl's point is also valid. At some point we have to just accept that theming a complex system like Drupal is complex; complex theming cannot be done by a novice whose only experience is Dreamweaver.

We already have a system where a non-PHP dev is hamstrung when trying to theme stuff. Every single version of Drupal since I've gotten involved has tried to make theming friendlier toward someone who doesn't know what they're doing, and every version it's gotten more complex and hasn't fixed that problem. (I'd argue Drupal 6 actually had the best balance there.)

Perhaps instead of putting our energy into "make it possible for someone to theme without understanding WTF is going on", we should put it into "make it easier for someone to *learn* (not understand intuitively, learn) WTF is going on so they can be useful." And if that means that anything more complex than pure-CSS you need to learn at least some PHP for... Then we're just being honest about what is, arguably, already the situation.

dvessel’s picture

I have not tested for possible side effects but if we must render before the template, using the second phase variable process functions will still allow some flexibility. It will allow modules and themes to preprocess specific fields before render() is called. I would test it myself but we're experiencing a major power outage.

c4rl’s picture

Not everyone has the ability to go to DrupalCon and hear the almighty Carl Wiedemann explain preprocess functions to them. :)

Well, now that I have finished swooning ;) I will remark that part of the issue here is a documentation/learning-curve issue. Not only do we need a consistent API, but we need to explain the API.

A comment and patch on a related issue by David_Rothstein beings to address this question. (#1158090: Separate usage of hide(), render(), and other template features from print statements to enable non-developers to create themes )

effulgentsia’s picture

In addition to Moshe's concerns, please also consider the subtheme issue. Copying from #1158090-92: Separate usage of hide(), render(), and other template features from print statements to enable non-developers to create themes :

If you render a top-level variable in a preprocess function (or even a process function), then you've removed the ability for a subtheme to render a subpart of that variable separately. It's fine for a site custom theme to choose to not be an easily extendable base theme, but I don't think we'd want to limit our core themes like this.

If we believe that bartik is not a good base theme, then ok, let's put render() in process(), and document somewhere why we don't think it's a good base theme.

If we want to move render() out of templates and also support subtheming, then we need a new type of callback that runs after process(), and that only runs for the concrete theme and not for base themes.

c4rl’s picture

Wow. That's a prickly one, indicated in #67. Indeed subtheming by definition requires the base theme's preprocessor to run.

So render(), by design, isn't *currently* intended for preprocessors, but rather for templates, and the subtheming issue is thus a consequence.

I don't want to throw off the conversation too much, but does this beg the question whether render() should change (and if so, how can render() change) to be used in preprocessors?

/me ducks :)

dvessel’s picture

If you render a top-level variable in a preprocess function (or even a process function), then you've removed the ability for a subtheme to render a subpart of that variable separately.

I believe this isn't correct. Process functions always come after preprocess no matter which module or theme implements them.

To be clear, I'd rather have consistent use of render(). Rendering in some_process_foo() will inevitably snowball into more inconsistent behavior.

DamienMcKenna’s picture

A few Centauri Ducats:

  • D7 is currently extremely limited in what can be done because a) it's released, b) there are lots of existing themes (and sites that don't have time to be heavily updated), so any changes to D7 *have* to be limited to the tpl files themselves.
  • D8 doesn't have any limitations, don't limit it to what is possible with D7.
  • IMHO this needs to be split into two issues - what is possible with D7 and forming something usable out of the sky's-the-limits possibilities for D8.

Ideas:

  1. Simple suggestion for improving the TX (Theming Experience): wrap "print render()" in one single "output()" function, IMHO that'd help theming n00bs.
  2. The pattern of using "if($monkey){print render($monkey);}" will (IIRC) give warnings if error_reporting is set to E_ALL and $monkey doesn't exist. Wrapping it as "if(empty($monkey))" resolves the E_ALL problem but still doesn't identify if the render array is "empty". How about adding a function like "has()" per comment #36, though renaming it to e.g. "is_empty()" or "drupal_empty()" (sic)?

Putting 1 and 2 together gives you:

<?php if (drupal_empty($monkey)): ?>
  <div class="monkey"><?php output($monkey); ?></div>
<?php endif; ?>
klonos’s picture

Once we get this right for D8, we can backport it in D7 by allowing D7 to check arrays for visibility both ways. This will offer some sort of early adoption of the new "proper" way by themers in D7 while still allowing themes crafted the old way to work (or rather not cause them to break). The old way can then be completely deprecated in D8.

Am I making any sense?

DamienMcKenna’s picture

Status: Needs review » Needs work

@klonos: Ok, but I recommend we completely ignore D7 in the current discussion for now.

Based on various feedback above I'm changing this back to NW.

FYI there's an issue to review another theming engine with a view to completely replacing the PHPTemplate in D8: #1441320: Evaluate Twig template engine for Drupal core (as an alternative for PHPTemplate and possibly even theme functions)

David_Rothstein’s picture

For D7, I'm pretty sure Damien's original solution in #2 works fine for any theme that wants to adopt it.

But as for the core themes, I'm not sure we'll ever have a backportable fix here? (By definition, fixing this means changing the HTML markup in some cases...)

hass’s picture

sachbearbeiter’s picture

is there now a solution for the never empty tabs?
it costs me a hour, to find out that this is a bug in D7 ...
symfony and twig are coming - my only comfort
i'm sorry but drupal7 is like vista - a monster ...

xjm’s picture

@sachbearbeiter, posts like that are unlikely to make anyone want to help you resolve the issue.

The main problem in this issue is finding a solution which is backportable to Drupal 7 -- we have to weigh carefully changing the output, because this will very easily break existing contributed and custom themes. See: http://drupal.org/node/1527558

See comment #2 for a workaround you can use in D7.

sachbearbeiter’s picture

thanks ...

hass’s picture

When are we able to get this finally fixed for D7? I believe this not longer exist under D8 since twig, but I'm not sure.

David_Rothstein’s picture

It looks like this happens less often in Drupal 8, but is still an issue. For example, just set $page['sidebar_first']['#access'] = FALSE in hook_page_alter(), and the sidebar will go blank but its HTML markup won't actually disappear.

I'm also not sure what the Twig equivalent of something like #2 would even be... does Twig support the ability to do anything like that?

Cottser’s picture

Issue tags: +Twig

I don't think Twig has an equivalent inline syntax like in #2. There are set blocks to capture output that we could teach people to use but that seems confusing to me. http://twig.sensiolabs.org/doc/tags/set.html

However we are already extending Twig for things like show() and hide(), so maybe we could just treat render arrays in a special way for if statements. One option might be to have {% if monkey %} do a kind of "soft render" (don't set #printed to TRUE), and then return TRUE/FALSE depending on if anything actually comes out. The rendering would probably need to be cached though otherwise performance will suffer.

Cottser’s picture

tim.plunkett’s picture

Priority: Major » Critical

Yeah, this is a huge problem now because it affects not just $tabs or $messages, but the *sidebar*. That's a big chunk of the page.

David_Rothstein’s picture

It affects the sidebar in Drupal 7 too, as far as I know.

Is this really a critical issue? It's just a bit of extra HTML on the page... Though it would be good to make sure there's a way to deal with this in Twig, since at least in Drupal 7 there's already a pretty good fix for any theme that wants to avoid the HTML.

tim.plunkett’s picture

Here's a clean install of D8, with the user login block hidden.
The entire sidebar div is printed, the one-sidebar class is added to the body.

You can't theme around that. And it's a regression.

Screen Shot 2013-07-30 at 10.10.00 AM.png

tim.plunkett’s picture

I used this hack to kill the sidebars:

function mytheme_page_alter(&$page) {
  foreach (element_children($page) as $region) {
    foreach (element_children($page[$region]) as $block) {
      if (!element_children($page[$region][$block])) {
        unset($page[$region][$block]);
      }   
    }   
    if (!element_children($page[$region])) {
      unset($page[$region]);
    }   
  }
}
tim.plunkett’s picture

We had a potential solution for this in phptemplate, but now it seems we do not have any twig solutions available.

steveoliver’s picture

Assigned: Unassigned » steveoliver

EDIT: I see this can (and should) move forward only one way:

1. Implement helper method has() or has_content() in D7 and D8, use it to check render arrays in PHP/template without rendering them as suggested in #36.
2. Implement {% if var has content %} in D8 Twig.

steveoliver’s picture

FileSize
3.03 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-render-array-visibility--953034-89.patch. Unable to apply patch. See the log in the details link for more information. View

I'm stuck with attached patch. I have a question: See comments in this patch for now, will elaborate in next comment or something.

steveoliver’s picture

Assigned: steveoliver » Unassigned

I'd like someone else to take a look at this. I don't want to use drupal_render(), of course, but it's there so i can see how 'visibility' is checked i.e. for a block.

The question first I'm trying to answer now is:

1. How is block visibility determined? Just by drupal_render()?

And then,

2. Can we determine render array output won't be blank /without/ actually [double] rendering it?

steveoliver’s picture

Issue summary: View changes

Updated issue summary.

jibran’s picture

Issue summary: View changes
Status: Needs work » Needs review

sending to bot.

Status: Needs review » Needs work

The last submitted patch, 89: drupal-render-array-visibility--953034-89.patch, failed testing.

joelpittet’s picture

I'm quite sure we'll need to render the full renderable array for this to work in all cases(We'll shoot ourselves in the foot if we forget #access checks or something). So a full call to drupal_render(). In twig that means you should use a 'render' filter {{ comments|render }}. This doesn't exist YET but close now is the render_var function which work like this:

{% set comments_rendered = twig_render_var(comments) %}
{% if rendered_comments %}
  <div>
    {{ rendered_comments }}
  </div>
{% endif %}

Propose (slightly simpler)

{% if rendered_comments = comments|render %}
  <div>
    {{ rendered_comments }}
  </div>
{% endif %}

render just gives us a string earlier. And it may also be possible to render in the if condition like this, but we'd have to change the TwigNodeVisitor to accommodate and maybe store a rendered version on the array to avoid multiple calls:
Propose (slightly simpler)

{% if comments %}
  <div>
    {{ comments }}
  </div>
{% endif %}
mdrummond’s picture

I think that last version, if we could get it to work, would be by far the friendliest to site builders and themers. That's how everybody is going to assume it works.

What specifically would we need to do to make that possible?

joelpittet’s picture

If we can simplify the way that render arrays are passed around, I'll gladly work on a solution to get the last one working. Just need eyeballs on the critical #2114563: Remove TwigReference with the help of 'without' filter replacing 'show'/'hide' functions. to get it RTBC and so we can solve some of these bigger problems.

rteijeiro’s picture

Assigned: Unassigned » rteijeiro
mariancalinro’s picture

Assigned: rteijeiro » Unassigned
Status: Needs work » Needs review
FileSize
44.01 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,560 pass(es), 133 fail(s), and 3,502 exception(s). View

Here's another idea:

We could use a twig test to render the array, and see if it's empty or not. Coupled with a static cache for the rendered content, this should not add too much execution time.

I took a stab at this idea, and created a patch. The only thing i don't like of my solution is the name I came up with for the test ("content"), but I'm sure someone will come with a better name, and it's easy to change.

For the rendered content cache, I created the identifier by doing a hash of the serialized array. This should change when the array changes, even somewhere deep in it.

Please let me know what you think about this solution.

mariancalinro’s picture

I forgot to add an example of how this works (although it's visible in the patch).

    {% if page.sidebar_first is content %}
      
        {{ page.sidebar_first }}
      
    {% endif %}
mdrummond’s picture

I'm not sure if there's a feasible way to do this, but if there's a way to make this construction work, I think it's going to be a lot easier and more intuitive.

    {% if page.sidebar_first %}
      <aside class="l-sidebar-first" role="complementary">
        {{ page.sidebar_first }}
      </aside>
    {% endif %}

The is content construction feels awkward. This feels like a Drupalism that departs from standard Twig syntax.

Cottser’s picture

It may end up being a necessary Drupalism because render arrays are definitely a Drupalism. Unless we can do something similar to twig_render_var where we wrap all Twig tests with twig_check_var or something.

I just want to mention that we will also need the ability to check renderable arrays from preprocess so I don't think the implementation can be tied so closely to Twig. Thanks for working on this @mariancalinro!

Cottser’s picture

And I want to note that there is already a render cache that may be of use, see the drupal_render() docs.

Cottser’s picture

Finally adding a somewhat related issue if we're talking about doing something along the lines of render_var()/twig_render_var().

Status: Needs review » Needs work

The last submitted patch, 97: 953034-97.patch, failed testing.

mariancalinro’s picture

I will look at the caching of drupal_render, thanks.

I also think I found a better name for the test : "renderable". Example:

    {% if page.sidebar_first is renderable %}
      
        {{ page.sidebar_first }}
      
    {% endif %}
mariancalinro’s picture

FileSize
45.15 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 953034-105.patch. Unable to apply patch. See the log in the details link for more information. View

So, I made a new iteration.

I created a function in common.inc, called is_printable, that checks if the render array produces output or not. This can be called from preprocess.

I didn't create a twig wrapper arround it, it didn't seem necessary, so the twig test only calls the function is_printable. I also changed the name of the test to "printable". This makes the semantics of php and twig similar. Ex:
PHP:

  if (is_printable($variables['page'])) {
    // Do something
  }

Twig:

  {% if page.sidebar_first is printable %}
    <aside class="l-sidebar-first" role="complementary">
      {{ page.sidebar_first }}
    </aside>
  {% endif %}

Now, for the caching, I don't think we can use the render caching for the following 2 reasons:

  1. drupal_render caching system is persistent between requests, and we don't always want that.
  2. we should cache the output of all the render arrays, but only use it if the content is exactly the same (ex: you have a node listed in 2 views on a page, but one has a field hidden, and one not). With the drupal_render cache, if the cid is the same, you can't differentiate the render arrays.

What I did was move the static cache inside the render function, and continue to use the hashing mechanism from #97. This allows us to cache the render output for the duration of the request, and also if something is changed in the array, the hash will be different, and the markup will not be retrieved from the static cache, instead the array is rendered.

This doesn't mean that we are bypassing the drupal_render cache, when render calls drupal_render, drupal_render can still get the markup from cache.

mariancalinro’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 105: 953034-105.patch, failed testing.

Cottser’s picture

@mariancalinro - thanks! I'd suggest taking out all the template changes while we're still trying to figure out how this could work :)

mariancalinro’s picture

Status: Needs work » Needs review
FileSize
45.6 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,636 pass(es), 133 fail(s), and 3,428 exception(s). View

reroll

mariancalinro’s picture

OK, I'll do that in the next patch, if any.

In the mean time, I would love to hear your thoughts on the last iteration.

Thanks.

Status: Needs review » Needs work

The last submitted patch, 109: 953034-109.patch, failed testing.

joelpittet’s picture

+++ b/core/modules/system/templates/details.html.twig
@@ -16,17 +16,17 @@
-  {%- if title -%}
+  {%- if title is printable -%}

It's an interesting approach to the problem but maybe there is a way to make this hidden from the user, aka not a test condition? This makes the conditions more verbose in expectation that you know that inside is a renderable array. And if it's not a renderable array you are needlessly running it through the "is printable" test.

Maybe there is a way to run values through twig_render_var and store the cached rendered output version like you have done here? Spit balling here...

steveoliver’s picture

Issue tags: +Needs profiling

mariancalinro++

  1. +++ b/core/includes/common.inc
    @@ -3994,7 +3994,20 @@ function render(&$element) {
    - return drupal_render($element, TRUE);
    +    // Try to get the rendered element from the static cache. This only
    +    // persists for the current request, and is needed because we test if a
    +    // render array produces output by rendering it and checking if the output
    +    // is empty. So without this, if the element is not cached by drupal_render,
    +    // it is rendered for each test, and also for printing on page.
    +    $static_cache = &drupal_static(__FUNCTION__);
    +    $static_cid = md5(serialize($element));
    +
    +    if (!isset($static_cache[$static_cid])) {
    +      $static_cache[$static_cid] = drupal_render($element, TRUE);
    +    }
    + return $static_cache[$static_cid];
    

    Adds (1) call to serialize() and md5() for each call to render().

  2. +++ b/core/includes/common.inc
    @@ -4059,6 +4072,20 @@ function show(&$element) {
    + * Tests if a render array produces output in the current context.
    + *
    + * @param array $element
    + *   The render array to test.
    + *
    + * @return boolean
    + *   Whether the render array produced content or not.
    + */
    +function is_printable($element) {
    +  $rendered = render($element);
    +  return !empty($rendered);
    +}
    +
    +/**
    

    Adds printable test to Twig.

  3. But since every call to render() first checks for an existing cache entry, the printable test isn't even necessary.

I'm scared to see the performance penalty of md5(serialize()) on every render() call, but think a caching approach like this is probably the only way forward with what we need to do here. Tagging for needs profiling. Also, needs to pass tests. :)

chx’s picture

I think twig_render_var should set back the rendered array into $context eliminating the heavy handed static containing oh-so-much memory and a big serialization too. To do this, it needs context and name where context is easy but name is not however Cottser points out #2031311: Exception message when trying to print an object without __toString() in a Twig template is not helpful has $name solved.

joelpittet’s picture

Just a pie in the sky idea (D9 or @c4rl's improvements for OOP RenderAPI transition). If we had a __isset() magic method internally call it's render() method, and cache the result for the __toString() to use when printing, we'd have no need for twig_render_var(), and may solve this issue as well...

For implementation details, we may be able to just do a wrapper object, maybe coming out of drupal_render for now instead of a string. Would love if we didn't need to kill renderable arrays just yet, and just map the #properties to a class. Then again maybe I should just do, instead of talk... but if @c4rl you have words of wisdom that will help your transition, do share.

2c

Fabianx’s picture

That would not solve the problem as long as what is in $variables is not an object, but a render array.

Really the only way is to pass the syntax information to the twig_render_var when it is known, e.g. only for simple cases - the code at the debug issue would solve it for the name case.

It is not too difficult to parse a tree for getAttribute(getAttribute(name))), now that this is just contextual information, and then the path within $context is known and can be replaced like chx' said.

Fabianx’s picture

Implementation plan

If we really want to get:

{% if comments %}
  {% comments %}
{% else %}
  Foo
{% endif %}

working then the first thing we need to do is:

- Create our own if statement and replace twig's if with it
-- Do a drupal_render() in the if statement similar to how twig_render_var works
-- Create a unique hash of the "path" of getName, getAttribute things within the expression after the if
-- Store the hash in a recursive context using the if - depth level as recursion level

Compile the if to:

$comments_if = twig_render_var($expression);

if ($comments_if) {
// body
}

BEWARE this makes _all_ if statements slower, therefore I liked:

{% if comments is renderable } MUCH better.

-- Create our own print function
- Again keep track of immediately consecutive getName, getAttribute trees and create a hash
- If we find the same hash, replace the whole expression with one using the temporary variable similar to SetNameTmp is already used by the twig optimizer.

That would also support:

{% if node.comments %}
  {{ node.comments }}
{% else %}
  Foo
{% endif %}

but not:

{% if getVariable(node, 'comments') %}
  {{ node.comments }}
{% else %}
  Foo
{% endif %}

This overall is not an easy task, but it is doable ...

Edit:

Overall the whole idea is to do what Twig Optimize does to replace:

$this->getName('node')

with

$_node_ = $this->getName('node');

but extend this to basic attributes paths as well (no complex expressions!) and in the case of "is renderable" even render it ...

Again this could be a two step process:

- Make it work
- Make it fast

where Make it work is:

- Create an if $expression is renderable and make the is_renderable twig_render_var that thing

Make it fast is:

- Create an optimizer to replace the getName, getAttribute paths with temporary names like the $_node_, but now

node.comments becomes _node__comments_, etc.

Wim Leers’s picture

General

I read the entire issue. Teaches one quite a bit of Drupal history :)

I think JohnAlbin summed up this issue rather eloquently in #29:

This issue shows the fundamental flaw in the idea of combining show/hide/render with the desire to be able to conditional flow HTML in the template.

And I'd add to that: This issue shows the fundamental flaw in having "code-defined" HTML in Render API and "template-defined" HTML in the Theme system: it's very hard to find a good balance and there are probably always going to be TX issues. They'll always be fighting each other for more control over the conditions of rendering something. If that's vague, I can explain it one simple real-world example:

  1. One could argue that we shouldn't have Entity Displays in D8, because that "should be done in the theme layer".
  2. One could argue that we shouldn't support conditionally printing entity fields, because that "should be done in Entity Displays".

That's an everlasting tug of war, I fear. I think it mainly depends on how much we want the site builder to be able to click together, without depending on a developer or themer.

Summary of this issue

I think this issue can be summarized as the search for an answer to the question

How can we avoid double-rendering in the theme layer, because we must render to check emptiness?

… and secondary to that, depending on the solution: we want to not cause regressions in Themer Experience (TX).

Review

  1. The patch in #105 is conceptually fine. Two problems: 1) its static caching of rendered output is likely expensive, 2) if the is printable is forgotten, it still won't work.
  2. The proposal in #117 would also work fine, but is obviously quite a lot of work, fairly complex, causes special cases (see the would also support […] but not part) and potentially also expensive (the calculating of that unique hash). It does solve the problem 2 for the #105 patch rather neatly though!

Alternative proposal

I think a hybrid solution is possible here.

  • Just like in #105 and #117, we need to render a render array to know whether it's going to be empty.
  • Just like both #105 and #117, we want to avoid rendering twice. #105 does this by hashing the render array, #117 does this by looking at the "location" of the variable under consideration in the template.
    I think there's another way to achieve this, without any extra cost: stop operating on copies of the render array.

Avoiding double rendering

Changing twig_render_var($arg) so that it receives $arg by reference (and the same goes for twig_drupal_escape_filter(), which was added since the last patch). That's an easy change. But due to the current syntax of the compiled Twig templates, that won't work:

echo twig_drupal_escape_filter($this->env, (isset($context["messages"]) ? $context["messages"] : null), "html", null, true);
echo twig_render_var((isset($context["secondary_menu"]) ? $context["secondary_menu"] : null);

We can't pass values in an array directly by reference. So if we could change the above to:

$tmp = (isset($context["messages"]) ? &$context["messages"] : null;
echo twig_drupal_escape_filter($this->env, $tmp, "html", null, true);
$tmp = (isset($context["secondary_menu"]) ? &$context["secondary_menu"] : null;
echo twig_render_var($tmp);

Both twig_render_var() and twig_drupal_escape_filter() call render(), which receives its argument by reference already. render() in turn calls drupal_render() — which also already receives its argument by reference.

Combined, this means that both

{% if comments %}

and

  {% comments %}

will be operating on the same value. The first will do the rendering. The second then reuses it.

Avoiding a Twig Test (to keep the TX great)

I'd do this in exactly the same way as #117:

$comments_if = twig_render_var($expression);
if ($comments_if) {
// body
}

… but because my proposal doesn't perform the hashing, it's considerably cheaper, making the cost likely negligible.

effulgentsia’s picture

Create our own if statement and replace twig's if with it

Conceptually, +1 to this. For me, this was always one of the selling points of Twig, that we could do things like implement our own definition of what it means to evaluate a render array (or any other variable) as a boolean, rather than needing is ... modifiers. But, I'm not experienced enough with the internals of twig compilation to give a detailed opinion on the feasibility and potential side effects of this.

Changing twig_render_var($arg) so that it receives $arg by reference

That also makes a lot of sense to me, but I think we would need to make sure that:

{% if foo.bar %}
  {{ foo }}
{% endif %}

Prints the entirety of foo (not with bar stripped out since it's already been "rendered"). Unless the above is too much of an edge case to worry about.

Wim Leers’s picture

#119: Yes, anything evaluated in an "if" statement MUST NOT cause a #printed = TRUE anywhere in the render array under consideration. The use case you cite should work just fine; it'll just cause less work to happen at {{ foo }}, because its child "bar" has already been rendered (but not printed). There's nothing new there; drupal_render() already works this way (in a nutshell: recurse into children, check if rendered, if so, return early).

joelpittet’s picture

@Wim Leers just a note there is no longer show/hide in the template layer(you may already know this). #2114563: Remove TwigReference with the help of 'without' filter replacing 'show'/'hide' functions. And for D8 i'd be cool with removing show/hide all together too as it doesn't provide all that much.

Fabianx’s picture

#118: The hashing only happens during parsing time, not during compile time.

The end result is equivalent to what you wrote.

We just create a var for every: getName->getArgument->getArgument path and re-use it in the parse tree.

I missed there is a patch in #105.

Fabianx’s picture

So yes, hybrid looks good, but references won't work in twig. We barked at that tree for way too long time.

After thinking about it:

New implementation plan

- Keep it simple and make it work first (non-commitable, but testable)

a) "if is printable" will just render the array - if its a render_array - in the normal case we render twice (105 without the static cache)

- Optimize it so that all code profits from it

Similar to the TempName optimization, we can then do:

Find a path of:

print|escape|is_printable -> getAttribute->getAttribute->getName() and replace with _$attr1__$attr2__$name = drupal_escape_filter(...), do this consistently for all occurrences.

Most code will look like:

$_new_name = drupal_escape_filter($this->getAttribute($this->getName(...)));
print $new_name;

But that is okay as its not slower than what we had before ...

The only difficulty to find out is how we deal with the different paths:

is_printable uses twig_render_var
print statement uses twig_render_var(drupal_escape_filter()) - but we optimize it out to just drupal_escape_filter()
print something|raw statement uses just twig_render_var

The following holds true:

- twig_render_var(drupal_escape_filter()) == drupal_escape_filter()
- drupal_escape_filter(twig_render_var()) != drupal_escape_filter()

I don't think escaping is much important for is_printable (is_renderable?, override is_empty?), so we can probably do it or not do it without much difference as we deal with render arrays anyway and |raw does not make sense for render arrays and the worst that happens is another function call.

Therefore:

is_printable uses always drupal_escape_filter - path/hash is escape|$attr1|$attr2|$name
print statement uses drupal_escape_filter() - path/hash is escape|$attr1|$attr2|$name

print something|raw statement uses just twig_render_var, path/hash is: twig_render_var|$attr1|$attr2|$name which is okay to not match the if, because it is an edge case.

So as long as we put a escape filter in front of is_printable, we just need to check escape -> getAttr -> getAttr -> getName

As now the hash of the subpath is the same, they will end up using the same variable. (this all during token parsing time).

Therefore the task gets way simpler and all we need is:

a) a second node visitor that runs after the one we have right now (no good idea to mix them)

b) Some nice recursion rules, I suggest to look into how the temp name optimizer does it within Twig.

c) The first part to make it work and add tests.

It is doable and no longer _that_ complicated! :)

Fabianx’s picture

Brainstorming some more:

- Lets assume we go to the whole token tree twice (instead of once) and we keep state somewhere (e.g. in the ROOT of the token tree)
- Lets also assume we do this before any other optimization takes place (for now)

* Find all print / render or if statements, e.g. {{ comments }}, {{ comments.sub1 }}, {% set x = render(comments.sub2) %}, {% if comments.sub1 %}

Note down the paths, this is a little tricky within the recursion, but the end result looks like

$paths['comments']['#render'] = TRUE;
$paths['comments']['sub1']['#if'] = TRUE;
$paths['comments']['sub1']['#render'] = TRUE;
$paths['comments']['sub2']['#render'] = TRUE;

$paths['something_else']['#if'] = TRUE;

So our tmp names are:

$_comments
$_comments_sub1
$_comments_sub2

$_comments_render
$_comments_sub1_render
$_comments_sub2_render

but not $_something_else, because it does not have a render statement for that would need to use set directly.

- We cannot blindly render those at the start, because they might be in different if contexts, e.g. sub2 is never rendered

But the first time e.g. $_comments_sub1 is used we instead do (because we know there is other stages we need to render:

$_comments_render = $context['comments'];
$_comments = drupal_escape_filter($_comments_render); // by reference, @todo will set #printed, needs to be reset somewhere
$_comments_sub1_render = $this->getAttribute($_comments_render, 'sub1'); // this is already render array, but not returned by reference
$_comments_sub1 = drupal_escape_filter($_comments_sub1_render); // by reference, but we lost the #printed meaning

if ($_comments_sub1) {
  print $_comments;
}

$_something_else = isset($context['something_else']) ? $context['something_else'] : NULL;

if ($_something_else) {
  $_comments_sub2_render = $this->getAttribute($_comments_render, 'sub2'); // this is already render array, but not returned by reference
  $_comments_sub2 = drupal_escape_filter($_comments_sub2_render); // by reference, but we lost the #printed meaning
  print $comments_sub2;
}

This would work, but the problems arise for cases like:

{% if cond_A %}
  {{ comments.sub1 }}
{% endif %}
{% if cond_B %}
  {{ comments.sub2 }}
{% endif %}
{% if cond_C %}
  // No comments printed
{% endif %}

Where do you put the $_comments_ = thingy.

Could potentially solve with if (isset()) {, which is cheap, which means code looks like:

if (!isset($_comments_render)) {
  $_comments_render = $context['comments'];
  $_comments = drupal_escape_filter($_comments_render); // by reference, @todo will set #printed, needs to be reset somewhere
}

if (!isset($_comments_sub1)) {
  $_comments_sub1_render = $this->getAttribute($_comments_render, 'sub1'); // this is already render array, but not returned by reference
  $_comments_sub1 = drupal_escape_filter($_comments_sub1_render); // by reference, but we lost the #printed meaning
}

if ($_comments_sub1) {
  print $_comments;
}

$_something_else = isset($context['something_else']) ? $context['something_else'] : NULL;

if ($_something_else) {
  if (!isset($_comments_sub2)) {
    $_comments_sub2_render = $this->getAttribute($_comments_render, 'sub2'); // this is already render array, but not returned by reference
    $_comments_sub2 = drupal_escape_filter($_comments_sub2_render); // by reference, but we lost the #printed meaning
  }
  print $comments_sub2;
}

In case {{ comments is never printed however, this would look like:

if (!isset($_comments_sub1)) {
  $_comments_sub1_render = $this->getAttribute($context['comments'], 'sub1'); // this is no render array and not returned by reference
  $_comments_sub1 = drupal_escape_filter($_comments_sub1_render); // by reference, but we lost the #printed meaning
}

if ($_comments_sub1) {
  echo "// Not printing comments";
}

$_something_else = isset($context['something_else']) ? $context['something_else'] : NULL;

if ($_something_else) {
  if (!isset($_comments_sub2)) {
    $_comments_sub2_render = $this->getAttribute($context['comments'], 'sub2'); // this is already render array, but not returned by reference
    $_comments_sub2 = drupal_escape_filter($_comments_sub2_render); // by reference, but we lost the #printed meaning
  }
  print $comments_sub2;
}

So we would be always searching for the "nearest" parent that is known in the context, other example:

{% if cond_A %}
  {{ comments.sub1 }}
  {{ comments }}
{% endif %}
{% if cond_B %}
  {{ comments.sub2 }}
{% endif %}
{% if cond_C %}
  // No comments printed
{% endif %}

which means the parent searching code needs to be dynamic, which makes our example look like:

if (!isset($_comments_render)) {
  $_comments_render = $context['comments'];
  $_comments = drupal_escape_filter($_comments_render); // by reference, @todo will set #printed, needs to be reset somewhere
}

if (!isset($_comments_sub1)) {
  if (isset($_comments_render)) {
    $_render_parent = $_comments_render;
  } else {
    $_render_parent = isset($context['comments']) ? $context['comments'] : NULL;
  }
  $_comments_sub1_render = $this->getAttribute($_comments_render, 'sub1'); // this is already render array, but not returned by reference
  $_comments_sub1 = drupal_escape_filter($_comments_sub1_render); // by reference, but we lost the #printed meaning
}

if ($_comments_sub1) {
  print $_comments;
}

and here it starts to get real complicated ...

I am still for #123 for now with is_printable, what we have here is very similar to the fast node visitors that no one could get to work already, which would have looked syntax wise like:

$_comments_sub1 = drupal_escape_filter($this->getRenderedPath($context, 'comments', array('sub1'))); // This is rendering on the fly like drupal_render - if its an array() - it is optimized for render arrays but falls back to $this->getAttribute or all other cases. It internally uses references.

if ($_comments_sub1) {
  print drupal_escape_filter($this->getRenderedPath($context, 'comments')); // sub1 will not be rendered twice here
}

$_something_else = isset($context['something_else']) ? $context['something_else'] : NULL;

if ($_something_else) {
  print drupal_escape_filter($this->getRenderedPath($context, 'comments', array('sub2'))); // Comments has already been rendered, so this just returns what has already been printed
}

As we hold render_state within $context this is similar to per reference, but does not need any changes to the drupal_escape_filter, etc. It is likely even quicker than default TWIG as it avoids the function chaining.

I may have failed with that approach in the other issue from hell, because:

a) the syntax was brain-dead - what is here is much nicer
b) I tried to cover all possible cases instead of just for the standard cases getAttr -> getAttr -> getName()

The syntax of this is much nicer anyway and it allows more inline templating, etc. in easier ways.

However we already have a twig optimizer which works great, so lets try again to match that syntax:

$_comments_ = isset($context['comments']) ? &$context['comments'] : NULL; // Gimme a reference!
$_comments_sub1 = drupal_escape_filter($this->getRenderedPath($_comments, NULL, array('sub1'))); // This is rendering on the fly like drupal_render - if its an array() - it is optimized for render arrays but falls back to $this->getAttribute or all other cases. It internally uses references.

if ($_comments_sub1) {
  print drupal_escape_filter($this->getRenderedPath($_comments, NULL, array())); // sub1 will not be rendered twice here @todo can optimize that out
}

$_something_else = isset($context['something_else']) ? $context['something_else'] : NULL;

if ($_something_else) {
  print drupal_escape_filter($this->getRenderedPath($_comments, NULL, array('sub2'))); // Comments has already been rendered, so this just returns what has already been printed
}

So, yes this works nicely, use a different function name and

Given all that and that I think the complexity of above twig output is not more than what we had before I suggest for this issue:

Another new implementation plan

a) Get the simple case to work first: Have the double drupal_escape_filter for if is_printable (baby step); add test coverage

b) Create a node visitor that does the following and runs after the optimizer, etc.:

- Search for straight paths of print|escape -> getAttr -> getAttr -> getName or getTmpName with and replace with a getRenderedPathNode: name => $name, attr => array() OR getRenderedPathNode: name => NULL, attr => array() but replace getTmpName node with getReferenceTmpName (optimized version)

- Add a compiler for the getRenderedPathNode and add the function itself to TwigEnvironment

The function should look like:

function getRenderedPathNode(&$context, $name, $path) {

if ($name == NULL) {
  $var = &$context;
} else {
  $var = &$context[$name]; // @todo add isset() check
}

for ($path as $attribute) {
  if (is_array($attribute) && isset($var[$attribute])) {
    $var = &$var[$attribute];
  } else {
    $var = $this->getAttribute($var, $attribute);
  }
}

if (is_array($var)) {
  drupal_render($var); // This does all the magic, but as this is only used for printed things we would have called drupal_render anyway in the escape filter.
}

return $var;

And this task now should be simpler.

So yeah, above syntax and approach are my recommendation. Will even gain some speed overall as getAttribute is rather slow, though need to check native twig extension.

Wim Leers’s picture

#121: Yes, I know show() and hide() are no more :)

#122: I didn't realize that the hashing would only happen while compiling into a template, and not during execution (that's what you meant in #122, right?) — that's definitely encouraging :)

#123:

[…] but references won't work in twig […]

Why not? :( Note that we don't need references to bubble up all across Twig, we just need references to work within one template, which is much more doable.

#123/#124:
I can't say I fully understand your proposals in #123/#124, but I think a patch might help. You seem very confident that this would work, and if it's much simpler, then: great!.

Fabianx’s picture

#125:

I will only have time to work on a patch in a month or two. I'd be happy to assist anyone that tries to implement that though.

- References: #124 would be a possible solution, the other thing is more complicated due to getAttribute() handling - trust me, I tried almost everything for that already ...

To #124:

The TL;DR idea is to convert from:

drupal_escape_filter($this->getAttribute('sub1', $this->getContext($context, 'comments')));

TO

drupal_escape_filter($this->getRenderedPath($context, 'comments', array('sub1'));

and drupal_render the result before passing back to drupal_escape_filter().

The rest is an implementation detail and further optimization can be done then.

and from

{% if comments.sub is printable %}
{{ comments }}
{% endif %}

which currently would be:

if (is_printable_value($this->getAttribute('sub1', $this->getContext($context, 'comments')))) {
  print drupal_escape_filter($this->getContext($context, 'comments'));
}

to

if (twig_is_printable_value($this->getRenderedPath($context, 'comments', array('sub1'))) {
  print twig_drupal_escape_filter($this->getRenderedPath($context, 'comments', array()));
}

which solves this whole thing elegantly. is_printable can still call drupal_render, but as its already rendered, that case is optimized and drupal_escape_filter also only renders the non-rendered parts.

Edit 2:

For the without filter its a little tricky as we don't want to render things that are without-ed, e.g.

{{ comments.sub2|without('x','y','z') }}

but my proposal for that is to include that in the getRenderPath function as a new argument and special case that:

print twig_drupal_escape_filter($this->getRenderedPath($context, 'comments', array('sub2', array('x','y','z')));

Though we need to save the without'ed elements, remove them and re-set them after the rendering ... (which is do-able)

Any more complicated constructs are just not supported ...

Edit:

That all said this issue could well only deal with the is_printable filter for now and deal with the optimization in a major / critical follow-up perhaps to split things more?

is_printable == takes a reference, can be used from PHP Templates and does not need to be optimized as drupal_render is called internally and keeps track of what has been rendered already. (Great idea, Wim! - We could have had that already in D7 ...)

twig_is_printable == is_printable, but takes a value instead, caller takes care of rendering, in non-optimized cases things are rendered twice.

I think thats a good strategy to move forward, #125 what do you think?

Fabianx’s picture

Issue summary: View changes
Fabianx’s picture

Issue tags: +Novice
function is_printable($element) {
$rendered = render($element);
return !empty($rendered);

This needs some more logic and some changes and should look like:

function is_printable(&$element) {

if (!empty($element['#printed'])) {
  return FALSE;
}

// Do not change #printed state
$rendered = drupal_render($element);
return !empty($rendered);
}

Copy the same logic to twig_is_printable, but without the reference.

Use twig_is_printable in getTests() instead of is_printable.

Remove the cache from render().

- Then first part of this issue is done! :)

Tagging novice for this part of the task, remove again once this is implemented; might also need a re-roll.

c4rl’s picture

Issue tags: -needs backport to D7, -Novice

I have an alternative suggestion/modification. Throughout this issue, the onus of whether $variable can be printed or not relies exclusively on whether output of drupal_render($variable) is non-empty, and once again (as we saw in #2099131: Use #pre_render pattern for entity render caching) puts a lot of reliance on the internals of drupal_render() for the sake of performance.

Optimization here is (likely) dependent on caching, and potentially leads to problems, say if is_printable($variable) is called before being used in a template and the $variable is altered in some way, etc.

As I've stated, I think the future of better & smarter rendering does not lie in the internals of drupal_render() but rather changing the way the renderables themselves work and behave.

So here's alternative suggestion: put the onus of whether something is printable on the renderable itself, not on whatever drupal_render() will produce.

With PHP5.3 we can use closures to have a renderable element determine if itself should be rendered or not, by simply passing itself to the closure:


// In page callback...
return array(
  '#theme' => 'foo',
  '#var1' => something_unpredicable(),
  '#printable' => function($self) {
    return !empty($self['#var1']);
  },
);


// In API...
function is_printable(&$element) {
  if (isset($element['#printable'])) {
    // If closure exists, simply evaluate.
    return $element['#printable']($element);
  }
  elseif ($keys = element_children($element)) {
    // If the element has children, delegate to logical `OR` of child
    // elements.
    $printable = FALSE;
    foreach ($keys as $key) {
      $printable = is_printable($element[$key]) || $printable;
      if ($printable) {
        break;
      }
    }
    return $printable;
  }
  else {
    // Assume printable otherwise.
    return TRUE;
  }
}

In (compiled) template...

<?php if (is_printable($variable)): ?>
  <h1><?php print render($variable); ?></h1>
<?php else: ?>
  <h1><?php print t('No data'); ?></h1>
<?php endif ?>

Generally, renderables *should* know pretty well based on their parameters whether to be printed or not.

**PROS:**

* Avoids delegating to drupal_render() and the theme system altogether.
* Puts more power in building a renderable array.
* Allows is_printable() to be called before possible alterations to the $variable.
* Looks more like a method on an object :)

**CONS:**

* Technically, yes, a theme function could simply return a blank string that would contradict the results of the closure, but this seems like an edge case, and feasibly fixed if the #printable parameter is pluggable by that theme.
* Another property in a renderable array (though, in my opinion, justifiable if it lessens reliance on drupal_render() to solve everything).
* Won't work with PHP5.2, so no backport.

Thoughts? :)

c4rl’s picture

Also, is_printable() could look for default closures for given #theme/#type via a registry of those implementations. So, one wouldn't need to define #printable in every circumstance; #theme => table implementation or #theme => item_list would have defaults.

// In API...
function is_printable(&$element) {
  if (isset($element['#printable'])) {
    // If closure exists, simply evaluate.
    return $element['#printable']($element);
  }
  elseif ($printable = get_printable_from_registry($element)) {
    return $printable($element);
  }
  elseif ($keys = element_children($element)) {
    // If the element has children, delegate to logical `OR` of child
    // elements.
    $printable = FALSE;
    foreach ($keys as $key) {
      $printable = is_printable($element[$key]) || $printable;
      if ($printable) {
        break;
      }
    }
    return $printable;
  }
  else {
    // Assume printable otherwise.
    return TRUE;
  }
}
Fabianx’s picture

Issue tags: +needs backport to D7, +Novice

This is an interesting suggestion, and it is not limited to PHP 5.3.

#is_printable => TRUE / callable / FALSE could be supported with both a closure and non-closure.

And that problem is pretty much solved, cause I totally agree. Every render stage should know if its printable.

However the effort to go through the stages might be difficult, e.g.

A region with three blocks.

How would a block know if its printable without printing?

That goes back to if the whole render tree is pre-rendered (#2273277: Figure out a solution for the problematic interaction between the render system and the theme system when using #pre_render) in advance, however if pre_render has been called already, yes a block would know if its printable or not. That recursive approach might even know what is printable and what not already, so could set that property while traversing the whole array.

So if render elements can set that, then yes e.g. for blocks in a region (one of the main things for this) this could work.

If we don't have the full recursive render tree, then nothing about the block is known and there is just:

#pre_render => array('block_render') in the render array.

and while setting a #is_printable callback as a property there would still work, it would still duplicate the work of #pre_render so would need internal caching, etc.

joelpittet’s picture

Well I just spent the last hour writing something long and thoughtful... and something happened and poof work gone... argh!

Ok coles notes...

We can do this now (no work, no changes).

// D7
if ($region = render($page['region'])):
  
$region
endif;
{# D8 #}
{% set region = page.region|render %}
{% if region %}
  
{{ region }}
{% endif %}

We are proposing (keep in mind for template designers)

{# D8 #}
{% if region is renderable %}{# Note: not "is printable" that doesn't make sense because you can "print" empty values. #}
  
{{ region }}
{% endif %}

Which is still not the ideal we're looking for:

{# D8 #}
{% if region %}
  
{{ region }}
{% endif %}

And they still need to know that the variable region is a renderable array for when to apply that Drupalism test "is renderable".

And we, building that test, need to either A) pre render and cache the value for it's eventual result reuse OR B) Try a lighter but still possibly heavy recursive tree check to see if there are values available to render.

And we need to know that that array is a special flower and not an normal array which just needs to be countable.

And we need to potentially subvert getAttribute which could mean a double hit to your performance when twig.c is trying to keep things fast.

I strongly believe, unless we go all out for the ideal:

{# D8 #}
{% if region %}
  
{{ region }}
{% endif %}

Then all this work is a LOT of backend work for a slower, more complicated drupalism tradeoff. #PoorROI

And as it stands I'd recommend documenting the render(var)/var|render as the recommended approach and deal with this problem in OOP(D9) where we can gain that ideal template syntax and not put burden and bulk into our theme layer to accomplish a half measure.

And last and likely least:

{# D8 #}
{% if region is not empty %}
  
{{ region }}
{% endif %}

= function twig_test_empty() may be a more accurate test to augment if we still want to put effort into this issue for D8. You don't need to change NodeVisitors or change how the if statements are compiled, it's more accurate to what you are actually trying to do anyways (test if value is not empty).

But still... double hit to drupal_render if you call that and without serious code contortions that would need to be undone for the OOP panacea later...

Cottser’s picture

Issue tags: -Novice

I don't think there's a clear path here for a novice yet, and I don't suspect there will be anytime soon, untagging.

Fabianx’s picture

Assigned: Unassigned » Fabianx

Because we are no longer micro optimizing and if we ever want to use strict mode we have that additional function call anyway, I give the all out solution:

If whatever not empty

A shot and overriding that filter.

What are two or three function calls :-D

While for most render arrays it would even be faster.

Wim Leers’s picture

Did some words get filtered away from #134? :)

Jeff Burnz’s picture

Posted issue #2355431: Breadcrumb block breaks layout I assume this is tightly related to this issue.

dawehner’s picture

Can someone update the IS and explain what needs/should be done here?

Maybe @fabianx and @chx could find someone on the friday critical hour, to help with this issue.

xjm’s picture

webchick’s picture

Priority: Critical » Major
Issue tags: -Needs D8 critical triage

The core maintainers are spending some time today triaging the D8 critical queue.

While this is definitely extremely frustrating for themers, bear in mind the definition of "major" in https://www.drupal.org/node/45111:

"Issues that have significant repercussions but do not render the whole system unusable (or have a known workaround) are marked major."

This issue didn't block the release of Drupal 7, so it's unclear why it should block the release of Drupal 8 unless there is truly no workaround for this issue in D8. If that's the case, we need specific steps to reproduce in order to escalate it back to critical.

effulgentsia’s picture

This issue didn't block the release of Drupal 7

By this standard, I agree with this being Major rather than critical.

unless there is truly no workaround for this issue in D8

I'm not sure if that is the right metric though. Because, in some sense there is no workaround in either D7 or D8. See issue summary and its reference to #20. In both D7 and D8, we have core and contrib themes adding CSS classes like layout-one-sidebar and layout-two-sidebars, there are numerous ways to reproduce situations in which those classes get added incorrectly (e.g., per #136, put a block in a sidebar that renders as empty content), and we have core CSS which targets those classes and therefore applies incorrect styling. Per the issue summary, various partial workaround exist, but they all have their flaws, so in practice, this bug is affecting real sites.

However, AFAIK, the corresponding layout bugs themselves (e.g., #2355431: Breadcrumb block breaks layout) aren't themselves critical, because it doesn't actually break the ability to use the site, it just gives you a layout that would have been appropriate if some content existed in a block, and so without that content, you get some undesired empty space, but that's it. Given that D7 production sites have this same problem, I don't see a justification for blocking D8's release on this, though of course, I'd love to see this fixed regardless of its priority.

About the only thing I can think of that would justify re-raising this to critical is if the layout regressions have gotten substantially worse in D8, for example, if it severely impacts responsive design in a way that wasn't a consideration for D7 core themes.

effulgentsia’s picture

Issue tags: +blocker
Related issues: +#2355431: Breadcrumb block breaks layout

Adding the "blocker" tag, since this does block resolving issues like #2355431: Breadcrumb block breaks layout. Hopefully, that will also help keep this issue sufficiently visible despite its lowered priority.

Jeff Burnz’s picture

@ #140, there are some workarounds for D7 that work acceptably, however in D8 other than using drupal_render() there is no way I have found around this bug.

Is there anything against us adding a render filter to twig so we can do #132? I know this is not perfect but at least if gives us one workaround so we can actually get control over the layout when we really really need to.

Fabianx’s picture

In Drupal 8 you can use:

{% set output = render_var(content) %}

While using a function is less convenient than a filter, it works today as that is how the 'automagic' printing works internally.

I have not yet given up on this issue ...

Wim Leers’s picture

tanc’s picture

I've also just encountered this issue whilst theming a D8 site. Using the technique joelpittet mentioned in #132 kind of works in that its possible to render the region and check for output in the Twig template. As I'm relying on body classes I've switched from Bartik's method of adding body classes in hook_preprocess_html and instead added a render check in html.html.twig like so:

{%
  set body_classes = [
    logged_in ? 'user-logged-in',
    not root_path ? 'path-frontpage' : 'path-' ~ root_path|clean_class,
    node_type ? 'node--type-' ~ node_type|clean_class,
    page.sidebar_first|render ? 'layout-one-sidebar layout-sidebar-first',
  ]
%}

This works ok when Twig debugging is turned off but otherwise the comments are picked up and the column and body classes are rendered. For now this is fine but the ideal would certainly be if this happening higher up the stack so a simple {% if region %} check would work.

joelpittet’s picture

Added a |render filter #2447049: Add a render filter to twig @Jeff Burnz

Jeff Burnz’s picture

@joelpittet awesome, thanks for the heads up, I can put that to use right away :)

Fabianx’s picture

We might need to won't fix that for everything except simple cases and the reason is #post_render_cache, where the visibility is determined at the last point.

For example the messages block is visible always, but just outputs a placeholder. which is then replaced with #post_render_cache at the last point when all messages have been output already.

There is 2 remedies if we really need to add those classes and there is no new CSS approach to determine if something is empty:

- DOM level processing to add the classes (Drawbacks: Costly, not compatible with AJAX, etc.)
- JS processing (Drawbacks: Flash of "Unstyled" content or JS in the header or just determined after the AJAX request)

Therefore question to all themers following this:

Are there any new CSS3 selectors to reliably determine if an empty region should be shown?

It is no problem to add e.g. a class 'dynamic-region' when there is #post_render_cache or placeholders present, but the 'one-sidebar', 'two-sidebars' etc. classes would be best deprecated as it will be hard to determine if a placeholder contains content or not before rendering the theme.

They could still work for certain cases, but we need different strategies for other cases.

markcarver’s picture

Are there any new CSS3 selectors to reliably determine if an empty region should be shown?

Technically, yes... there is the CSS3 pseudo selector :empty. However it behaves a little unexpectedly (from what some people may think). Basically, what it boils down to is: it's sensitive to whitespace. We'd have to trim any and all whitespace from the content as well (including line breaks \n).

I've created a JS Bin here to show an example of how :empty actually works:
http://jsbin.com/retisoruto/3/edit?html,css,output

IE has support for :empty in versions 9 and above (which is fine), but the one caveat using this may be how a browser actually interprets :empty (I only tested the above JS Bin in chrome).

If we do happen to go this route, I would suggest we use the class rendered-content; this issue isn't restricted to just regions, but all rendered content:

.rendered-content:empty {
  display: none;
}
markcarver’s picture

Issue summary: View changes
FileSize
43.16 KB

Here's a screenshot of what the above JS Bin should look like:

dawehner’s picture

When I talked with @amateescu today we thought that we should simply add a | trim call into the page HTML templates for regions.

derheap’s picture

I ran into a (maybe) relateted issue with fields:

#2478077: Markup (and label) of empty fields get rendered

Wim Leers’s picture

Related, possibly useful notes, for checking whether a render array may render into something or not:

      // If the content consists only of cacheability metadata, then don't
      // render the block wrapper, but an empty string with that cacheability
      // metadata.
      if (count($content) === 1 && array_keys($content) === ['#cache']) {

and:

      // When there are no nested children, nor #markup, nor a #type or #theme
      // that may be rendered, then this block can never be rendered into
      // anything.
      if (!Element::children($content, TRUE) && !isset($content['#markup']) && !isset($content['#type']) && !isset($content['#theme'])) {

(where $content is a render array)

Wim Leers’s picture

The code in #153 is in core as of #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees:

class Element {

  …

  /**
   * Indicates whether the given element is empty.
   *
   * An element that only has #cache set is considered empty, because it will
   * render to the empty string.
   *
   * @param array $elements
   *   The element.
   *
   * @return bool
   *   Whether the given element is empty.
   */
  public static function isEmpty(array $elements) {
    return empty($elements) || (count($elements) === 1 && array_keys($elements) === ['#cache']);
  }

}
joelpittet’s picture

@Wim Leers hmmm, maybe an 'is_empty' => callback with a default callback?

Wim Leers’s picture

#155: I have no idea what you mean :) #154 == Element::isEmpty().

joelpittet’s picture

@Wim Leers maybe I don't understand what you are getting at then but Element doesn't get sent to the twig template, it just builds a renderable array, didn't realize at first that it was static so that can be used as the 'default' but the issue is around the template not knowing if a renderable array is "actually empty" and sometimes that check isn't good enough. the Field API has a nice method that can be overriden for it's "empty" state for when a field has multiple values or what ever other use-case where the isEmpty doesn't cut it the callback can be replaced.

So I'm suggesting maybe that callback property on renderable arrays that twig can invoke when a renderable array is in a condition. Or am I still crazy? But also it will still have to traverse children... argh that sounds slow (close won't fix is on my mind)

alexej_d’s picture

@Wim Leers I just wanted to throw in that Element::isEmpty() does not always work. Sometimes an element can be empty but has a weight – the test fails in that case.

EclipseGc’s picture

Also, as I originally reported, #access => FALSE on a lone block in a region causes the region to render, even though we don't have access to the underlying rendered output.

Eclipse

samuel.mortenson’s picture

Another example of this issue is checking if a region contains a cached block, like the status_message block. The block has the keys #cache and #lazy_builder set, so it is not covered by the isEmpty() call. When the block is actually rendered, it contains no content. What's the right way to check if a region is empty in a case like this?

Wim Leers’s picture

#160 brings up a good point and shows the fundamental problem at play here.

Drupal in general, but Drupal 8 even more so than previous versions, tries to do as little work as possible. That means calculating (and rendering) things only when we know they're going to be needed. That means lazy evaluation: only calculating things when they are actually necessary.

Applied to this issue that means that we only try to render the contents of a region when it is accessed. That does not currently happen. So there's only one possible conclusion: it is broken currently.

We could fix it, by doing the rendering of a region's contents when checking if a region is empty. But… Drupal 8 also has the concept of "placeholders". Their whole point is that when they're rendered initially, some placeholder HTML is generated, to allow large parts of the page (or even the entire page) to be cached across users. The placeholder HTML is then replaced just before sending the response to the end user.

In this environment, it truly is impossible to know whether a region is empty or not.

So… AFAICT the only solution to this problem is to never have conditionally appearing regions; regions must be assumed to be always present, they just may end up containing nothing.

Fabianx’s picture

#161: Yes, our only sane option is #149 / #150.

I think to trim whitespace from our output and using :empty is likely the way to go there except for JavaScript ...

MustangGB’s picture

I don't find CSS a very good solution, it should be possible to not transmit the unwanted extra HTML.

markcarver’s picture

I too am not really a fan of the "CSS solution".

However, given the reality of caching and placeholders in D8, there's really no other "easier" solution around this issue.

Sure, we could implement a much more complicated JS solution... but I would argue that would introduce rather unnecessary DOM parsing/manipulation (performance impact, because CSS can do this natively for us and is a separate engine from JS).

The simplest way is to just trim content provided by the back end (whenever it is ultimately rendered) and then use native CSS3 :empty pseudo selector to determine visibility on the front end.

mdrummond’s picture

Not being able to detect if a region has content or not is a serious flaw. Lots of empty divs on the page because we can't detect if a region has contents or not seems like a big problem to me, even if we can cover it up with CS. I understand we might not be able to fix this now. Is there a long-term fix for this? In the magical future where we have an OO render API, could this be fixed?

Fabianx’s picture

#165: No, unless you mark all of those blocks as un-placeholderable OR if you ensure that blocks are only empty if access is denied (and hence not even the placeholder is created).

The point is that things like regions, etc. do no longer exist as clear boundaries, a page is composed out of dynamic and static parts.

The example is:

You have a left sidebar, there is a placeholder for a block, if that block can be sometimes "empty" and sometimes not empty, there is no way for the sidebar to know this as just when the block is finally rendered, which can be as late as happening just on the users computer within the browser, then you can ensure empty-ness or non-emptyness.

TBH, I am also not a fan of a CSS or JS solution and I think frankly best would be if there was an extended CSS filter, which looked for !empty(), where whitespace is stripped automatically.

But that is overall not a new problem and most sites can use a JS solution fine.

This all is mitigated by the fact that usually a block is not just empty, but only empty / non-existing when its access callback says 'access denied', which also works for the visibility rules.

markcarver’s picture

Is there a long-term fix for this? In the magical future where we have an OO render API, could this be fixed?

AFAIK, not specifically. This has always been an issue due to how convoluted the theme system/render "API" has been (e.g. data can be thrown in at any time).

I honestly don't think this issue could ever be 100% "fixed" on the back end for 8.x (or previous versions) due to this lack of consistency, hence the CSS solution.

I think the "long-term fix" for this is really just obliterating the arrays of doom ("#whatever_you_want_here") and thus wouldn't have to worry about what might or might not be necessary to render data. That is, of course, 9.x though.

Having the theme system/render "api" become more OO would most likely allow us to look ahead/upwards (bubbling) in the render process too; something that is nearly impossible to do currently given the fact that we still rely on these arbitrary arrays.

Wim Leers’s picture

AFAIK, not specifically. This has always been an issue due to how convoluted the theme system/render "API" has been (e.g. data can be thrown in at any time).

Exactly!

I think the "long-term fix" for this is really just obliterating the arrays of doom […] Having the theme system/render "api" become more OO would most likely allow us to look ahead/upwards (bubbling) in the render process too; something that is nearly impossible to do currently given the fact that we still rely on these arbitrary arrays.

No; we already have bubbling.

The problem is that the desire to check if something is empty is incompatible with placeholders. Placeholders allow us to cache large parts of the page (or even the entire page) despite personalized/dynamic content still needing to be calculated for the current user/current time/current whatever. But because we have those placeholders, we end up with HTML like:

<html>
<regionA><block1><block2></regionA>
<regionB><block3><PLACEHOLDER></regionB>
<regionC><PLACEHOLDER></regionC>
</html>

i.e. some regions are guaranteed to not be empty, the problem discussed on this issue does not exist in those cases (regions A and B). But for others, we will not know until we do the final rendering.

I agree that this is annoying. But it is not quite as bad as it looks at first sight, because:

  • regions that don't contain any blocks still are not rendered
  • regions whose blocks we know for certain (because it's the same across all users, think for example an empty Views block) will be empty also are not rendered
  • regions that contain at least one block that will be visible to all users are necessary anyway
  • so only regions where all blocks are placeholders because they are so dynamic/personalized that they cannot be cached across users are where this problem manifests itself

Finally: please note that in advanced HTML delivery mechanisms, such as #2469431: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts BigPipe, it would literally be technically impossible to not render the region's HTML, because the HTML for the placeholders in that region are rendered on the server and sent to the client after the initial page is delivered. So, in a way, this helps ensure that Drupal 8 sites are built in a way so that their CSS continue to works just fine when they enable the BigPipe D8 contrib module to make their sites faster — see #2469431: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts.

I am therefore quite hopeful we'll collectively find different strategies to deal with this in different situations, so that it doesn't get in the way too much.

Jeff Burnz’s picture

Category: Bug report » Feature request

only regions where all blocks are placeholders because they are so dynamic/personalized that they cannot be cached across users are where this problem manifests itself

So how to deal with these types of blocks? We have issues like #2614954: Empty highlighted region returns whitespace which will always evaluate to TRUE when using twig IF , this is because of the messages block. Very bad if such blocks are moved into a sidebar type region, the layout is then broken etc.

tim.plunkett’s picture

Category: Feature request » Bug report

Still a major bug.

Fabianx’s picture

#169:

- You can render the region in twig using the |render filter, then making sure to use the same pre rendered region variable again. That won't work for messages obviously.
- You can use Javascript.
- You can use CSS with :empty().
- You could implement your own late EventSubscriber as a Response Subscriber and re-write the response using either a DOM manipulation library or regexp.

There is no real generic solution possible / known at the moment - sorry.

samuel.mortenson’s picture

Most people in this issue are probably looking for :blank, not :empty. Twig templates spit out tons newlines and spaces, which will cause the :empty check to fail. If you really love :empty you'll have to add {% spaceless %} to your templates or do some JS trickery to remove new lines.

Fabianx’s picture

#172: Unfortunately :blank is only a CSS selectors 4 draft at the moment with almost no browser support and only supported in Mozilla as :moz-whitespace-only, but yes, that is exactly what we would need.

davidhernandez’s picture

I think it will be helpful to have a best practice code snippets for doing it with javascript and with |render. That is the solution a lot of themers will navigate to, to just "make it work." And JS is what we were about to do with Bartik just before release when the problem came up. How costly is the |render method?

samuel.mortenson’s picture

If it helps, this is what I have to do in Javascript after removing any element, to keep my :empty selectors working on regions. $region.html($region.html().replace(/\n/g, ''));

Jeff Burnz’s picture

Thanks guys, much appreciated, however removing empty markup is not my major issue, I want to be able to set layout classes, i.e. to replicate, reliably, Drupals collapsible region capability. This is now broken and, IMO, a major regression, yes that is a bug, but also afaict a deliberate direction Drupal core has taken. The "feature" would be a way around this in PHP that all themers could use reasonably easily.

What I'm going to try now is setting the layout classes with JS, not just removing empty markup, I don't really care about empty markup - what we need to be able to do is set layout based on the regions that have printed, I think JS is the only way to go at this stage. It's doable, but I'm concerned about performance which is why I am avoiding this approach until all else fails :)

davidhernandez’s picture

Check nod_'s last patch in the Bartik issue. #2529308: bartik_preprocess_html() only works correctly without lazy rendering, without placeholders, without cacheability metadata for access checks It has the JS that was almost added to make the one-sidebar, two-sidebar, etc classes work.

Fabianx’s picture

#176: Also as written in the other issue, you can use: display: flex and :empty easily to create the same collapsible effect.

If you really need classes / attributes to be set on the regions, the only way is to do it way after all the HTML is rendered via Html DomDocument parsing or indeed via Javascript.

Jeff Burnz’s picture

I've done some testing with flex, I need to look into it more, I do not see a way of setting width without knowing what is printing, again this has to be JS solution afaict in D8. We use all pseudo classes for layout and need to know what is printing so we can set specific widths on regions depending on how many are printing in a row (a row is a group of regions in a wrapper). Flex is also expensive to implement compared to float, not only in strait code conversion but massive testing, not sure we can afford that at this stage.

Thanks davidhernandez for the link, much appreciated.

Fabianx’s picture

Just to share how I did it with :flex and :empty() - yes need to ensure sidebars use spaceless and no further region markup inside and just the blocks, but then it works:

.layout-main {
  display: flex;
}

#sidebar-first {
  order: 1;
  width: 25%;
}

#content {
  flex-grow: 1;
  order: 2;
}

#sidebar-second {
  order: 3;
  width: 25%;
}

#sidebar-first:empty, #sidebar-second:empty {
  width: 0%;
}

And thats it - if you ensure that there is no whitespace for the regions - which can be achieved using twig's trim filter and overriding templates as usual.

See more:

https://css-tricks.com/snippets/css/a-guide-to-flexbox/#flexbox-examples

for how to combine that with nicely responsive layouts.

Edit: And as long as you don't have any placeholdered blocks in there, this will obviously also work without the :empty as if the #sidebar-left or #sidebar-right are not present at all, the flex-grow:1 will nicely ensure that the main content uses the full width available.

markabur’s picture

Flex might be ok for adjacent columns in certain situations, but what about stacked regions?

E.g. I have a D7 project now that assigns "with-subhead" and "without-subhead" classes based on whether the subhead region is empty, and changes the top margin on the main content area based on that. In D8, from what I gather, the only way to do this is with Javascript?

samuel.mortenson’s picture

@markabur You can either use Javascript or replace the instances of without-subhead in your CSS with ":empty". For example you could just have a "subhead" class, then target rules with ".subhead:empty".

dcrocks’s picture

Flex works equally well with a vertical as well as a horizontal orientation. You just specify a different baseline.

Jeff Burnz’s picture

Edit post: my code changed a lot so this was no longer relevant.

Jeff Burnz’s picture

Looks like another contrib related issue, the user thinks there should be breadcrumbs because all the wrapper code is printing. So here we have a site builder experience issue.

Any tips helping with my JS approach above would be welcome, of course it works but it's slow and creates flicker as the layout classes apply a 10th of second after the document is fully loaded. Oddly in Safari once the page is cached there is no flicker, but every other browser there is.

markabur’s picture

You can either use Javascript or replace the instances of without-subhead in your CSS with ":empty". For example you could just have a "subhead" class, then target rules with ".subhead:empty".

Thanks, but the example I'm thinking of is more like #bodytext.without-subhead, where the might-be-empty subhead region is outside of and totally separate from the #bodytext div. So, #bodytext.subhead:empty is not equivalent to #bodytext.without-subhead. And in this example I don't think flexbox would be helpful, either.

I think for me the best solution will be to simply avoid designing pages with optional regions in the first place. It was a sort of nifty feature in old versions of Drupal that one dynamic template could be used for several different page layouts, but going forward I can just plan on having several different page templates that exclude certain regions depending on the content type or whatever. (I'm not comfortable delivering client work where the page flickers and jumps around as it loads, so I personally am not interested in JS solutions to this problem.)

Jeff Burnz’s picture

#186 js is not really a viable solution afaict, the flicker is terrible etc, so totally agree there, and the approach you are taking is, unfortunately, pretty much the only real solution. This is a ridiculious situation, if someone changes the block config now you need a themer.

agoradesign’s picture

Imho a CSS-based or even JS-based solution is terrible. I also understand, that a general working solution may be impossible (with regard to BigPipe), but I believe, that we can find a viable solution, that leverages theme developers to be able to decide to exclude certain regions, whether the region is empty or not, for many, but not all, use-cases (for rather static blocks and regions).

We ran into the same problem today. Time pressure led us to a quite specific workaround, as we needed this for a very specific region only. Luckily the only block, that can appear in this region, is a custom one. So we were able to check for emptyness within our template_preprocess_page() implementation und unset the region, if necessary.

However, we are of course thinking for a more general solution, so that we have a generic function for to check any block for emptyness that meet certain requirements.

We were thinking of (ab)using cache tags because this a cheap operation on the one side, and you always find this information in the render arrays:

  1. A block can add this cache tag, informing whether or not it is empty.
  2. Next, the region can check this tag for every single block it contains. If, and only if, every block has set this cache tag, and every block is marked as empty, the region itself can be seen as empty.

It may sound a little bit hackish, but imho it's the best idea so far. Additionally, adding this extra bit of meta information to a block (or any other renderable element), wouldn't hurt.

To further refine this, I'd propose a new Interface (containing an isEmpty() function), that any block can implement. Further, for any block implementing this interface, the new cache tag should be added automatically.

We'll try to implement this or a similar approach for sure in one of our next projects

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

lauriii’s picture

Version: 8.1.x-dev » 8.2.x-dev
Issue tags: +Triaged D8 major

We discussed this issue with @catch, @alexpott, @effulgentsia, @xjm , @Cottser and @joelpittet and we all agreed that this should remain as a major bug. Because of this bug we cannot support adding wrappers for elements only if there is content inside the element. Only known workaround for this is to early render the element, which is discouraged.

We also agreed that this bug should be first fixed against Drupal 8.2.x because we expected the bug fix to be somewhat risky/disruptive. Drupal core maintainers will evaluate the possibility to backport the fix to 8.1.x later when we have come up with a solution.

sic’s picture

this is really annoying :(

A.Eid’s picture

same issue in D8.1.3

joachim’s picture

One workaround is to render the content, and set the result of the render into a twig variable. You can then print that later without re-rendering. Eg:

    {% set facets_rendered %}
      {{ facets }}
    {% endset %}

    {% if facets_rendered|trim is not empty %}
      ... wrapping HTML
      {{ facets_rendered }}
    {% endif %}
sic’s picture

@joachim doesnt work since the {{ facets }} seems to output placeholders at that point!

joachim’s picture

What do you mean by placeholders? That code's been working fine in the theme for a project I've been working on.

Jeff Burnz’s picture

mlncn’s picture

Yeah, this is definitely a problem when templating.

Drupal knows when a region has blocks, right? Would it be possible to calculate block visibility for a region, without trying to render anything, and provide that in a region.has_blocks twig function? Regions with message blocks would always return true, but we can live with that, for now at least.

What we can't live with—seriously, it might kill any one of us in our sleep—is this working-but-horrific approach:

    <div class="{% if page.sidebar|render|striptags('div,p,table,span,a')|trim is not empty %}col-md-8{% else %}col-md-12{% endif %}">
      {{ page.content }}
    </div>

    {% if page.sidebar|render|striptags('div,p,table,span,a')|trim is not empty %}
      <aside class="col-md-4" role="complementary">
        {{ page.sidebar }}
      </aside>
    {% endif %}

It even ignores twig debug: true HTML comments. And it is a monstrosity that i most definitely did not just deploy to production on a client's site after 16 straight hours of dealing with other problems in i hope more elegant ways.

[Edit: Sorry this comment is specific to regions. Maybe worth spinning off into a separate issue? It's not quite the same as #2614954: Empty highlighted region returns whitespace which will always evaluate to TRUE when using twig IF either.]

joachim’s picture

> Drupal knows when a region has blocks, right? Would it be possible to calculate block visibility for a region, without trying to render anything, and provide that in a region.has_blocks twig function?

That would cover some cases, but not all of them.

Some blocks may be set to be visible, but end up rendering to something empty. Eg, with facet blocks, the only way to know if they have content is to run the underlying queries.

joachim’s picture

Maybe we could add a method to the block plugin interface, which allows the theme system to ask the block plugin, 'Do you think you'll have some content to render?'.

Blocks can implement this to give a definite yes or no, and the default in the base class would be 'I can't tell without actually rendering'.

markcarver’s picture

I really think the people commenting recently need to re-read this entire issue.

Maybe we could add a method to the block plugin interface, which allows the theme system to ask the block plugin, 'Do you think you'll have some content to render?'.

Blocks can implement this to give a definite yes or no, and the default in the base class would be 'I can't tell without actually rendering'.

No, this is not possible.

Simply put: this problem cannot be solved in PHP or Twig given the current architecture of render caching and placeholders.

Placeholders were put in as a way to defer rendering until the absolute last possible moment. There are several reasons for this was introduced in core, but it primarily focuses around render caching and BigPipe.

I really haven't focused on this issue in a while, but I do not see this being fixed without some sort of CSS/JS implementation. Like it or not, I suspect it will be a necessary "evil" given how render cache and placeholders work.

Wim Leers’s picture

@markcarver is correct.

And this is a necessary evil that is simply a hard requirement for many advanced use cases and sites, for example Facebook.

In fact, it could be argued that the opposite is true: that the evil is conditional markup.

Mark's suggestion of #150 still stands. And in fact, Firefox has a better (yet custom) version of :empty: they have :-moz-only-whitespace. This is being standardized in CSS4 as :blank.
See https://css-tricks.com/almanac/selectors/b/blank/ + http://stackoverflow.com/questions/31382881/css-selector-for-empty-or-wh....

markcarver’s picture

It also doesn't help that the issue summary is severely out of date and still suggests the "PHP solution(s)" that obviously will not work.

Wim Leers’s picture

Right. It'd be great if the next person who reads this entire issue can resummarize the issue.

markabur’s picture

Seems like the root of the problem is an assumption that every page on the whole site will use the same template with the same regions. Which is strange, because in my experience no Drupal site actually works that way. There are always pages with varied layouts and different regions populated.

Using something like :empty only helps in the case when you're trying to hide an empty region. It doesn't help when some other part of the page needs to dynamically change depending on the emptiness of a region somewhere else on the page. I'd argue that this is an "advanced use case" that used to work fine and now is broken. E.g. if the "slideshow" region is empty, display the main image full width, but if there is a slideshow, float a smaller copy the main image instead -- that sort of thing. What we've (potentially) gained in (perceived) performance, we've lost in design flexibility.

One solution I can think of is to make region visibility into a setting in the UI. E.g. if I have a "Slideshow images" field that populates a slideshow region, I could also have a "Display slideshow?" field that the template could respond to. If the field is unchecked then the slideshow region would be hidden regardless of whether or not the "Slideshow images" field has any content.

markcarver’s picture

Seems like the root of the problem is an assumption that every page on the whole site will use the same template with the same regions.

No. The root of the problem is an antiquated theme system and the use of abstract render arrays. Unfortunately, this will not be changing in 8.x anytime soon.

I'd argue that this is an "advanced use case" that used to work fine and now is broken.

No. This has always been an "issue", even in 7.x. Granted, In 7.x, it was easier to just check for things like #access, #printed, and #children.

Anytime we add new features/requirements to render arrays, we run the risk of fracturing it further.

Placeholders is a prime example of this and they are just exposing an issue that has always been an inherent design flaw with the render array system: how to determine if a render array has any actual content.

One solution I can think of is to make region visibility into a setting in the UI.

This isn't a solution. It's only masking the problem and, in fact, would make this issue even more complicated.

samuel.mortenson’s picture

If our implementation of Twig had something like promises, that would be the only non-JS/CSS solution I can think of. Basically a way to write "After the entire render process is complete and placeholders are replaced, run this condition block". There are some Twig issues about this (see https://github.com/twigjs/twig.js/issues/61), but no one has really approached this problem before.

Fabianx’s picture

#206: Yes, the easiest likely for the moment is to indeed use Javascript for it - as it will always - after some time have the final assembled page.

And yes it would be nice to create that #attached javascript settings needed automatically when evaluating a section in twig.

And in case it is not a streamed, but cacheable response, we could even think of using DOM manipulation instead of the Javascript.

There is several things here that get mixed and I am trying to write up my thoughts on it:

We want things to be fast and easily allow to add technologies like BigPipe (just enable), ESI, AJAX loading of fragments, etc. on top. For all those cases the page layout is indeed considered fixed - per cache key combination.

a) That means at least for blocks - if the conditions of emptyness are known in advance - the condition could be added before those blocks are even rendered (using the block visibility conditions).

That would never render the block and hence not create the problem. Then the region is still a render array, however then at least #193 works fine.

That would not easily solve the advanced use case though - as you create a cross-region dependency without declaring it.

b) Placeholders usually get replaced in the same page request, but using other technologies like BigPipe, ESI, AJAX or even a Middleware it can be as late as directly before sending a response or even after sending a response.

So as soon as you have a placeholder you have a problem - if your page layout - depends on this part being empty or not. As you cannot possibly know during page generation.

Lets take a simple example:

  • - You have a block that varies by user
  • - For user 1 the block is empty, for user 2 it is not

Now you have essentially bound your page-layout to be 'per-user' - if you rendered the placeholder early - and hence would no longer be able to use dynamic page cache efficiently.

So whatever solution we come up with needs to run after dynamic page cache, e.g. in a Middleware and in case of BigPipe-NoJS even while replacing placeholders when a dependency is detected and in case of BigPipe with JS or ajax requests, even check after every fragment is rendered.

Therefore I propose the following plan:

  • 1. Collect _all_ use cases that people need from the most simple (90% cases) to the most complex
  • 2. Create an API like the Form API states API, which can be added to #attached (internal technical detail) on the Response.

    e.g.

    $page['my_section']['#render_states']['visible'] = [
      ['#block-id-2' => ['visible' => TRUE],
      ['#block-id-3' => ['visible' => TRUE],
    ];
    

    or

    $page['my_other_section']['#render_states']['render_states']['empty'] = [
      ['.my-other-section' => ['visible' => FALSE],
    ];
    

    This is just out of the blue, obviously the API would need to be closer defined.

  • 3. Create a Javascript implementation of this API
  • 4. Create a Middleware implementation of this API using DOM parsing, but make that as efficient as possible, e.g. use HTML comments around elements, so only the subtrees need to be parsed.
  • 5. Make it simple to use this API from twig, e.g. introducing a new section block:
    {% section sidebar (hide-if-empty=TRUE) %}
    {% end section %}
    

    or

    {% section my-special with states = {...} %}
    {% end section %}
    

    I am sure we can come up with a nice syntax to make that easy.

I also suggest to make this one a Plan (there is no easy solution) and create child issues for the tasks above.

What do you think?

markcarver’s picture

Category: Bug report » Plan

Agreed with making this a plan as we really don't know what to implement yet.

The above code examples are, IMO, a massive DX regression. The render array "system" is already too complex. This would make it even more of a nightmare.

If we're going to go with DOM manipulation, of any kind, I'd rather we go with a pure JS "solution" opposed to complicating PHP/Twig even further.

Something like the following (bear with me, just thinking out loud here):

  1. Add a class/data attribute to any content that has replaceable content: $element['#attributes']['data-drupal-dynamic-content'] = 'true' (or just use the existing data-drupal-selector attribute and create some drupalSettings for placeholder/dynamic content stuff).
  2. Create some new JS that waits for the page to finish loading and all Drupal.behaviors to finish
  3. Find all empty elements, based on above selectors, using $selector.is(':empty')
  4. Remove those elements

The only "downside" I see to this is that we may have a very brief instance where the page may "look" a little off until these elements are actually removed.

markabur’s picture

5. Create some JS that updates classes and whatnot depending on what got removed (for example to change the main column width depending on if there is a sidebar, as in #197)

The only "downside" I see to this is that we may have a very brief instance where the page may "look" a little off until these elements are actually removed.

For example, an empty sidebar may appear momentarily on each page as you click around the website?

John Pitcairn’s picture

The only "downside" I see to this is that we may have a very brief instance where the page may "look" a little off until these elements are actually removed.

Also add a CSS rule to initially hide those potentially empty elements for js-enabled devices.

Jeff Burnz’s picture

Also add a CSS rule to initially hide those potentially empty elements for js-enabled devices.

Maybe, empty markup is not the major problem IMO, the real nasty issue is not being able to reliably set the layout for collapsible layouts, this is where the loss of design flexibility is most sharply felt.

mdrummond’s picture

I would bet that the vast majority of the reasons that conditional regions are useful in the first place is because Drupal does not have a system that allows for multiple page layouts that can be selected based on certain conditions.

Yes, even with Panels sometimes you have panes that appear only under certain conditions, but entire regions? I would bet that's more rare.

Hiding or showing regions post-render with CSS or JS is probably possible, but also seems quite likely to cause herky-jerky rendering and page reflows.

Conditional regions in Twig is an artifact of a version of our theme system that no longer exists. It's time to let it go.

Yes, there is a need to solve this use case, but I very much believe we'd be better off putting our time and effort into getting core to the point where we can have multiple page layouts, rather than a super-convoluted workaround to restore the days of yore.

markcarver’s picture

Conditional regions in Twig is an artifact of a version of our theme system that no longer exists. It's time to let it go.

Not true.

It very much exists.

Sidebars are a particular prominent example in a project like Drupal Bootstrap (most installed [base] theme for 7.x and 8.x). Sidebar widths, and the content region width, are assigned using classes from the external framework. These classes change based on whether there is content in these regions. Despite what some people like to believe, simply ignoring an issue just because core has little to no issues regarding it doesn't mean that it "doesn't exist".

There are also other regions that come to mind like: header, highlighted, help, possibly even messages in some themes. They should only show up based on whether or not there is content.

This issue is ultimately about any markup (render arrays) that has the potential for placeholders to be stored in them. This, of course, causes the entire element to appear not as "empty". It's a symptom of a wider design flaw of the theme system/render arrays itself. It is not specific to just "regions".

Please, let us not diverge into an entirely separate topic about "layouts/regions" when this particular issue clearly has nothing to do with that.

mdrummond’s picture

The render array system exists (at least in part) as a way to allow for arbitrary items to be placed into a region/variable by site builders, modules or themes. Having only one layout by default means a lot of site builders end up shifting things around on different pages through complex block conditions, and when that is the case, conditional regions comes in really handy. Multiple layouts might not solve every use case, but they certainly would solve a number of them.

If people want to work to add additional complexity to the rendering system in order to support JS deletion of empty nodes and/or changing classes around after the page loads, which may cause a page re-render, that's definitely an option. To me the scope of that work seems bigger than the benefit it would provide. But this is open source, people are free to scratch their own itch, etc., etc.

markabur had mentioned:

Seems like the root of the problem is an assumption that every page on the whole site will use the same template with the same regions. Which is strange, because in my experience no Drupal site actually works that way. There are always pages with varied layouts and different regions populated.

That's what I was getting at. For those who care about this issue because of that, having multiple layout options in core is probably going to be a better option. For those with other concerns, that alone won't do the trick.

markcarver’s picture

This isn't really an issue solely about display logic. That is merely the side-effect IMO. This is more about an issue with how render arrays can now contain even more arbitrary elements, but in a deferred state causing various logic to fail in PHP/Twig.

This issue isn't about layouts. Period. Layouts would not be able to solve this particular issue and would, in fact, still have the exact same issue.

As I said above, this particular issue cannot be solved in PHP/Twig (which is what layouts are) given how the current render array "API", render caching/placeholder is architected.

Given that there was a huge amount of effort put into these systems, I do not see these changing anytime soon. The only recourse I see is some JS/CSS based solution until the entire theme system can be re-architected (9.x).

Fabianx’s picture

#208: I do agree that we can fix the simple use-case with that.

And that would even work for cacheable responses with DOM parsing - without a performance hit.

I like the drupal-dynamic-content attribute

For a high level solution however (the advanced use cases), I think we will need an API similar to states API, then make it easy to use from twig.

--

To all the comments in between #208 and now:

- This is an architectural decision, which we would always have. It is 100% independent of any render arrays or whatever.

I could create a plan PHP script with the same issues or a Symfony one, or ...

And even a layout would only in so far solve it as it would be possible to pre-determine the visibility then. However then you can also use block conditions.

--

So for now we have two use cases - one complex / one simple:

- 1. Simple: When section is empty, remove it. (e.g. empty sidebar)
- 2. Complex: When section A is empty, do something to section B

I know another use case for at least non-flex layouts:

- 3. Medium: When section is empty, add a class to body / parent, etc.

Again 2./3. would be solved by the more encompassing solution using render states.

1. could be an attribute, but it could also be a theme_wrapper.

The advantage of a theme wrapper would be that we likely could avoid the DOM parsing as we could use HTML comments with special markup to wrap the dynamic sections.

Those we can reasonably find via explode() and preg_replace('e.g. strip all tags and whitespace, see if content remains')

And in JS it would just use the added data attribute to find those sections. So as it would need both, we likely would need a helper function or twig filter to add to the section - hmmmm.

If we can avoid the DOM parsing I think I can even see this running on every request. (string operations are pretty fast)

I still think a solution should run in a middleware as well as in Javascript behaviors (as fallback for e.g. BigPipe'd / ESI / AJAX sites).

We could start with some simple Javascript though, but ultimately it would be good to avoid the jumpiness.

Still thinking about the API ...

markcarver’s picture

- This is an architectural decision, which we would always have. It is 100% independent of any render arrays or whatever.

Not really, no.

Render arrays are one of the foundations of the theme system. That is not disputable. It's a fact. That has not changed from 7.x, if anything it was simply enforced and complicated more.

What you're proposing is more band-aids on top of an already gaping wound. I'm tired of band-aids. They make our lives more complicated, not less.

Yes, this is about architecture, however one that is currently limited due to the fact that we still rely on render arrays.

Whatever is implemented here will undoubtedly be incompatible with whatever ends up replacing render arrays in the future.

The fact of the matter is this: 8.x was released. We're already living with markup that is being printed regardless if they're ultimately empty.

Even if there is some slight "jumpiness" (which, btw, no one has actually proven one way or the other... I merely said it was a possibility... it needs proof), that's still better than having elements that are currently causing permanent displacement.

I'm not saying that we shouldn't try and fix the fundamental issue, but I don't think there's much we can do with the given the current architecture (render arrays). Adding another arbitrary "API" to the render arrays is just compounding the problem, not solving it.

I'm under no disillusion that what I'm proposing is probably not any better. However, if given the choice, I would much prefer implementing an automated JS based solution rather than having to remember when said new "API" should be used.

Fabianx’s picture

#217: No, the design principle is placeholders that give much power, but bring this problem.

Whenever you have a page with holes, you will have the same problem.

No designed system can solve that as it is a feature of the architecture - if and only if the is_content_empty decision is not pre-determined.

That is very key to solving this issue.

Render arrays is what we need to work with right now, but the solution in the end needs to work on the HTML markup level + possibly JS settings.

Fortunately we already have HTML subscribers for other things like that already, so there is pre-existing ideas in that space.

I can promise that if we keep placeholders, we will continue to have this problem in D9, D10, ..., as it is a design principle.

--

Oh, I certainly am for automating it as much as possible and making it as easy to use as possible.

--

I am 100% +1 to trying a JS based version first, but ultimately if it can be done with a few CPU cycles in a middleware we should do so - especially also for sites that have JS disabled (graceful degradation).

And core itself needs to support a JS-less version, too.

And to give this some more weight. If we create an API, I am pretty sure we will be able to backport it to D7, too.

markabur’s picture

Even if there is some slight "jumpiness" (which, btw, no one has actually proven one way or the other... I merely said it was a possibility... it needs proof), that's still better than having elements that are currently causing permanent displacement.

As a designer, I disagree. Permanent displacement is fine and it's how most layouts work anyway. If Bartik is a three-column template then it should simply show three columns. No problem. If later we get a way to configure Bartik to show two columns in certain circumstances, that's great. If there is a way to do a one- or two-column Bartik layout using custom theming, or by using something other than blocks and regions, that's fine too. What's not fine is to have content jumping around *at all* after the reader has started reading. That's terribly unprofessional and would not be acceptable to my clientele, at least.

If it's a decision between programmers griping about too much complexity and clients complaining about the readability of their website, I'd choose to satisfy the clients every time. Sorry programmers, it's your job to deal with complexity and to hide implementation details from our clients' customers.

I completely agree with #212, though I do understand that a new layout system can't get into Drupal for a few years yet. If there was a nice way for themes to provide multiple layout options, and a way to specify default layout for each bundle, and a way to override it for individual entities, then (a) dynamic region hiding would be unnecessary, and (b) the layout system would finally be comprehensible to writers/editors.

Although it's easy to think of examples where conditional regions are helpful, it's also easy to think of ways to avoid depending on them in the first place. The placeholder system was apparently designed with the assumption that regions would always be present:

So… AFAICT the only solution to this problem is to never have conditionally appearing regions; regions must be assumed to be always present, they just may end up containing nothing.

... which to me is ok as long as I can design around it. The last D7 theme I created didn't really use regions at all, and to be honest it was easier that way.

#2542050: Toolbar implementation creates super annoying re-rendering. has some tricky stuff that might be helpful in a JS workaround for this issue.

Fabianx’s picture

Note:

- There is several things that are mixed up here again:

a) "Layout" determines empty-ness.

An empty region can still contain, data so it must be rendered to determine its empty-ness.

Example: A block only appears for users with the uid=2,4,6,8,10,... (even ones) using custom block visibility conditions

That means it is effectively cacheable only by user ID. Even if the region containing that block is empty, it will still need to be rendered, because the page itself needs to be varied per user as well.

That is a mere syntactical thing and work arounds to early render in twig will work fine.

Note: It might be a good idea to just put the data into the render "context" instead of into the region itself, which would make

{% if page.sidebar_left %}

working again as well as avoid loosing metadata that determines information on the page.

However the problem here is that custom code might have the condition that the sidebar is only shown when the moon is full, so it would never be rendered, hence the page would never be needed to be varied per user, so rendering to determine empty-ness is still needed.

Syntactical sugar is possible:

e.g.

{% section page.sidebar_left %}
  {{ content }}
{% endsection %}

could only show the section if sidebar_left is not empty and the rendered result would be stored in e.g. {{ content }}.

However that is little more effective than:

{% set sidebar_left %}
  {{ page.sidebar_left }}
{% endset %}

{% if sidebar_left %}
// ...
{% endif %}

b) Inner content determines empty-ness.

samuel.mortenson’s picture

From what I understand, there is no Twig work around for checking if placeholders will eventually be replaced with HTML. Conditional blocks are only one part of the problem.

DamienMcKenna’s picture

Regarding use cases, I help maintain a site that has sections which list different groups of related content in the sidebar; the Panels page's logic has visibility rules set up to check the previous panes (which were all views) to see if they had any content before determining if the current one should be rendered. The logic written by the site's original developers resulted in each pane being rendered over and over and over again as each subsequent pane had to check if each previous pane contained any results. I was working on a custom Panels cache / visibility rule combination that would run generate each pane one and then determine if it should be rendered, but it gets rather complicated rather quickly.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xeM8VfDh’s picture

So, is there at technical reason why #151 can't be implemented, so #149 can be used as a simple css fix? A non-JS fix would be nice. Or was there some other comment explaining how that isn't a valid solution (sorry if I overlooked it).

Jeff Burnz’s picture

I made a jsbin to show how :empty can work with flex and flex-grow in case anyone is interested in this approach.

http://jsbin.com/hofiwijoma/edit?html,css,output

The problem is of course, as markcarver points out, ensuring emptiness. As it stands this is quite difficult and would, most likely, have to be done with Javascript, for example there is currently an issue to add a data-attribute wrapper to messages, if that goes through it would always be there in the output: #77245: Provide a common API for displaying JavaScript messages

And this is only trying to solve a small issue of a column layout, e.g. Bartik in D8.

xeM8VfDh’s picture

Thanks @Jeff Burnz. Since :empty is only helpful when the tags have literally no characters between them, we'd have to get rid of the blank space you demonstrate. I've done this by updating my bootstrap sub-theme's page.html.twig from:

      {# Sidebar First #}
      {% if page.sidebar_first %}
        {% block sidebar_first %}
          <aside class="col-sm-3" role="complementary">
            {{ page.sidebar_first }}
          </aside>
        {% endblock %}
      {% endif %}

to

      {# Sidebar First #}
      {% if page.sidebar_first %}
        {% block sidebar_first %}
          <aside class="col-sm-3" role="complementary">{{ page.sidebar_first }}</aside>
        {% endblock %}
      {% endif %}

That seems to have done the trick. This adjustment could be made for all blocks/regions, using a unique class, then hiding empty block/regions with css becomes a 1 liner.

EDIT: a little side affect of this not-so-great-but-its-working-for-my-situation workaround is as @audriusb mentions in the comment immediately below... if you enable twig debugging with debug: true in your Dev environment's *services*.yml file (lets hope its not enable in your global/live/production yaml file), this workaround will break because twig will add comments between , and the css rule won't be triggered. You can either accept the weird spacing with the blank blocks in dev, or disable twig debugging in dev... or write some JS to strip comments in specific blocks (probably more headache than its worth). Can't wait until someone smarter than I comes up with a robust solution to this very tricky problem.

audriusb’s picture

css :empty stops working when twig debug is enabled because of template suggestion comments. Not the end of the world but quite annoying. Also it is necessary to have javascript fallback for older browsers.

xeM8VfDh’s picture

Thanks @audriusb for the heads up. And I agree with the JS backup for old browsers, for those that want to support them. I do think a css solution with a JS backup is better than a js solution, but thats just my opinion.

MustangGB’s picture

CSS is a bad solution because it's not possible to perform any of the use-cases.

1) If region A is empty show region B - can't be done
2) If region A is empty add class "region-a-empty" to body - can't be done
3) If region A is empty remove region A - impossible to remove, only option is to hide with :empty which is pernickety at best and breaks if you sneeze on it

The sticking point seems to reside around placeholders which can either be fulfilled:
A) Client-side
B) Server-side

For (A) you'd be reliant on JavaScript anyway so it's your only option. Also you shouldn't have to worry about jerkiness as anything with placeholders should be hidden initially then only shown (or removed) once the placeholders have been populated, if you want to be really fancy you could animate the show.

However for (B) I really think this should not rely on JavaScript, this should be done after placeholders have been populated and in PHP.

xeM8VfDh’s picture

Sorry @MustangGB, you are correct. I should have pointed out more clearly... the solution I outline is a work-around for the 90%. It cannot handle the complex use cases you've outlined, or anything other than hiding an empty block/region. Sorry if I mislead anyone or got anyone's hopes up. The larger issue, including the complex use cases, appears to be a very tricky/complex situation. I for one would love the "if block A is empty show block B" type of functionality.

Fabianx’s picture

#229: I do agree with you.

What do you think of the plan in #207?

Jeff Burnz’s picture

#229 #231 of course CSS is not a robust solution, it's fragile, requires JS with associated reflows etc, please, tell me something I don't know, but it's better than a borked website, and can most certainly work in specific situations - don't forget we need work-arounds that work now, not in (another) 6 years time.

agoradesign’s picture

A CSS/JS based solution is never more than a workaround, but better than nothing.

regarding a server-side plan like #207: +1 for that! I know, that not every block knows about its content in advance, but many blocks do. So it's quite common to have regions full of blocks knowing if they're empty or not. And these cases could be easily handled server-side as well. I'd never be satisfied to hide such regions with CSS/JS only. Like mentioned in #188, we've already implemented a server-side workaround in a client project some months ago

Jeff Burnz’s picture

Regarding #207 I'd like to comment in it but I'm not sure I understand the proposal.

Thinking it would work like states, is the idea that a data attribute is set on elements, then use JS?

E.g.

$page['my_section']['#render_states']['visible'] = [
  ['#block-id-2' => ['visible' => TRUE],
];

or

{% section sidebar (hide-if-empty=TRUE) %}

etc, might result in something like:

<div id="block-id-2" data-drupal-render-states="{visible:{has-content:true}}"></div>

OK, but from there it's a little opaque, not really following the idea or how DOM parsing solves some part of the issue (I don't really understand what you mean), and with the JS, assume if has-content==false, set display:none; or something like that, e.g. [hidden]. I'm assumign the DOM parsing bit is to answer the question - does this have some content?

With regards to syntax/DX, well hey, I'm on the fence, what I see in D8 with themers is they are not getting it anyway (especially those new to Drupal), we're so far past where average themers can grok the theme/render systems it's a lost cause. Whats more complexity piled on more complexity - honestly, not much.

jorgediazhav’s picture

Well, this may not be the solution, but it is a workaround that works good for me (render_var()):

{% set rendered_sidebar_second = render_var(page.sidebar_second) %}
{% if rendered_sidebar_second %}
   {{ page.sidebar_second }}
{% endif %}

I agree it is not optimal, but on a specific project case it is a performance perk that I'm willing to have over uncontrolled sidebars showing/hiding :(

samuel.mortenson’s picture

@jorgediazhav Will that replace placeholders? I think that's the main problem with pre-rendering in Twig/PHP - placeholder replacement happens further down the pipeline.

Jeff Burnz’s picture

@ #236 short answer - no.

alexej_d’s picture

Stupid question (please don't hit me :): Can't we introduce a placeholdered conditional logic? Like special if statements which generate placeholders. Sections which are inside those placeholdered conditionals are not rendered until the placeholder is evaluated

Fabianx’s picture

#238 Interesting idea, so a section that if it contains placeholders, it would actually be placeholdered itself and the rendering be pushed to later.

It would be possible, but probably would just be an internal change from the API proposal above - but just a different implementation.

--

To bring this forward we need to agree on two things:

- How to define sections that should be rendered conditionally in the most simple and TX friendly way
- The actual technical implementation

I still owe an answer to a concrete implementation of that and how this would work in practice.

ressa’s picture

I am not sure if there is a solution in this post, but I got here searching for a way to hide empty blocks. I have a View where I have checked off "Hide block if the view output is empty" in my view, yet the block is still displayed... I am using the Bootstrap theme, if that makes any difference. Can I add some code to my .theme file to hide it, or is there another way of hiding empty Views blocks?

audriusb’s picture

check if there are no html comments there. i.e. theme debug.

ressa’s picture

Here is an interesting observation: If I uncheck "Display title" under block configuration, it will respect the "Hide block if the view output is empty", and not show the block if there are no fields in the View.

But it also will not then show the title of the block, which is kinda important :-)

dkre’s picture

Here's a hack/workaround (of sorts). In my case I'm running into this issue with a views generated block with debugging on.

I would strongly recommend this patch which is required for this workaround. There are instances where Drupal will render debugging comments as string values on more complex field templates (such as date time). It's also vital for instances like this where the comments must be removed during development.

{% if page.jumbotron|render|remove_html_comments|trim is empty %}
  <div class="page-banner">
{% else %}
  <div class="page-banner jumbo">
{% endif %}

page.jumbotron|render Render is required to turn the array as a string, then remove_html_comments removes debugging followed by twig's trim to remove the masses of whitespace to return true on empty as expected.

I would recommend pushing this up the cue a little and updating the description to reflect D8/twig theming now that this is tagged as a 8.3.x issue. I know this is just a theming issue but this is the sort of issue that prevents frontend developers adopting Drupal.

amklose’s picture

In one of our themes, we're using hook_preprocess_html() to set some classes on the body depending on which sidebars are showing. The code was checking !empty($variables['page']['sidebar_first']) but even if no blocks were set to display in a particular sidebar, the $variables['page']['sidebar_first'] array would not be empty. To get around this, I am doing this instead: !empty(trim(render($variables['page']['sidebar_first'])))

This seems to be working so far, but I don't know how much overhead it adds to render both sidebars on every page load just to determine if they're empty before doing anything else.

It seems like there should be some kind of function that accepts a region array and it would recursively check all of the contents to see if anything will be rendered without actually rendering them.

Jeff Burnz’s picture

I am wondering if we need to start a new thread to specifically discuss issues around placeholders, it seems pretty clear to me that checking renderable arrays is not really ever going to work.

Perhaps we can archive this as historical discussion and move on?

Wim Leers’s picture

+1

Although I fear it won't really help, because clearly people continue to want to be able to do this, given that new comments keep being posted…

joachim’s picture

It seems to me that we have two incompatible goals here:

On the one hand, Big Pipe works with fixed, predictable layouts. Facebook has the same grid layout and every element of the grid gets something put into it.

On the other hand, people making sites with Drupal often have designs that are flexible, where parts of the page are not always populated. Drupal's own design of regions and blocks encourages the design of a single overall layout where some parts may be absent.

The problem is we're trying to serve both approaches with one system.

cwells’s picture

joachim really hits the nail on the head here. One thing I think would be useful is that if we did close this issue, there was a nice summary at the very end, so that people can scroll down and see it. Here's my attempt:

The competing interests are (1) BigPipe (really, placeholders - which have utility beyond BigPipe) and (2) "dynamic" regions -- that is, regions that are completely removed from markup if they have no content in them.

What I'm calling "dynamic regions" has been in Drupal core since I've been working with it (which is 4.7), so it was a very well established pattern of how to build Drupal sites. In Drupal 8, placeholders kind of does away with this ability because we're saving the rendering of those regions, which have a placeholder in them, until the very last second, which doesn't allow us to determine if something is truly "empty" because it has a placeholder in it (which ultimately COULD be replaced with something, or with nothing), so traditional "if ($left)" doesn't work.

It seems to me that if you turn off anything that's using placeholders (for me I had to disable BigPipe), then you can use some kind of workaround in the Twig that works like the old thing did. Essentially you render the region into a variable, then you can test against this variable to determine if you should print it. I think core Bartik does this to make body classes. Here's just one example (comment 243, above):

{% page.sidebar_first|render|remove_html_comments|trim is empty %}

Another thing that can be done, that works if BigPipe is enabled, is to solve it after the fact, client-side, with some JavaScript. Look inside each of your regions, determine if they're "empty" by your standards, and then set body classes appropriately. This is useful if trying to replicate selectors like:

body.one-sidebar #main {
  width: 66%;
}
body.no-sidebars #main {
  width: 100%;
}

You can also use JavaScript in a different way - which is to strip out all the whitespace characters from a region, and couple that with the CSS pseudo ":empty." Note, :empty only works if ALL the whitespace is trimmed out. Hence, why you need the JS. (comment 176, also see 180 for a solution enhanced with flexbox).

The other pseudo that's available is :blank, but that's not really available yet (comment 173).

There's also a backend-code based solution (comment 171): "You could implement your own late EventSubscriber as a Response Subscriber and re-write the response using either a DOM manipulation library or regexp." That is, you can substitute the placeholders that are NOT ultimately replaced LATER in the game so that they truly come out empty. I *think* this would provide the most "Drupal-7 like" ability. (Ed. note - could this be a contrib module? Does this interfere with BigPipe? Should this just be in core?)

I hope this is a decent summary of the possible approaches for folks who are struggling with the lack of old body classes. I'm sure that there's a big gap in my knowledge here, but I think most people that are ending up at this issue are just wanting to know "how do I make it work like it used to?" If we can answer that at all, we're in good shape.

Jeff Burnz’s picture

The problem is we're trying to serve both approaches with one system.

Indeed, so really we should not be discussing placeholders as I mentioned in #245, what we need to be advancing on is layouts in core.

For those finding this issue please see comments: #200 and #247, and if you have time at least the comments since around 150 or so.

We need to accept that for now we live with compromises and move forward on layouts in core as the real solution. Personally I think we're done here. I maybe wrong, but it just feels like a merry go round :)

Fabianx, it's your call I believe.

tim.plunkett’s picture

Honestly, I'm already working on Layouts in core, and I'm hitting this exact bug :)

EclipseGc’s picture

So... given this seems to be getting a lot of attention lately, I'm just going to revisit it and realign with some of the original testing I did that lead to the filling of this issue.

This is as much about #access as it is anything else. You have a region, it has a block, but that block's access is false for this user, the block won't display, but the region will because the region is not empty, its contents are just inaccessible. Because of this, "if" checks on things like !empty() or whatever for the contents will always return true because there is in fact data in that variable... it's just data that won't result in anything but an empty render string.

To a certain degree, it feels like D8's render caching could be a critical component of solving this. In D7 we wanted delaying rendering as late as possible. This was for a number of reasons, but hook_page_alter() is the most obvious one developmentally. This was especially dangerous because it meant that any module/theme implementing this hook could actually alter the access of the blocks in our above scenario. Given now that block access actually runs through the AccessResults system and that we have a pretty fantastic caching mechanism in place both for the access results and the rendered output, it seems like we could opt to render sooner rather than later and bubble up the results so that they arrive to the layout system(s) as strings in a render array via #markup and accompanying cache contexts/tags/max-age metadata. Obviously, this depends on us nailing render caching for authenticated users too, but I wanted to at least put this out there and see if others agree, or if this is just totally wrong.

Eclipse

Jeff Burnz’s picture

@tim.plunkett how so tim, if the layout is essentially static (no conditional regions) what are the problems, are there #issues for this stuff, love to hear more about it.

dkre’s picture

Coming back to this now that I've had some time to read through the thread, it does seem like a great idea to close this issue thread and restart the discussion with a fresh summary. Because this is likely to be an ongoing issue it would be great to be able to summarise workarounds.

The real pickle I found with this issue is that it breaks the documented/demonstrated logic of how the theming system should work, almost every twig template with more than a couple of lines has a if var

joachim’s picture

> Indeed, so really we should not be discussing placeholders as I mentioned in #245, what we need to be advancing on is layouts in core.

Could you expand a bit on how layouts in core is going to fix this?

I've not been following that initiative, so what I am imagining might be completely wrong. I'm imagining that if I have a theme where the LH sidebar shows a block if a node has field foo, and the RH sidebar shows a block if the node has field bar, then that is already 2x2 = 4 different layouts. If other regions are collapsible, that gets worse very fast! It seems to me that this is as bad as having multiple page.tpl files in the old days, which was always something to avoid as much as possible for maintainability.

markabur’s picture

It really just needs to be some sort of UI to manually show/hide regions, so that the theme can check a setting rather than checking for emptiness.

Layouts in core would be a way to package up that UI with a template and some css (like Panels layout plugins, but for blocks/regions).

joachim’s picture

> so that the theme can check a setting rather than checking for emptiness.

But emptiness is often more complicated than a setting. Regions can be empty because a node has or doesn't have a value in a field, or if a view returns an empty result, or if a search produces a result that has no facets.

tim.plunkett’s picture

This bug affects layouts the same as it affects regions. Please stop discussing layouts here.

Jeff Burnz’s picture

This bug affects layouts the same as it affects regions. Please stop discussing layouts here.

Some explanation of that might be interesting. I assume you mean the general problem of emptiness, i.e. you can't conditionally set alternative layout based on a condition of emptiness?

Could you expand a bit on how layouts in core is going to fix this?

By not using collapsible regions. It may well mean more layout management. I'm not entirely sure how this will work. Why layouts in core is to put layout into hands of site builders and not themers, and nothing more, i.e. at least move beyond single page layout paradigm.

All I can see right now is 1) either an abandonment of the idea of emptiness as a condition, or 2) make emptiness work as a condition (this looks very difficult, maybe even impossible to achieve in a general sense).

Are there other alternatives?

I'm all for us making it work with collapsible regions, OK, but its been 6+ years and the problem now appears unsolvable due to placeholders (which I would not trade for collapsible regions ever, because they rock). I'm just trying to think of how we might solve this, think right outside our current paradigms.

Fabianx’s picture

Summarize again:

0. Placeholders only have partially to do with BigPipe - They allow our dynamic page cache to work at all for complex scenarios. It is a pre-requisite for authenticated user caching.

1. Access is not affected in core right now. If you use block visibility access conditions you are fine => with or without placeholders. (because it happens _before_ any rendering takes place). The only thing that there is, is that there is cache data in the region, so !empty() won't work. A helper function by Wim will filter that there are no renderable children. This needs a helper function, however at least the cacheable meta data needs to be put to the upper level.

We could also for empty regions just remove it and render it on the render stack instead. I think that would be safer, too and is exactly what happens when people using work-arounds to pre-render things.

This would make block visibility condition blocks work nicely - e.g. the user login block and is a really simple fix - just render an empty region before returning.

2. Empty blocks

Empty blocks happen just after rendering, so we cannot know if the region is empty before rendering it actually. Pre-rendering work-around works, but is ugly especially because it doubles the time to render the region.

3. Placeholdered blocks => 99% of you are by default not affected by this

Empty blocks happen just after placeholder replacement, with BigPipe even just on the client. => Pre-rendering work-around will not work. A late event subscriber or for BigPipe a JS solution will work.

2 and 3 could be solved with the same API or with different approaches.


I still think a section tag would be nice, which automatically pre-renders the content of the variable in question:

{% section sidebar_left %}{# This is empty if {{ content }} is empty #}
<div class="wrapper-for-section" {{- content_attributes }}>
{{ content }}
</div>
{% endsection %}

That won't solve the advanced use cases of adding classes, etc. obviously though.

Also the code could check for placeholdered items in the rendered render array as they will be in #attached and then fallback to the 3) API.

Would that help at all as a starting point to resolve this?

It is pretty simple to implement - now that I understood how Twig internally uses class functions to implement blocks.

markabur’s picture

But emptiness is often more complicated than a setting. Regions can be empty because a node has or doesn't have a value in a field, or if a view returns an empty result, or if a search produces a result that has no facets.

The idea is that a setting would be used instead of worrying about emptiness in the first place. There are lots of reasons someone would want to hide the left sidebar in Bartik, and "because it is empty" is only one of them.

If there is a UI to control the region visibility, then the writer/editor/builder gets explicit control over it, and the theme doesn't have to determine context at all when it is rendering regions, adding layout-related classes, etc.. Emptiness becomes irrelevant as far as the theme developer is concerned.

This bug affects layouts the same as it affects regions.

In what way? If I have a setting called "single sidebar left" then Bartik would be able to look at that and hide the other sidebar without any trouble at all.

markcarver’s picture

If there is a UI to control the region visibility...

@markabur (re: #260), that's layouts, which is an entirely separate issue/concept. Please stop scope creeping this issue (as @tim.plunkett asked in #257). This issue is specifically about determining if a render array has visibility and should be rendered in the first place.

---

How does this issue [indirectly] affect layouts?

Because layouts would use the same conditionals in Twig like regions currently do to determine when a section (region) in the layout (page) should be visible or not.

Regardless, layouts isn't the issue here; it's just suffering the same problem like everything else.

Fabianx’s picture

Title: Themes improperly check renderable arrays when determining visibility » [meta] Themes improperly check renderable arrays when determining visibility

For those interested in technical approaches, I have split off two child issues with actionable items and/or technical discussions on how to solve this:

One question that I asked that was never answered however is:

What do we consider as _empty_?

  • - a) Empty means there must be text other than normal whitespace or tags or HTML comments (e.g. render|strip_tags|strip_comments|trim)
  • - b) Empty means that there is no whitespace, but HTML tags or HTML comments are considered non-empty (render|trim)
  • - c) Empty means there is absolutely no content in there not even whitespace (render)

Which of those do we want? Even in the work-arounds there is several competing solutions for this ...

Of course we could make it configurable, but is that really what we want and then the question stands on what we want as default.

So let the discussion begin.

MustangGB’s picture

Seems like you've already ordered them from best to worst, and from hardest to easiest.

Unless of course they all have the same level of difficulty, in which case A is probably the one to go with by default.

Or is it less a question of difficulty and more one of performance?

danbohea’s picture

If it is a question of performance then I think the suggestion to make it configurable is a good one. Different use cases will warrant different degrees of compromise in terms of performance. Even then though, as @Fabianx points out, we'd still need to settle on a default.

Can we confirm whether it is a question of performance first?

Fabianx’s picture

It is not a problem of performance for me, all filters should be pretty fast.

cwells’s picture

I prefer (a), but obviously if that were somehow configurable for the theme developer that would I suppose be ideal. But if I had to choose one, I like the looser definition of empty being "nothing of any real markup substance."

danbohea’s picture

In the case of option A, what would happen if an <img> element was the only content? That would constitute no text "other than normal whitespace or tags or HTML comments". If that was considered "empty" and therefore hidden then I can imagine it being a pretty confusing default for some fairly common use cases.

If I've got that right then I'd prefer to see option B as the default:

- b) Empty means that there is no whitespace, but HTML tags or HTML comments are considered non-empty (render|trim)

danbohea’s picture

That said, with Twig debugging enabled that could get very annoying, given all the HTML comments. Is it possible to consider HTML tags non-empty and consider comments empty?

Fabianx’s picture

#268 Sure, I did not think of twig debugging here, so yes.

danbohea’s picture

OK, so there's a fourth option for people to consider for a default emptiness definition:

  • A: Empty means there must be text other than normal whitespace or tags or HTML comments (e.g. render|strip_tags|strip_comments|trim)
  • B: Empty means that there is no whitespace, but HTML tags or HTML comments are considered non-empty (render|trim)
  • C: Empty means there is absolutely no content in there not even whitespace (render)
  • D: (**NEW**) Empty means that there is no whitespace, HTML tags are considered non-empty, HTML comments are ignored and considered empty (I guess that's render|strip_comments|trim ?)

Option D is my preference: it's compatible with Twig debugging and shouldn't accidentally hide non-text HTML (e.g. <img> tags).

jwilson3’s picture

Does the strip_comments Twig filter exist today or is this just hypothetical? I couldn't find it documented anywhere online -- do you have a link?

dkre’s picture

Yeah theres a WIP patch here: Remove HTML Debug Comments from a particular field in debug mode

However after just completing my first D8 site I would now recommend doing this process (checking for empty regions) in preprocess.
|render|strip_comments basically renders the region twice if it has content (someone correct me?), the performance hit isn't ideal.

The above patch is extremely handly though for certain nested templates which end up displayed comments and rendered text in Views etc.

jphelan’s picture

I agree with danbohea, option D in #270 makes the most sense to me as the default. I keep running into this issue.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

realityloop’s picture

Anopther vote for option D in #270

klasseng’s picture

I too am having trouble with this behavior.
Running Drupal 8.2.7 | Theme is Bartik

Case 1

No block placed in the right sidebar, site looks as it should

Case 2 - where the bad behavior is evident

A view generated block placed in the right sidebar, the view has no content on this page, view is set to Hide block if the view output is empty. But the site renders with an empty right sidebar. Sample page: http://webv3a.mennonitechurch.ca/

Two things relevant things show up in the page source:
1. The body tag includes the code:
<body class="layout-one-sidebar layout-sidebar-second ... etc etc
2. There is code for the right sidebar:

<div id="sidebar-second" class="column sidebar">
            <aside class="section" role="complementary">
              
            </aside>
          </div>

Case 3

as in Case 2, but on this page the view has content and renders properly. Sample page: http://webv3a.mennonitechurch.ca/sacrifice/arson

hkirsman’s picture

Until some genius fixes this, tx for the tip on :empty: https://www.drupal.org/node/953034#comment-9794141 :)
I changed my region.html.twig template so it wouldn't have any white-space and applied .region-foobar:empty {display: none}

susannecoates’s picture

If the access property is callable and could be used to abort rendering... you would be able to abort the render of a region if it's empty. i.e. Allow renderable arrays to abort their own render using callables.

lukasss’s picture

When will be fixed this error?