Create twig |add_suggestion filter

Initial implementation pushed to the fork. Needs tests.

What do you think about adding a second parameter to inject a class to the attributes? I find that’s often needed in order to easily make use of the template suggestion. I.e., I want something to display with the markup and a component-specific class, like this:

{{ content.field_event__links|as('list', 'event__link-list') }}
{# Produces: #}
<ul class="list event__link-list">
  <li><a href="/">foo</a></li>
  <li><a href="/">bar</a></li>
</ul>

In this example, the list template can be more easily reused because we get both the list markup and a class to adjust styling for the specific placement.

Do you think this is something desirable and worth trying to add it in still?

Fixed the failing test

Change record exists

Just jumping here to mention that this was implemented on a client project we're working on right now with @hawkeye.twolf and it really helped reducing the number of templates needed. Although the most important benefit for me is that it keeps as much logic as possible in the component itself.

I'd like to add another point why this is a great idea:

In our case we have one common override template for a certain number of fields. This template could not be distinguished by common template suggestions based on entity type, bundle or field name. So this led to a weird situation where we had one

shared_special_field_template.html.twig containing the actual template

and about 30-40 field--node--FIELDNAME--BUNDLE.html.twig templates containing only

{% extends 'shared_special_field_template.html.twig' %}

With the proposed|as filter we could have used the node template and spared all the dummy extend templates.

Thank you for catching that, @mherchel! I was being overly strict in the function declaration. I've pushed a change to allow NULL values, so missing content should no longer throw an error. I also added test coverage for the fix. Please re-test with your example when you get a chance, thank you!

@lauriii asked me to have a look at this issue.

This is a really neat idea. I like how it puts the themer in more control and makes certain theming task less complex and easier to learn and do.

The current MR has really nice test coverage of all the logic in the function. Nice.

I chatted to @lauriii about the semantic-ness of as as a filter name. We agreed that this is really nice for someone that does not know much about how Drupal themes work. But, on the other hand, it does not help them really understand what is going on under-the-hood. One option would be to alias the as filter to addSuggestion so then we can be semantic for people who do have this type of knowledge of the theme system. I would also suggest renaming the function to addThemeSuggestion() as that is what it is doing.

The only awkwardness I see in the current patch is were you have multiple theme suggestions... in the test case we have:

          'kitten__stripy__cute',
          'kitten__cute',
          'kitten__stripy',
          'kitten',

This seems quite tricky if kitten__stripy__cute does not exist but kitten__cute and kitten__stripy do. I’m not sure what I’d expect to happen. One on hand, I can imagine being given a task to add stripy kitten template to a theme which has kitten and kitten__cute templates already and so I’d add a kitten__stripy template and then be surprised when it doesn’t work everywhere. But, as @lauriii pointed out to me, he expect that if I have called |as('cute') in a template that I wouldn’t accidentally override that by adding a theme suggestion for stripy.

I think this could be mitigated completely if we only added a single theme suggestion based on the base theme hook. (@lauriii thinks this might be quite hard to do). Then again, maybe this append to everything is quite powerful.

Also one fun thing is that filters can be chained... field|as(‘details’)|as(‘more_details’) - I'm also not sure how I feel about this especially in light of the append this to every suggestion appraoch.

@mherchel my suggestion is that both |as and |addSuggestion work.

Filter renamed from |as to |add_suggestion. Thanks @mherchel @andy_blum @ckrina and @alexpott!

@alexpott, thank you for bringing up the concerns about adding the suggestion to every theme hook, very happy to have additional input! Here's some context from Slack where Lauri and I discussed this question and ended up going with "add to every suggestion". Please let me know if you have additional thoughts or if you end up agreeing with the current approach so we can mark this issue back to RTBC. Thank you! 🙏🏻

Hawkeye Tenderwolf
1:28 PM
ah okay I understand now. “Should it extend the lowest or highest priority existing theme hook” (edited)

Lauri Eskola
1:28 PM
or both
1:28
or all of them

Hawkeye Tenderwolf
1:28 PM
aha! Interesting

Lauri Eskola
1:29 PM
I’d say it should extend the lowest
1:29
I think we call it the base hook

Hawkeye Tenderwolf
1:30 PM
yeah, I think either base or “all of them”.
:heavy_plus_sign:
1

Lauri Eskola
1:30 PM
@mherchel
wondering if you have thoughts on this?
1:31
how about in cases where a single string is passed?
1:31
I guess we could convert it to an array and add it to all levels

Hawkeye Tenderwolf
1:32 PM
If someone specifically wanted the kitten__stripy__cute suggestion, and we only prepend the base hook, they could always do:
content.field_kitten|as('stripy__cute`)
(edited)
:heavy_plus_sign:
1

Lauri Eskola
1:33 PM
I think adding it to the base hook is the 90% use case

Hawkeye Tenderwolf
1:33 PM
What do you mean by this?
how about in cases where a single string is passed?
When would an array get passed?

Lauri Eskola
1:33 PM
‘#theme’ could be either string or array of strings (theme hooks) (edited)
1:34
'#theme' => [
'kitten__stripy',
'kitten',
],
and
'#theme' => 'kitten__stripy'
1:34
both are valid

Hawkeye Tenderwolf
1:34 PM
ah I see what you mean. the question of how to determine what is the “base hook” vs. just the “lowest priority hook”

Lauri Eskola
1:35 PM
yeah, but if it’s string, it’s always what’s before the first __

Hawkeye Tenderwolf
1:36 PM
With:
'#theme' => 'kitten__stripy'
kitten is still technically the base hook. So would the expected result of |as('cute') be
['kitten__stripy__cute', 'kitten__stripy']
or
['kitten__cute', 'kitten__stripy']
1:37
I could see either way. I think I’m finally grokking your question :slightly_smiling_face: I guess always going for the base hook feels most natural. I could update the MR to do that (right now it’s just taking the lowest priority) (edited)

Lauri Eskola
1:38 PM
well technically what I said earlier is not true, lol: https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/lib/Drupal/C...
there’s a lot of complexity there but maybe we can be a bit naive about it
GitLabGitLab
core/lib/Drupal/Core/Theme/ThemeManager.php · 9.5.x · project / drupal · GitLab
For more information about this repository, visit the project page at https://www.drupal.org/project/drupal
1:39
okay let’s look at concrete example. For example 'input__color'

Hawkeye Tenderwolf
1:40 PM
ah i see. I’ve actually done that before, implemented my own theme hook that was technically based off another, like field__kitten so that I could have full control over the theme hook (function template_field__kitten() )

Lauri Eskola
1:40 PM
if you have following code 'input__color'|as('color_picker') what do you expect to happen? (edited)

Hawkeye Tenderwolf
1:41 PM
I would say input__color__color_picker

Lauri Eskola
1:41 PM
I think that probably makes sense
:+1::skin-tone-2:
1

1:43
so I think what is there probably makes sense, but would be interesting to hear from Mike what he thinks

Hawkeye Tenderwolf
1:44 PM
To do “base hook” properly (without the risk of changing from one base hook to another), it would require a BC change to add the theme registry service I think

Lauri Eskola
1:44 PM
I think that all of the cases I saw where we use #theme with a theme suggestions are pretty special and in those it would make sense to have the behavior you said you’d expect
:raised_hands::skin-tone-2:
1

1:45
There’s a way to add constructor arguments but it’s a :hankey: show so I didn’t want us to have to go through that :laughing:

Hawkeye Tenderwolf
1:47 PM
Definitely down to avoid :hankey: shows :)

Mike Herchel
1:49 PM
looking through thread now
1:49
all of the examples make sense that you've discussed

Hawkeye Tenderwolf
1:53 PM
Cool, so if we’re down with keeping “lowest priority” suggestion, I’ll update the MR to remove references to “base” theme hook and rename the variable to make it not misleading.

Lauri Eskola
1:54 PM
that would be great :+1:

Hawkeye Tenderwolf
2:28 PM
I just found something that surprised me…
#theme => kitten__striping__cute
behaves differently than:
#theme => [
kitten__striping__cute
kitten
]
when there’s no “kitten--striping--cute” template. The former automatically includes suggestions for kitten__striping, while the latter doesn’t (see here).
It makes me wonder if we should instead just append the suggestion to the existing theme hook, rather than convert it to an array.
And if the theme hook is already an array, makes me lean more toward “append to all the existing suggestions”, since the fallback mechanisms act differently when the theme hook is a list. (edited)

