Problem/Motivation

There is a huge Drupal core issue about evaluating a render array as true or false from a Twig template: #953034: [meta] Themes improperly check renderable arrays when determining visibility

In certain situations regions that contain empty blocks still render.
The core issue is that there is no possible way to definitely identify an 'empty' block without first
rendering it.
The ideal result is the ability to add this to a template:

{% if region %}
  <div>{{ region }}</div>
{% endif %}

Very annoying but, after 13 years, still no fix. But I hope it may be fixable at SDC level.

Steps to reproduce

Let's do some tests on Drupal 10.1.1 with this variable:

{% if description %}<p class="fr-tile__desc">{{ description }}</p>{% endif %}

We expect description to be resolved at false.

Test 1: empty array

$variables['description'] = [];

OK: description is false, because in PHP an empty array is false

Test 2: non renderable properties

$variables['description'] = [
  "#cache" => [
    "contexts" => [
      "user.permissions"
    ],
    "tags" => [],
    "max-age" => -1
  ]
]

KO: description is true

Test 3: non renderable properties wrapped in a sequence

$variables['description'] = [
 [
  "#cache" => [
    "contexts" => [
      "user.permissions"
    ],
    "tags" => [],
    "max-age" => -1
  ]
]
]

KO: description is true

Test 4: non renderable properties wrapped in a mapping

Layout Builder is sometimes wrapping the render array in a mapping where the key is the block UUID:

$variables['description'] = [
"cf124739-3274-41cc-829d-c3465ee4eaa5" => [
  "#cache" => [
    "contexts" => [
      "user.permissions"
    ],
    "tags" => [],
    "max-age" => -1
  ]
]
]

KO: description is true.

Test 5: empty mapping

$variables['description'] = [
    "whatever" => []
];

KO: description is true.

Test 6: empty #markup

 $variables['description'] =  [
    "#markup" => "",
  ];

KO: description is true.

Same with #plain_text I guess...

Proposed resolution

