Create twig filters: |add_class and |set_attribute

Hello,

Tested in three of my D10 projects. Thanks a lot for this strong improvement of the themer experience.

However, a common situation is not handled: a renderable variable can be a list of renderables.

Examples:

[
  [
    '#type' => 'html_tag',
    '#tag' => 'h1',
    '#value' => 'Hello',
  ],
  [
    '#type' => 'html_tag',
    '#tag' => 'h2',
    '#value' => 'World'
  ]
]

Or

[
  [
    '#theme' => 'image',
    '#url' => '/foo/bar.png',
  ],
  [
    '#theme' => 'image',
    '#url' => '/foo/bar2.png',
  ]
]

In this situation, we want the classes and the attributes to be applied to all render arrays of the list.

I have a patch to propose, but i am not able to push my commit (this is my first contribution to a Drupal core issue, so I may do something wrong):

$ git remote -v
origin	git@git.drupal.org:issue/drupal-3301853.git (fetch)
origin	git@git.drupal.org:issue/drupal-3301853.git (push)
$ git push --set-upstream origin 3301853-create-twig-filters__with_array_is_list_loop
remote: You are not allowed to push code to this project.

Anyway, the patch is attached here.

Thank you very much for testing this new filter and for reporting your issue.

IMHO I think you should handle your use case differently. Rather than having a the filter to deal with lists, it is simpler to do the loop in your Twig template directly, such as:

{% for index,item in my_list_of_renderable_arrays %}
  {{ item|add_class('item', 'item--' ~ index) }}
{% endfor %}

Does it work for you?

Bonjour Mathieu,

Thanks for you feedback.

I am afraid it will not work because the themers don't know if the renderable they print is a single render array or a list of them.

For example, without talking about those new filters, because a list of renderable is also a renderable, it is already safer for themers to print directly the variable:

<div class="foobar">
    {{ cta }}
</div>

Without knowing if they are getting something like this:

[
  '#type' => 'custom_button',
  '#label' => 'Hello',
]

Or something like this:

[
  [
    '#type' => 'custom_button',
    '#label' => 'Hello',
  ],
  [
    '#type' => 'custom_button',
    '#label' => 'Hello',
  ]
]

Doing a loop will break the first case, and may be considered as something to avoid:

<div class="foobar">
   {% for button in cta %}
    {{ button }}
   {% endfor %}
</div>

Maybe I am caring that much about this use case because I am working with design system integration with my clients, where the Drupal themer is not aware of what will be the business usage of his/her templates. In a more traditional project, where the theming team is a part of the project team, they can figure it out together, adapting the Twig code to what Drupal is sending.

However, I am convinced that considering this use case will make the filters more solid, and is a must-have for contrib themes and modules, which are also developed ahead of their business usages.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Thanks for the feedback @pdureau! That seems like a valid use case, but OTOH we don't generally apply filters across child elements like what you are proposing. I'm wondering if this could be handled on the render array level by introducing a new type that allows inheriting properties to children?

Something like this:

[
  '#type' => 'inherit',
  '#attributes' => ...
  [
    '#type' => 'custom_button',
    '#label' => 'Hello',
  ],
  [
    '#type' => 'custom_button',
    '#label' => 'Hello',
  ]
]

I guess there could be other ways to create similar functionality. I'm not too keen to what I'm proposed here so if someone has better ideas, I'd be open to that.

I understand the request @pdureau. It would be nice to have a filter which automagically add_class/set_attribute on all children items but I don't think it is a valid use case for this issue specifically.

It is quite a specific need. I don't see why add_class would want iterate on all children everytime it is called. In your example, you could have a details in children elements and you'd want to use add_class to add a class on the details, not on children.

I think what you are looking for is a custom Twig filter - like add_class_recursively or add_class_on_children -
which would iterate on all children items to actually apply add_class/set_attribute. IMHO such a filter should live in a custom module.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Hei Laurii, salut Matthieu,

My request was not about recursively altering all children items, but only the one wrapped into lists (arrays with only continuous numeric keys), without traversing the children keyed with a string. So, it will be less impactful than expected, no mess with with containers, details, fieldsets, lists...

It may look like a specific need, but I met it many times in my projects : When templates are created ahead of their usage and a variable accept a list of renderables, we have no way to be sure the templates usages (presenter templates, render arrays, plugins...) are sending such a list instead of a single renderable.

