SDC: Check renderable arrays when determining visibility

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.

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 🌱 [meta] Themes improperly check renderable arrays when determining visibility Needs work 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.

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 contains:

• 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, butprinted (in a Twig print node, with {{ ... }}:
  • 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:

• object not Stringable & not RenderableInterface, 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 at the Twig level anyway: Warning: Array to string conversion.

Knowing this, we can inspect and alter all slots PHP arrays as if they are 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 🌱 [meta] Themes improperly check renderable arrays when determining visibility Needs work 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.

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 🌱 [meta] Themes improperly check renderable arrays when determining visibility Needs work 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.

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?

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

Can you explain what the win is here?