So, it may be fixable at SDC level because we are lucky to know:

  • which Twig variables are expecting a render array : the slots as defined in the component definition
  • where to put the bubbled properties: the component render array ([#type => 'component'])

2 informations other render elements are not able to know easily.

So, in ComponentElement::preRenderComponent() for each component slot:

  1. if the array is not a render array, do a recursive loop on items
  2. if the array is a render array where all the children are empty AND where all properties are non-printable or empty:
    • move the non-printable properties (#cache, #attached... what else?) to the component render element if they are bubbable
    • remove the non-printable properties (#weight... what else?) if they are not bubbable
  3. unset the keys of empty children & empty properties

So, the renderable will be evaluated to false by Twig if empty 👍

Comments

pdureau created an issue. See original summary.

pdureau’s picture

pdureau commented
Issue summary: View changes
pdureau’s picture

pdureau commented
Title: SDC: Check renderable arrays when determining visibility » SDC: Make empty render arrays evaluate to false in component templates
Issue summary: View changes
e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

One of the challenges I see with this is that we will have to identify and manually handle all these non-printable keys. This includes bubbling, and or erasing some keys.

I would love to have this feature, but I am not sure a solid / maintainable implementation is feasible. Let's see what an initial patch looks like.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Postponed (maintainer needs more info)

I hope it may be fixable at SDC level.

I don't see how, because SDCs' slots may contain arbitrary contents, including arbitrary render arrays, so all the problems outlined in #953034: [meta] Themes improperly check renderable arrays when determining visibility also apply here? 🤔

The 6 tests in the issue summary are all simple cases. Introduce more complex render arrays using #lazy_builder anywhere (top level or deeply nested) in a render array for a slot, and it won't work anymore.

pdureau’s picture

pdureau commented

One of the challenges I see with this is that we will have to identify and manually handle all these non-printable keys. This includes bubbling, and or erasing some keys.

We need to try :) Maybe a predefined list of properties to bubble (#cache, #attached...) would be enough, maybe its is too naive.

I don't see how, because SDCs' slots may contain arbitrary contents, including arbitrary render arrays

Slots may contain:

  • scalars, which are transformed to [ "#plain_text" => (string) $slot_value ] before landing in the template
  • in a template, slot values are not expected to be "browsed" or altered, but processed by a Twig print node {{ ... }}:
    • PHP object implementing RenderableInterface are transformed to render arrays
    • render arrays are sent to the renderer service
    • Stringable PHP objects are cast as strings
    • strings are simply printed

So, the only blind spots, the only things not expected in a slot, are:

  • objects not stringable or renderable, because they will raise 'Object of type ' . get_class($arg) . ' cannot be printed.'
  • arrays which are not render arrays, because they will raise InvalidArgumentException: "pop" is an invalid render array key.

So, we know that all PHP arrays which are expected, which are legitimate, to be sent in a slot are Drupal render arrays. It is not our own decision, it is the current state of Drupal rendering. If we decide one day to remove this restriction, we will meet an other one at the Twig level anyway: Warning: Array to string conversion.

Knowing this, we can consider all PHP arrays in slots as Drupal render arrays, and apply to them our logic of bubbling some render properties and removing others.

So all the problems outlined in #953034: [meta] Themes improperly check renderable arrays when determining visibility also apply here? 🤔

This issue is not pretending to fix #953034: [meta] Themes improperly check renderable arrays when determining visibility everywhere, but only for SDC. As SDC will become more and more successful, this issue will be more and more helpful :)

The 6 tests in the issue summary are all simple cases. Introduce more complex render arrays using #lazy_builder anywhere (top level or deeply nested) in a render array for a slot, and it won't work anymore.

Indeed. Thanks you for sharing that. We will be careful.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

I think it's worth starting with a single (simple) #lazy_builder example. I'm pretty confident you'll find that is immediately a hard blocker.

Something like:

$slot_content = [
  '#lazy_builder' => ['_what_time_is_it', []],
  '#create_placeholder' => TRUE,
];

Assign 👆 to a slot, and define:

function _what_time_is_it() {
  $time = time();
  $reply = $time % 3600 === 0
    // Rudely no reply if it is exactly the top of the hour!
    ? ''
    // A polite reply otherwise.
    : '<marquee>It is ' . time() . '!</marquee>';
  return [
    '#markup' => $reply,
    '#cache' => [
      'max-age' => 0,
    ],
  ];
}

I don't see how you can make that work reliably. It's the exact same problem as #953034: [meta] Themes improperly check renderable arrays when determining visibility has. And yes, it's a silly example, but replace that silliness with access checking. That is a very real use case that must also work.

pdureau’s picture

pdureau commented

Thanks Wim.

Indeed, a #lazy_builder renderable can be found in a slot (and only in a slot) and we don't want to execute the callback in order to get and evaluate the "real" renderable.

The #lazy_builder renderable contains keys which don't belong to the non-printable keys we can remove or move to the #type => component level, so it will not be altered.

So, we may have some false negative and miss some empty render arrays here. But I guess it is OK. If we catch the other ones, it is already a win.

Do you agree?

pdureau’s picture

pdureau commented
Assigned: pdureau » wim leers
pdureau’s picture

pdureau commented
Status: Postponed (maintainer needs more info) » Active
catch’s picture

catch
he/him
Primary language English
commented

If we catch the other ones, it is already a win.

Can you explain what the win is here?

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
Related issues: +#3526176: Allow empty array in SDC slots

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

pdureau’s picture

pdureau commented

Can you explain what the win is here?

The win here is to send renderables which are evaluated to FALSE when empty, so Twig can tests them right: {% if my_renderable %}

As Wim told us in comment #7, the proposed change will not work for the #lazy_builder renderables because this array will never be evaluated by false even if the resolved renderables is empty.

In my opinion, this is not cancelling the proposed change because it will already be a "win" to be able to properly evaluate a lot of renderables in Twig.