Problem/Motivation

We use preprocess hooks in UI Suite themes and it appears we are always doing the 2 same stuff:

Preprocess are PHP function and are not expected to be written by the front developer, so they reduce the ownership of the design system maintainer, so let's remove them.

Proposed resolution for: the theme base path

We can take inspiration from ECA module and its use of tokens: https://www.drupal.org/project/eca/issues/3278918
However, there is token to get theme path in Drupal Core and we may need take inspiration from: https://www.drupal.org/project/theme_filter

card:
  label: Card
  fields:
    media:
      label: Media
      preview:
        - type: html_tag
          tag: img
          attributes:
            src: "[path-to-theme:absolute]assets/card.png"

Proposed resolution for: adding attributes to a children component

It may be managed by #3301853: Create twig filters: |add_class and |set_attribute so we need to evaluate those new filters.

We can also introduce new properties (add_class, set_attribute...) and namespacing in pattern definitions. Example:

card:
  label: Card
  fields:
    action_buttons:
      type: render
      add_class: 
      - mdc-card__action
      set_attribute: 
        role: action
      preview:
        - type: "pattern_preview"
          id: "button"
        - type: "pattern_preview"
          id: "button"
 
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:

Comments

pdureau created an issue. See original summary.

grimreaper’s picture

Issue tags: +Prague2022

Let's use set_attributes and not add_class to ensure to have this level of genericity.

g4mbini’s picture

pdureau’s picture

Issue summary: View changes
pdureau’s picture

pdureau’s picture

Evaluation of the 2 filters proposed by #3301853: Create twig filters: |add_class and |set_attribute and planned for Drupal 10.1.

They work correctly only when the twig variable is a single render array, and not a list of render arrays (which is a very common use case).

So, I will propose un patch for https://git.drupalcode.org/issue/drupal-3301853/-/blob/3301853-create-tw... to loop on array items is the array is a list:

  public function addClass(array $element, ...$classes): array {
    if (array_is_list($element)) {
      foreach ($element as $index => $item) {
        if (!\is_array($item)) {
          continue;
        }
        $element[$index] = $this->addClass($item, $classes);
      }
      return $element;
    }
    $attributes = new Attribute($element['#attributes'] ?? []);
    $attributes->addClass(...$classes);
    $element['#attributes'] = $attributes->toArray();

    // Make sure element gets rendered again.
    unset($element['#printed']);

    return $element;
  }

  public function setAttribute(array $element, string $name, mixed $value = NULL): array {
    if (array_is_list($element)) {
      foreach ($element as $index => $item) {
        if (!\is_array($item)) {
          continue;
        }
        $element[$index] = $this->setAttribute($item, $name, $value);
      }
      return $element;
    }
    $element['#attributes'] = AttributeHelper::mergeCollections(
      $element['#attributes'] ?? [],
      new Attribute([$name => $value])
    );

    // Make sure element gets rendered again.
    unset($element['#printed']);

    return $element;
  }

If this patch is accepted, there is nothing to do on ui_patterns side.

For example, for https://git.drupalcode.org/project/ui_suite_bootstrap/-/tree/4.0.x/templ... we can replace the preprocess by the add_class filter:

<figure{{ attributes.addClass('figure') }}>
  {{ image|add_class('figure-img') }}
  <figcaption class="figure-caption">
    {{ caption }}
  </figcaption>
</figure>
pdureau’s picture

Title: Move preprocess hooks to patterns definition » Remove the need of preprocess hooks
pdureau’s picture

Maybe the 2 new Drupal 10.1 Twig filters are not such a good idea, even if our patch is accepted.

A lot of filters are "typed": they must be applied on specific types. Examples:

  • capitalize on string variables
  • column on array variables
  • abs on number variables

We tests several Twig filters on UI Patterns fields ("slots"), applying them to the wrong type:

  • either it looks weird. Example: clean_class on a render array
  • either it makes a fatal PHP error. Example: column on a string

We are fortunate the UI Suite themes are currently only using filters on settings ("props"),not on "fields" ("props")
Except DSFR, but there is a ticket about that: #3341139: [beta1] ⚠️ Fix compatibility with layout builder

We can therefore promote this ("no filters on slots") as an official good practice

So, let's explore the new YAML definitions properties (add_class, set_attribute...) again.

pdureau’s picture

First MR: https://git.drupalcode.org/project/ui_patterns/-/merge_requests/22

It is just an humble proposal for now. Let's discuss.

Wih this MR, when we have this markup to integrate:

<figure class="figure">
  <img src="..." class="figure-img img-fluid rounded" alt="...">
  <figcaption class="figure-caption">A caption for the above image.</figcaption>
</figure>

In order to inject the figure-img class to the children render element, we can avoid an hook like ui_suite_bootstrap_preprocess_pattern_figure by adding the class into the definition YAML:

figure:
  label: "Figure"
  description: "Anytime you need to display a piece of content—like an image with an optional caption, consider using figure. https://getbootstrap.com/docs/4.6/content/figures/"
  fields:
    image:
      type: "render"
      label: "Image"
      description: "Figure image"
      add_class:
       - 'figure-img'
      preview:
        - theme: "image"
          uri: assets/figure-image.png
    caption:
      type: "render"
      label: "Caption"
      description: "A caption under the image."
      preview: "A caption for the above image."
pdureau’s picture

Status: Active » Needs review

Feel free to share feedbacks

pdureau’s picture

Status: Needs review » Needs work
pdureau’s picture

Because we are considering filters on slots again in #3342390: Make the Twig loops safer we can propsoe a Twig filrer instead of altering the render element.

Instead of relying on #3301853: Create twig filters: |add_class and |set_attribute which will have those issues:

  • the filters don’t loop, Pierre has proposed a patch but it seesm it will not be accepted.
  • they accept only array, so they break on strings, so they are not suitable for slots.
  • the filters will not be available on D9

We implement our own versions in UI Patterns:

  • Testing if array and if list (with PHP 7.4 compatibility)
  • Overriding the Drupal core implementation
  • Compatible with the Drupal core implementation

So :

  • We don’t need to introduce new pattern fields properties and the template owner keep the control
  • We will be compatible with D9

pdureau’s picture

Assigned: pdureau » grimreaper
Status: Needs work » Needs review

New MR: https://git.drupalcode.org/project/ui_patterns/-/merge_requests/25

First commit, 1e4d214a3d9 : Add set_attribute and add_class filters, exactly as proposed in #3301853: Create twig filters: |add_class and |set_attribute, included the unit tests.

Second commit, 7874e0c5e90 : with expected changes for UI Patterns:

  • Slot friendly: the filter loop on lists and don't break on scalars
  • Override the Drupal core implementation when both are present, and stay compatible with it

Compliant with feedbacks from Grimpreaper:

  • Compatible with PHP 8.0 and Drupal 9
  • Moved to a trait so it can be used in custom code or somewhere else.

This MR replace the one from comment #10, and its branch ("3311480-remove-the-need") replace "pierre_proposal" branch.

pdureau’s picture

Title: Remove the need of preprocess hooks » Reduce preprocess hooks usage by adding add_class() & set_attributes() filters

I rename the issue, because we haven't deal with the theme base path subject for a long time. Let's create a new specific issue if there are some progress.

grimreaper’s picture

Assigned: grimreaper » pdureau
Status: Needs review » Needs work

@pdureau thanks for your work in this issue.

Maybe an additional test to add and some minor comments to update.

For the PHPCS part, I will do it when the other review points are done.

pdureau’s picture

Assigned: pdureau » grimreaper

Done.

2 new tests:

$ ../vendor/bin/phpunit -c core/ modules/local/ui_patterns-3311480/tests/src/Unit/Template/TwigExtensionTest.php 
Testing Drupal\Tests\ui_patterns\Unit\Template\TwigExtensionTest
..........                                                        10 / 10 (100%)
Time: 00:00.012, Memory: 10.00 MB

PHPCS is OK:

$ ../vendor/bin/phpcs --standard=Drupal modules/local/ui_patterns-3311480/tests/src/
$ ../vendor/bin/phpcs --standard=Drupal modules/local/ui_patterns-3311480/src/Template/
pdureau’s picture

Status: Needs work » Needs review
grimreaper’s picture

Assigned: grimreaper » pdureau
Status: Needs review » Needs work
pdureau’s picture

Assigned: pdureau » grimreaper
Status: Needs work » Needs review

Finally, I have removed TwigExtensionTestString which is not used for testing.

Tests are still OK:

$ ../vendor/bin/phpunit -c core/ modules/local/ui_patterns-3311480/tests/src/Unit/Template/TwigExtensionTest.php 
Testing Drupal\Tests\ui_patterns\Unit\Template\TwigExtensionTest
..........                                                        10 / 10 (100%)
Time: 00:00.012, Memory: 10.00 MB

PHPCS is still OK:

$ ../vendor/bin/phpcs --standard=Drupal modules/local/ui_patterns-3311480/tests/src/
$ ../vendor/bin/phpcs --standard=Drupal modules/local/ui_patterns-3311480/src/Template/
grimreaper’s picture

Assigned: grimreaper » pdureau
Status: Needs review » Needs work

Some review comments remains.

pdureau’s picture

Assigned: pdureau » grimreaper
Status: Needs work » Needs review

  • Grimreaper committed 99e98c73 on 8.x-1.x authored by pdureau
    Issue #3311480 by pdureau, Grimreaper, DuaelFr: Reduce preprocess hooks...
grimreaper’s picture

Version: 8.x-1.x-dev » 2.0.x-dev

Merged on 8.x-1.x. Cherry-picking on 2.0.x.

  • Grimreaper committed 5597c7b4 on 2.0.x authored by pdureau
    Issue #3311480 by pdureau, Grimreaper, DuaelFr: Reduce preprocess hooks...

grimreaper’s picture

Assigned: grimreaper » Unassigned
Status: Needs review » Fixed

Merged on 2.0.x too.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

sharique’s picture

Documentation and examples update is required for this.