GitLabGitLab
core/lib/Drupal/Core/Theme/ThemeManager.php · 9.5.x · project / drupal · GitLab
For more information about this repository, visit the project page at https://www.drupal.org/project/drupal

Lauri Eskola
2:29 PM
That probably makes sense. The array of theme suggestions is pretty rare I think. But I’m not sure since there could be some popular use case which I can’t think of where it’s used..

Hawkeye Tenderwolf
2:31 PM
Sweet. I’ll make the change.

Hawkeye Tenderwolf
3:15 PM
Okay one more edge case. So edgey that I may just be overthinking it, so let me know. Let’s say I have a kitten render array that includes striping:
#theme => kitten__striping
And I want to render my kitten as cute so I do:
{{ kitten|as('cute') }}
And I make a template called kitten--cute… It won’t get picked used because the suggestion is looking for kitten--striping--cute
Goes back to the question of “do we append to the base theme hook, or the lowest, or the highest?”
Catching up with your thought process from earlier
@lauriii
, the “naive” approach of just adding them all (splitting by __) :allthethings: In the above example, we would get:
{# Add "cute" suggestion to both "kitten__striping" and "kitten" #}
#theme => [
'kitten__striping__cute',
'kitten__cute',
'kitten__striping',
]
I might just be totally overthinking this :thinking_face:
I am stepping away for a while, will be back later. (edited)

Lauri Eskola
3:23 PM
The use cases I saw for that were along the lines of animal__cat and animal__dog and you would never expect to change animal__cat to animal__dog but do something like animal__cat__cute. But then there’s a question if animal__cute makes sense or not?

Hawkeye Tenderwolf
12:47 AM
In the end I think the best compromise is adding a new suggestion for each existing theme hook, but not to break them apart and add new suggestions for each “level” (__) of each theme hook. I pushed that change, and in so doing obviated 3 tests, since a new suggestion is always added now. I’m not sure that’s a good thing or not :shrug::skin-tone-2: (edited)

Once the CR https://www.drupal.org/node/3301862 is updated for the new name and has something about #28 then I think we're good to go. It's important the CR documents how it affects all existing theme suggestions and talks about the precedence stuff.

Discussed with @lauriii and we agreed that as a pure addition this can go into 10.0.0 and be there from the beginning of Drupal 10.

Can someone add a release notes section?

Committed and pushed d985697c26 to 10.1.x and 75f7ef8b29 to 10.0.x. Thanks!