Make the Twig loops safer

Proposal, we use a filter.

I know we are considering filters on slots a bad practice, because most or all of the Twig filters are too typed and don't fit with slot's free types.
However, we can introduce a list of "slots filters", filters compatible with slots.

2 ways:

Proposal 1. Introduce a new one “seq_wrap” (lists are called sequences in Twig):

<div class="carousel-inner" role="listbox">
  {% for slide in slides|seq_wrap %}
    <div class="carousel-item">
      {{ slide }}
    </div>
  {% endfor %}
</div>

If the value is a sequence, do nothing. If not, wrap into a single item sequence.

A level parameter: |seq_wrap(2) (default value: 1) can be useful when slots are wrapped in 2 levels of loops, for example the cells of a table.

Proposal 2. Use a combination of existing filters. Which ones?

Proposal with a "sequence" test: https://git.drupalcode.org/project/ui_patterns/-/merge_requests/26

That doesn't mean we will pick this proposal instead of the other. Let's discuss.

Let's try to implement this in upstream : https://github.com/twigphp/Twig/issues/3828

As you know, I don't like having to add things in templates to handle Drupal problems. IMHO, Twig templates should be Drupal agnostic to help integrating with templates made for other systems and ease frontenders on-boarding.

The way I suggest to handle this is by preprocessing data coming from Drupal. I wish we had processing plugins but as we don't, this is how I do it in my projects (it's probably missing some edge cases but it's working so far):

In my_module.module:

use Drupal\my_module\Helpers\PatternsHelper;

/**
 * Implements hook_element_info_alter().
 */
function my_module_element_info_alter(array &$info) {
  if (!empty($info['pattern'])) {
    $info['pattern']['#pre_render'][] = [
      PatternsHelper::class,
      'handleMultipleFields',
    ];
  }
}

In src/Helpers/PatternsHelper.php:

<?php

namespace Drupal\kumquat_core\Helpers;

use Drupal\Core\Render\Element;
use Drupal\Core\Security\TrustedCallbackInterface;
use Drupal\ui_patterns\UiPatterns;

/**
 * A helper including some trusted callbacks to pre_render patterns.
 */
class PatternsHelper implements TrustedCallbackInterface {

  /**
   * Force patterns fields content to be usable in twig loops.
   *
   * If they have the "multiple" setting.
   *
   * @param array $element
   *   A render array element.
   *
   * @return array
   *   A render array element.
   */
  public static function handleMultipleFields(array $element) {
    $pattern_definition = UiPatterns::getPatternDefinition($element['#id']);
    foreach ($pattern_definition->getFields() as $field_name => $field) {
      if (!empty($field->toArray()['multiple'])) {
        if (!array_key_exists('#' . $field_name, $element)) {
          continue;
        }

        // Handle use case of multiple fields that get a single scalar value
        // instead of an array.
        if (is_scalar($element['#' . $field_name])) {
          $element['#' . $field_name] = [['#markup' => $element['#' . $field_name]]];
        }

        // Multiple fields in the same area in the UI creates a #sources key. We
        // recreate this key even for single sources to reduce the use cases to
        // manage.
        if (empty($element['#' . $field_name]['#sources'])) {
          $element['#' . $field_name]['#sources'] = $element['#' . $field_name];
        }

        // Get sources to work on.
        $element_sources = $element['#' . $field_name]['#sources'];

        // Drupal expects sources to be render arrays. Ensure we didn't give it
        // scalar values directly in the templates to prevent issues.
        foreach ($element_sources as $key => $value) {
          if (is_scalar($value)) {
            $element_sources[$key] = ['#markup' => $value];
          }
        }

        $values = [];

        $sourceKeys = Element::children($element_sources, TRUE);

        // For each element in the sources we need to extract final values.
        foreach ($sourceKeys as $sourceKey) {
          $source = $element_sources[$sourceKey];

          // The element is a multiple field.
          if (!empty($source['#is_multiple'])) {
            $deltas = Element::children($source, TRUE);

            // Extract common source field properties.
            $source_properties = array_diff_key($source, array_combine($deltas, $deltas));

            // Explode the field values in one field per value and keep
            // common properties in case they contain something important.
            foreach ($deltas as $delta) {
              $values[] = ($source_properties + [0 => $source[$delta]]);
            }
          }

          // The element is a view.
          elseif (!empty($source['#view']) && !empty($source['#rows'])) {
            foreach ($source['#rows'] as $delta => $row) {
              $values[] = $row;
            }
          }

          // The element is empty.
          elseif (array_key_exists('#cache', $source) && !array_key_exists('#type', $source) && !array_key_exists('#theme', $source)) {
            // Do nothing.
          }

          // Fallback for other cases (custom render array).
          else {
            $values[] = $source;
          }
        }

        $element['#' . $field_name] = $values;
      }
    }

    return $element;
  }

  /**
   * {@inheritdoc}
   */
  public static function trustedCallbacks() {
    return [
      'handleMultipleFields',
    ];
  }

}

In a pattern.yml file:

my_pattern:
  label: My Pattern
  fields:
    my_multiple_field:
      type: whatever
      label: My multiple field
      multiple: true
      preview:
        - 'You can'
        - 'Try this'
        - 'At home'

In pattern-my-pattern.html.twig:

<div class="my-pattern">
  {% for value in my_multiple_field %}
    <div class="my-pattern__item">{{ value }}</div>
  {% endfor %}
</div>
</div>

