[2.0.0-alpha3] Replace component() function by native Twig mechanisms?

Created on 24 May 2024, 11 months ago

Problem/Motivation

In UI Patterns, we were using pattern() custom Twig function, with those parameters:

  • id: string
  • fields (slots) & settings (props): associative array
  • variant: string
pattern('nav_item', {
    'attributes': attributes,
    'active': is_active,
    'link': link
})

SDC community is currently using existing Twig mechanisms.

embed tag:

{% embed 'governor:usa_accordion_item' with {
  attributes,
  open: (block_attributes.open) ? 'true' : 'false',
  heading: block_attributes.heading,
  content: block_content,
} only %}
  {% block content %}
    {{ content }}
  {% endblock %}
{% endembed %}

include function or include tag :

  {{ include('governor:usa_pagination', {
    pages: items.pages,
    first: items.first,
    last: items.last,
    previous: items.previous,
    next: items.next,
    ellipsis_previous: ellipses.previous ? true : false,
    ellipsis_next: ellipses.next ? true : false,
    current: current,
    last_page: last_page,
  }, with_context = false) }}

{% include 'kinetic:form' with { attributes, children } %}

It is not OK because they are doing template to template calls, not template to components. They don't trigger the render element and the related processing.

In UI Patterns 2, we have currently implemented a custom component() Twig function, with those parameters:

  • component: string
  • slots: associative array
  • props: associative array
{{ component('sdc_examples:my-button', { text: 'Click Me'}, { iconType: 'external' }) }}

It works well, but it is a custom fucntion, too specific.

Proposed resolution

Is it specify to alter the behaviour of embed tag and/or include tag and/or include function to make it pass through the SDC render element instead of calling directly the template? A bit like what Core is already doing with print nodes: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...