Thanks for your two proposals (#type' => 'inherit' and add_class_recursively...) but let's put them apart for now.

I will try to do progress on this subject at contrib level (probably in the scope of UI Patterns module), staying compatible with the current core proposal.

Discussed this with @lauriii to figure out what we want to do to move this forward. I've opened up a followup issue to discuss what should happen when we run into an array of render arrays: 📌 Decide how Twig filters should process against an array of render arrays Active

Setting this back to NR for now, since it looks like a random failure kicked it back to NW. I will review this shortly!

Pinging both @nod_ and @ckrina in Slack to get their thoughts (per @lauriii)

This is still looking great! With the array of render arrays discussion moved to 📌 Decide how Twig filters should process against an array of render arrays Active , moving this back to RTBC so we can hopefully get into 10.1.

I'll wait for @nod_ to weight in, but +1 to this change. Also code looks fine for me.

Cool, exciting to see this close to getting into core! I helped @matthieuscarset for quite some time at a previous $dayJob to understand the automated testing changes for this.

I pushed a commit to fix some trivial docs nits I saw in the current MR. I wouldn't downgrade to NR for that. However, Gitlab thinks this needs a rebase, and when I tried to do so locally, the git commits on this MR branch are full of duplicate commits from 10.1.x core. Regular rebase fails miserably, and interactive rebase is huge and tedious to untangle. I don't want to make even more of a mess of this, so I'm going to stop now and see if @mherchel and/or @matthieuscarset can do the needed rebase to get this actually ready to merge.

Thanks!
-Derek

+1 to adding the methods, for the implementation what chx said in chat makes sense: adding a couple of methods to AttributeHelper object. Maybe if not in this issue it can be done in #3314482: Add |remove_class twig filter → .

Assumptions of #44 makes sense to me as well, looking forward to 📌 Decide how Twig filters should process against an array of render arrays Active .

@dww I rebased onto 10.1.x and somehow end up pushing +130 changes 😱

I'm not sure how to undo this.

Any help will be much appreciated.

😆 I've totally done this before! Reminds me of this website: https://ohshitgit.com/

I have some work to do, but afterwards I'll attempt to clean this up if no one beats me to it.

That MR is beyond FUBAR now.

I talked with @lauriii, and he suggested I post a patch with the same changes at the point it was RTBC'd. I'm setting it to NR so @matthieuscarset can verify the changes in the patch are exactly the same as within the MR before the failed rebase.

Tested the patch. It applies successfully on 10.1.x and changes all look correct 👌

Thank you for your help @mherchel

Tests passed. Marking this issue as RTBC

Changing the actual status field to RTBC per #55 😆

Re-confirming the patch in #53 matches what I reviewed and includes the doc fixes I pushed.
$RTBC++;

Also, updating credit:

1. Removing credit for #9: a useless 1-off patch that didn't apply, for an issue already using an MR.
2. Crediting myself for the automated testing help I gave @matthieuscarset (offline from the issue), for the docs nits, reviews, etc.
3. Crediting @Chi for thoughtful reviews.

I'm glad to have discovered this issue, since I created a module, Twig Attributes → , a few years ago to scratch a similar itch. However, my focus was on allowing attributes to be added to child elements, such as a link theme hook in a link field, similar to what @pdureau brought up earlier in this thread. I recently added a new filter to Twig Attributes named add_class, but I'll revert that, since it will conflict with one of the filters being proposed in this issue.

I am curious if there was any consideration given to supporting setting classes or attributes on variables other than attributes, such as title_attributes, content_attributes, or description_attributes, which appear to be the most common attributes variables besides attributes. I didn't see any mention of that option in this issue, but it seems as though it would be useful.

If we wanted to support setting a class or other attribute on variables other than attributes, perhaps the variable name (or part thereof) could be passed in as an optional additional argument that, if omitted, would default to attributes.

{{ content.field_event__links|as('list')|add_class('event__link-list', 'title')|set_attribute('data-kitten', 'mila', 'content') }}

The above would add the event__link-list class to title_attributes and assign "mila" to the data-kitten attribute on content_attributes.

It would be a shame to add this great feature, but only for the one attributes variable.

Thanks @dww for helping with the issue credit and everybody for the work.

@rlhawk I'm sure your suggestions could be addressed in followup issues so we don't block this useful feature any longer. :)

Committed 4d500c5 and pushed to 10.1.x. Thanks all!

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

matthieuscarset → closed merge request !2752