(This helper also have a handleOptionalFields method to be able to use a Twig if on slots and a handleBooleanFields that I don't use that much now we have settings.)

Thanks for your feedback @Duaelfr

I 100% agree with your sentence: "Twig templates should be Drupal agnostic to help integrating with templates made for other systems and ease frontenders on-boarding."

That's the reason why:

• I have proposed to add a test extension coming from Jinja, the "industry standard" template language. By the way, Twig was started as a Jinja clone for PHP, by the Jinja creator.
• I have also proposed to add it upstream at Twig level and I can also do it at Drupal level, because this is a more universal need than UI Patterns only.

You are proposing to introduce a new "multiple" boolean property. This duality "new definition property" VS "twig extensions" was already in ✨ Reduce preprocess hooks usage by adding add_class() & set_attributes() filters Fixed where we were hesitating between "set_attributes" & "add_class" properties VS Twig filters.

IMO both way are valid. However, some issues with the "multiple" boolean property are:

• the lack of control: what about the case where they are 2 nested loops (example: a table where every cell is a slot)?
• the lack of sync: changes on templates may have impact on the definition YML, and may be forgotten

Proposal using only what is already provided by Twig:

  {% set list = ["yo", "foo", "bar"] %}
  {% if list is iterable and (list|keys[0] == 0) %}
    <p>list IS A LIST</p>
  {% else %}
    <p>list IS NOT A LIST</p>
  {% endif %}

Result:

list IS A LIST

  {% set mapping = {"hello": "world", "foo": "bar"} %}
  {% if mapping is iterable and (mapping|keys[0] == 0) %}
    <p>mapping IS A LIST</p>
  {% else %}
    <p>mapping IS NOT A LIST</p>

Result:

mapping IS NOT A LIST

If accepted, there is no code to merge, only documentation ("good practices") to update.

Hi,

I have added some review comments.

But I have not read the previous discussion, so I may be outdated, and finally the MR is useless?

But I have not read the previous discussion, so I may be outdated, and finally the MR is useless?

Hard to tell. In comment #11 I have proposed a solution without MR, but Duaelfr remembered me that some render arrays are starting with "0" (example: facets), so this naive test will not work.

I don't know what to do. No solution seems perfect.

In order of preference:

1. Add Twig tests at Twig level: https://github.com/twigphp/Twig/issues/3828
2. One of those:
3. • Add Twig tests at Drupal level (no issue yet) or UI Patterns level (the current issue MR) but Duaelfr righfuly told us "Twig templates should be Drupal agnostic to help integrating with templates made for other systems and ease frontenders on-boarding."
   • Add a pattern property with a generic preprocess in UI Patterns module, as proposed by Duaelfr in #9

But I have not read the previous discussion, so I may be outdated, and finally the MR is useless?

Hard to tell. In comment #11 I have proposed a solution without MR, but Duaelfr remembered me that some render arrays are starting with "0" (example: facets), so this naive test will not work.

I don't know what to do. No solution seems perfect.

In order of preference:

1. Add Twig tests at Twig level: https://github.com/twigphp/Twig/issues/3828
2. One of those:
3. • Add Twig tests at Drupal level (no issue yet) or UI Patterns level (the current issue MR) but Duaelfr righfuly told us "Twig templates should be Drupal agnostic to help integrating with templates made for other systems and ease frontenders on-boarding."
   • Add a pattern property with a generic preprocess in UI Patterns module, as proposed by Duaelfr in #9
4. Add a Twig filter at UI Patterns level: too specific, and not as nice as Twig tests

Postponed until we get some news from Twig project.

Pull Request was done for Twig: https://github.com/twigphp/Twig/pull/3859/files

Enough is enough :)

Let's add those 2 tests in UI Patterns 1.x and 2.x now instead of waiting https://github.com/twigphp/Twig/issues/3828 is ready, let's implement is sequence and is mapping tests directly in UI Patterns 1.x & 2.x. We will remove them once implemented upstream.

We already sure about the API to add, because it is already available in Jinja and other template engines.

However, is sequence test is not enough when a list of renderables has mapping keys (non consecutive, strings) instead of sequence (integer, consecutive) keys.

For example a list of blocks from page layout or layout builder: each block is keyed by its UUID.

So, normalize this list of renderable to be a proper Twig sequence from the pattern (or component in UI Patterns 2.x) render element itself, as a prerender function, on the first level of each slots, no recursivity.

I will do the 2.x implementation in an other issue. Someone has to take care of 1.x implementation here.

https://github.com/twigphp/Twig/issues/3828 has resumed!

So, if you want to wait the upstream, this issue will only be for the prerender.

Merged: https://github.com/twigphp/Twig/commit/045f5adec773f6430676fdda078b2fa39...

So, we keep this issue for this remaining task.

However, is sequence test is not enough when a list of renderables has mapping keys (non consecutive, strings) instead of sequence (integer, consecutive) keys.

For example a list of blocks from page layout or layout builder: each block is keyed by its UUID.

In other words:

• in Drupal, a unique renderable is always an associative array ("mapping"). In Twig, it is always a mapping. So it is OK.
• in Drupal, a list of renderables can be a list array ("sequence") or sometimes an associative array ("mapping"). In Twig, it must always be a sequence.
• When a list of renderables is an associative array in Drupal, the keys are meaningless, just traversable, and can be removed. So let's remove them before sending them to Twig in a prerender method.
• UI Patterns is a perfect place to do that because we know which variables expects renderables (slots/fields) and which are not (props/settings). It would not be easy at a Drupal core level. Same with ✨ Empty field values when not renderable Active

So, normalize this list of renderable to be a proper Twig sequence from the pattern (or component in UI Patterns 2.x) render element itself, as a prerender function, on the first level of each slots, no recursivity.

2.x implementation has its own specific issue : 📌 [2.0.0-beta1] Add sequence and mapping tests Postponed