Will it not break the existing SDC themes which have a "liberal" use of SDC components (there are errors in defintiions, but they don't trigger them because they do template to template calls)?

Can we remove component() function? Can we convert pattern() to one of this Twig native tags/functions?

📌 Task
Status

Active

Version

2.0

Component

Code

Created by

🇫🇷France pdureau Paris

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @pdureau
  • Assigned to mogtofu33
  • Status changed to Postponed 10 months ago
  • 🇫🇷France pdureau Paris

    Postponed because there is no clear solution now.

    Jean: "We can override embed and include in Twig, but the props & slots are not easily available once they are transformed as variable in the Twig template."

  • Issue was unassigned.
  • 🇫🇷France mogtofu33

    Here is a starting point before reaching the problem of re-mapping props and slots from Twig Node to Drupal render array API.

    And another issue is how to pass the render array to Twig for render throw `render_var`.

  • Status changed to Active 8 months ago
  • 🇫🇷France pdureau Paris

    Time flies. We have other priorities for beta2

  • 🇫🇷France pdureau Paris

    In a way, it is more or less what SDC is doing:

    ComponentsTwigExtension is adding 2 twig functions:

    • add_component_context which is adding the attribute Attribute object if missing
    • validate_component_props which is executing the JSON schema validator

    They are not supposed to be used by templates owner, there exists because ComponentNodeVisitor is printing those Twig functions on every template, which will be executed on rendering in this order:

    1. attach_library
    2. add_component_context
    3. validate_component_props

    In order to make the use of "template to template" Twig functions or tags (include and embed) more similar to the use of the Render API, leveraging the render element. For example, attach_library is used here because #attached is not executed.

  • 🇧🇪Belgium oldeb Namur 🇧🇪
  • 🇧🇪Belgium oldeb Namur 🇧🇪
  • 🇫🇷France pdureau Paris

    Ok, i will give a try

  • Merge request !262First proposal for include → (Open) created by pdureau
  • 🇫🇷France pdureau Paris

    First proposal for include: https://git.drupalcode.org/project/ui_patterns/-/merge_requests/262/diffs

    It was the easy one. embed will be an harder challenge

  • Pipeline finished with Failed
    5 months ago
    Total: 659s
    #339025
  • 🇫🇷France pdureau Paris

    pdureau changed the visibility of the branch 3449653-only-twig-visitor to hidden.

  • 🇫🇷France pdureau Paris

    pdureau changed the visibility of the branch 3449653-only-twig-visitor to active.

  • Pipeline finished with Failed
    5 months ago
    Total: 217s
    #341425
  • Pipeline finished with Failed
    5 months ago
    Total: 325s
    #341426
  • Pipeline finished with Failed
    5 months ago
    Total: 325s
    #342000
  • 🇫🇷France pdureau Paris

    Another branch, so another MR, with another proposal: https://git.drupalcode.org/project/ui_patterns/-/merge_requests/267

    Instead of calling the render element from include function, include tag and embed tag, we stay closer to the current SDC implementation:

    1. reorganize our Twig modules visitors to be run before and after the SDC one
    2. move some logic from the render element to an internal Twig function run by one of the Twig modules visitors
    3. promote include() function as a replacement of component() function

    Why include() function? Because:

    The crazy, ambitious, change is not forgotten and will be discusse din the Core issue: 📌 Trigger SDC render element when using Twig include or embed Active

  • Pipeline finished with Success
    5 months ago
    Total: 373s
    #342008
  • Pipeline finished with Success
    5 months ago
    Total: 405s
    #342130
  • First commit to issue fork.
  • Pipeline finished with Success
    5 months ago
    Total: 331s
    #343898
  • 🇫🇷France just_like_good_vibes PARIS

    I really like the new direction, because it will cope with current (bad) usages out in the wild and we will use a twig function instead of a custom twig function.

    What i would expect in addition to this work, is a little bit of automatic testing. may i help with that maybe?

  • 🇫🇷France just_like_good_vibes PARIS

    if you agree, i can continue the work in the MR and add new kernel tests.

  • Status changed to Needs work 5 months ago
  • 🇫🇷France pdureau Paris

    yes, please, add the kernel tests because I never done such a thing

  • 🇩🇪Germany Christian.wiedemann

    Yes that is the way! Right now we "only" normalize the prop values. Shouldn't we support full UI Patterns support like.

    {% embed "module:component" with { 
      "ui_patterns": {
        props: {
            prop1: {
               value: value
            }
        }
      }
    }
    

    So we move the preparing logic from the element to twig. Not sure if it goes too but from an architectural point of view it is straight forward and opens new possibilities.

  • 🇫🇷France pdureau Paris

    Hi Christian, it is great you like the change.

    The data structure you are showing here is not compatible with the issue goal: being compatible with existing Twig mechanisms, to be compatible with the (kind of sad) state of existing SDC themes.

    So, it must be this instead:

    {% embed "module:component" with { 
            prop1: value
        } %}
    

    Also, in your example, you used embed which is a Twig tag we are not promoting. Our change is compatible with all Twig mechanisms but we are promoting only include() function because:

    So, it must be this instead:

    {{ include("module:component", { prop1: value }) %}
    
  • Pipeline finished with Failed
    5 months ago
    Total: 335s
    #349201
  • Pipeline finished with Canceled
    5 months ago
    Total: 109s
    #349207
  • Pipeline finished with Failed
    5 months ago
    Total: 425s
    #349212
  • Pipeline finished with Failed
    5 months ago
    Total: 534s
    #349577
  • Pipeline finished with Success
    5 months ago
    Total: 522s
    #349798
  • 🇫🇷France just_like_good_vibes PARIS

    with a few tests now, let's merge this :D

  • 🇫🇷France pdureau Paris

    Instead of adding this:

        if (!isset($context['attributes'])) {
          $context['attributes'] = [];
        }

    I beleive this would be better to move here a few lines from ComponentElementAlter::

        // Attributes prop must never be empty, to avoid the processing of SDC's
        // ComponentsTwigExtension::mergeAdditionalRenderContext() which is adding
        // an Attribute PHP object before running the validator.
        $element["#props"]["attributes"]['data-component-id'] = $component->getPluginId();
    
  • 🇫🇷France pdureau Paris

    pdureau changed the visibility of the branch 3449653-only-twig-visitor to hidden.

  • 🇫🇷France pdureau Paris

    pdureau changed the visibility of the branch 3449653-only-twig-visitor to active.

  • Pipeline finished with Failed
    4 months ago
    Total: 369s
    #350533
  • Pipeline finished with Canceled
    4 months ago
    Total: 75s
    #350550
  • Pipeline finished with Success
    4 months ago
    Total: 332s
    #350551
    • pdureau committed a24adc27 on 2.0.x
      Issue #3449653 by pdureau, just_like_good_vibes: Replace component()...
    • pdureau committed b7439f45 on 2.0.x
      Issue #3449653 by pdureau, just_like_good_vibes: Replace component()...
  • 🇫🇷France Grimreaper France 🇫🇷

    Hi,

    Possible to change a little bit the warning message to have "...function and will be removed before UI Patterns 2.0.0."?

    The strong tag is not needed, only for this issue queue comment.

  • Pipeline finished with Failed
    4 months ago
    Total: 662s
    #358256
  • Pipeline finished with Failed
    4 months ago
    Total: 93s
    #358368
  • Pipeline finished with Failed
    4 months ago
    Total: 92s
    #358370
  • Pipeline finished with Failed
    4 months ago
    Total: 687s
    #358384
  • Pipeline finished with Success
    4 months ago
    Total: 1164s
    #358431
  • Pipeline finished with Skipped
    4 months ago
    #360799
  • 🇯🇴Jordan rahaf albawab Amman

    Hi

    I'm trying to use pattern and component function in my twig template but I got this error, why?

    Twig\Error\SyntaxError: Unknown "pattern" function. Did you mean "_ui_patterns_normalize_props", "_ui_patterns_preprocess_props"? in Twig\ExpressionParser->getFunction() (line 95 of themes/custom/x/components/organisms/nav/nav.twig).

  • Pipeline finished with Success
    3 months ago
    Total: 151s
    #402867
  • Pipeline finished with Success
    3 months ago
    Total: 152s
    #402875
  • Pipeline finished with Success
    about 2 months ago
    Total: 166s
    #425150
  • Pipeline finished with Success
    about 2 months ago
    Total: 160s
    #425152
Production build 0.71.5 2024