Create twig filters: |add_class and |set_attribute

Something to take into account on the implementation is that we should probably support attributes both in arrays and in \Drupal\Core\Template\Attribute object.

Starting to work on this issue

Implemented the add_class filter in this PR.

Here's an example of the markup I used to test this filter:

{# node--article--full.html.twig #}
<article{{ attributes }}>
  <div{{ content_attributes.addClass('node__content') }}>
    {% set text = {'#markup': 'This is a test string'} %}
    {{ text|add_class('my-class') }}

    {{ 'This is a string'|add_class('my-class') }}
    {{ 'This is a translated string'|t|add_class('my-class') }}
    {{ label|add_class('node-title', 'another-class-for-the-title-which-is-already-a-printed-markup') }}
    {{ content.field_media_image|add_class('node-image') }}
    {{ content.body|add_class('I like my body') }}
  </div>
</article>

Pending on creating functional tests but I wanted to have this code reviewed before going any further 🤷.

Fix Custom command error. please review

All filters are implemented and work as expected - based on local testing with this kind of markup:

{# Testing `add_class` and `remove_class` filter. #}
<div>
  {% set text = {'#markup': 'This is a piece of text', '#printed': true} %}
  {{ text|add_class('master', 'piece', 'mind')|remove_class('piece') }}
  {# result: <div class="master mind">This is a piece of text.</div> #}

  {{ 'Monday'|t|add_class('best-day-of-the-week') }}
  {# result: <div class="best-day-of-the-week">Monday</div> #}

  {{ content.body|add_class('I like my body') }}
  {# result: <div class="class="i-like-my-body clearfix text-formatted field field--name-body [...]">Content of the body...etc</div> #}
</div>

{# Testing `set_attribute`. #}
<div>
  {% set img = {
    '#theme': 'image',
    '#uri': 'https://images.unsplash.com/photo-1508280756091-9bdd7ef1f463?auto=format&fit=crop&w=1216&q=10'
  } %}
  {{ img|set_attribute('alt', 'Happy dog'|t) }}
  {# result: <img alt="Happy dog" src="https://images.unsplash.com/photo-1508280756091[...]"> #}

  {{ 'Friday'|t|set_attribute('title', "No deployments today"|t) }}
  {# result: <div title="No deployments today">Friday</div> #}
  
  {% set body_settings = {'customProp': true} %}
  {{ content.body|set_attribute('data-settings', body_settings|json_encode) }}
  {# result: <div data-settings="{&quot;customProp&quot;:true}" class="clearfix text-formatted [...]">Content of the body...etc</div> #}
</div>

Still pending:

• Code review
• Implementing functional tests in core/tests/Drupal/Tests/Core/Template/TwigExtensionTest.php

Thank you for this review.

As far as I know, there's no way to actually remove a class from {{ content.field_XXX }} (e.g. from a Node template) because it's a render array without any #attributes yet. Here's an example of the content the render array:

In your test, I understand you want to remove the .field class but this class does not actually exist yet - it will be added within the field.html.twig template later during the rendering process.

I don't see solutions here. Maybe someone else has an idea/workaround?

Regarding set_attribute filter, I thought it was useful to allow the NULL value as it gives the possibility to remove an attribute but as mentioned above, this won't necessarily work if you target an attribute not already rendered. So your comment makes me think that we should set the attribute value as an empty string by default so that it is actually set. Example: {{ content.field_test|set_attribute('data-custom-attribute', '') }} will result in <div data-custom-attribute></div>.

Committing this change now.

Both |add_class and |set_attribute filters have been rewritten to match existing code/methods.

Now working on Unit tests.

Happy to say add_class and set_attribute filters are done and Unit tests implements for both.

MR currently mergeable so ready for review👌

I think these filters should not care about cleaning and validation provided values. They are meant for developers and should set exactly what was given to them because magic may confuse. Garbage in, garbage out.

Meanwhile updating element attributes can be done with Twig Tweak.
{{ element|with(['#attributes', 'name'], 'value') }}

Thank you very much for the review and comments. I'll reply in Gitlab.

I agree there are better/quicker/more advanced filters in Twig Tweak. @Chi You might want to port |with filter here - in another PR - to replace add_class and set_attribute?

with cannot fully replace add_class as it requires knowing array index, i.e. class position.

Also I propose we do not reference filters here as |filter_name, it's a bit confusing and not consistent with documentation in other places. )))

assert(is_array($element), 'Invalid target for the "|add_class" Twig filter; element must be an array');

Answering #20: The idea with allowing NULL value in |set_attribute('alt') was to be able to reset an attribute. I might be wrong but if an attribute is null it will ultimately not be rendered in the HTML. Therefore we empower themers by allowing the removal of existing attributes.

Answering #21: Thanks for the explanation - I understand add_class is still a relevant filter to have in core. About mentioning the filter name in assert, I thought the same in the first place but I've seen other assertions with such mentions so I tried to stay consistent and did the same (e.g. suggestThemeHook()).

@matthieuscarset I requested native typehints because #3154762: [policy, no patch] All new Drupal 10 code should use appropriate type hints wherever possible is basically accepted. It's just waiting for change record.

A note of the classes parrameter.

They could be documented this way.

string[]|string ...$classes

Cast the value to string in case it implements MarkupInterface.

To clarify. The docblock stays that $classes variable is either a variadic string or array of strings. There is no chance for it to be an object.

If we do need to support stringable object then the documentation needs to be updated. Something like this.

string[]|\Stringable[]|string|\Stringable ...$classes

And native PHP typehint could be like follows.

array|string|\Stringable ...$classes

The documentation could be a bit simpler if we use is_variadic option.

Failing test has been fixed but latest commit does not appear on the MR.

Commit is here though 🤷 Not sure if there is something I should do to force update the MR or if it is just a matter of time.

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.

This looks like a fantastic feature!

Reviewing MR 2752 and the changes look good.

Think with this new filter it will need a change record (maybe release note?)

Tested this out by altering a template in olivero

node--teaser.html.twig by adding {{ content.field_image|add_class('my-class')|set_attribute('aria-hidden', 'true') }}

When I checked on Drupal 10.1.x with a standard install I can see that a class and attribute is being added correctly..

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.

Example:

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

Or simply this:

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

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 container, a details, a fieldset, a list...etc with children elements. In this case, the themer want to use add_class to alter the container, all the potential existing children items.

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 ones 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 "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 expect a list of renderables, we have no way to be sure the templates "users" (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.

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 #3354683: Decide how Twig filters should process against an array of render arrays.

@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.

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

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!