Problem/Motivation

Our understanding of Twig practices has improved since the first release of UI Patterns:

  1. 2017. At the beginning, a pattern template was considered as a regular Twig template, with a robust pattern() function instead of the block, include, sandbox, use tags and include, source and blocks functions.
  2. Then, we considered that hardcoding such a nesting from the templates was bad practice, and even pattern() must be avoided most of the time. It is better inject them from outside.
  3. October 2018. Patterns Variants were introduced, cleaning the fields list by removing the "weird" one.
  4. June 2020. ui_patterns_settings was released and we were able to move the variables with "controlled values" (list of values, boolean, strong typing...) out of the fields list.
  5. October 2022. We introduced during a Monthly the "slots" (UI Patterns Fields) VS "props" (UI patterns Settings), which provide us a mental framework to build our patterns definitions.
  6. February 2023. We have noticed that we are never using Twig filters with slots, and we can promote this as a good practice

Today, the front dev has a very good tool to build its pattern in a safe and efficient way.

However, there is still an issue: looping on a slot is a risky thing.

For example, this will break if 'slides' value is not a list but a single renderable. :

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

See also this comment: https://www.drupal.org/project/drupal/issues/3301853#comment-14901185

Proposed resolution

It is not possible to use a iterable test because render arrays are considered as iterable :

{% set slides = (slides is iterable) ? slides : [slides] %} 
{% for slide in slides %}

function twig_test_iterable($value)
{
    return $value instanceof \Traversable || \is_array($value);
}

It may not be advisable to create a new filter (which will wrap the value in a list if its not a list) if we said before they are bad practices on slots:

{% for slide in slides|as_list %}

And this may leverage a PHP 8.1 function not compatible with Drupal 9: https://www.php.net/manual/en/function.array-is-list.php

Remaining tasks

So, someone has an idea? ;)

Issue fork ui_patterns-3342390

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

pdureau created an issue. See original summary.

pdureau’s picture

pdureau commented
Issue summary: View changes
pdureau’s picture

pdureau commented

Proposal with 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.

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 with a "sequence" test

Like in Jinja: https://jinja.palletsprojects.com/en/3.0.x/templates/#jinja-tests.sequence

<div class="carousel-inner" role="listbox">
  {% set slides = (slides is sequence) ? slides : [slides] %}
  {% for slide in slides %}
    <div class="carousel-item">
      {{ slide }}
    </div>
  {% endfor %}
</div>
pdureau’s picture

pdureau commented
Assigned: Unassigned » pdureau

pdureau opened merge request !26

pdureau’s picture

pdureau commented

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.

pdureau’s picture

pdureau commented
Status: Active » Needs review
pdureau’s picture

pdureau commented

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

duaelfr’s picture

duaelfr
he/him
Primary language French
Location Montpellier, France
commented

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>

(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.)

pdureau’s picture

pdureau commented

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 #3311480: Reduce preprocess hooks usage by adding add_class() & set_attributes() filters 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
pdureau’s picture

pdureau commented

Proposal using only what is already provided by Twig:

  {% set list = ["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.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

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?

pdureau’s picture

pdureau commented
Status: Needs review » Needs work

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:
    • 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
  3. Add Twig filters at UI Patterns level: too specific and not as nice as the Twig tests solution (which is similar to Jinja)
pdureau’s picture

pdureau commented
Status: Needs work » Postponed

Postponed until we get some news from Twig project.

pdureau’s picture

pdureau commented

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

pdureau’s picture

pdureau commented
Assigned: pdureau » Unassigned
Status: Postponed » Active

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.

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). Itr would not be easy at a Drupal core level. Same with #3383544: [2.0.0-beta5] Empty field values when not renderable

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: #3422265: [2.0.0-beta1] Normalize list of renderables in slots

Someone has to take care of 1.x implementation here.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Assigned: Unassigned » grimreaper
pdureau’s picture

pdureau commented

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.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Assigned: grimreaper » Unassigned
pdureau’s picture

pdureau commented

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 #3383544: [2.0.0-beta5] Empty field values when not renderable

So, normalize this list of renderable to be a proper Twig sequence from the patternrender element itself, as a prerender function, on the first level of each slots, no recursivity.

Be careful to not break layout builder :)

2.x implementation has its own specific issue : #3422265: [2.0.0-beta1] Normalize list of renderables in slots

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Assigned: Unassigned » grimreaper
pdureau’s picture

pdureau commented

Done for UI Patterns 2.x: https://git.drupalcode.org/project/ui_patterns/-/commit/c66fc5d0e4eb9a59...

Grimreaper closed merge request !26

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Assigned: grimreaper » Unassigned
Status: Active » Closed (won't fix)

Discussed with @pdureau, possible in UI Patterns 2 because data structure is will defined. But UI Patterns 1 is too permissive and we cannot be sure of what we are manipulating.