- π¬π§United Kingdom catch
Unpostponed finally.
π Investigate preprocess_field in the comment module Active was dealing with this problem, and we can tidy up that preprocess implementation here probably.
- π¨πSwitzerland berdir Switzerland
This is a lot trickier than the views issue we we just deprecated the suggestions completely. Order in general is not something we can *directly* provide BC for, but we might not actually need to...
The issue summary is very old, unsurprisingly and outdated. multiple entries are no longer there like that, specifically not bundle-only suggestions.
This is the current list:
$suggestions[] = 'field__' . $element['#field_type']; $suggestions[] = 'field__' . $element['#field_name']; $suggestions[] = 'field__' . $element['#entity_type'] . '__' . $element['#bundle']; $suggestions[] = 'field__' . $element['#entity_type'] . '__' . $element['#field_name']; $suggestions[] = 'field__' . $element['#entity_type'] . '__' . $element['#field_name'] . '__' . $element['#bundle'];
(note: this is reversed as the last entry is the most specific)
one issue that's easy to resolve is
$suggestions[] = 'field__' . $element['#field_name'];
, field names are no longer global, so this should not be a thing and we can deprecate this. field type is global and then unique but also least specific, that's fine. However, because we only deprecate the suggestions here, you will only be able to rely on the field type once the field name suggestion is actually removed (D13). Including in core. One option is that we introduce a prefix for the field type and make that$suggestions[] = 'field__type_' . $element['#field_type'];
. But this would force everyone to rename their field type specific templates.This also solves all order problems. entity type + field name is more specific than entity type + bundle.
The next problem is the conflict of entity type + field name/bundle, meaning a field name and bundle of the same name. That seems less common but possible. I wonder if we should just straight up deprecate entity type + bundle. Is it really a common use case to have a template for *all* fields of a given bundle? If someone wants that, they could still introduce that on their own again.
Alternatively, we need to introduce prefixes for these two, so
$suggestions[] = 'field__' . $element['#entity_type'] . '__bundle_' . $element['#bundle'];
. This is what #2831144: Bundle / field name / view mode theme suggestions can conflict β was discussing. Which I think is not a duplicate but a parent/meta issue of this and the entity one. Either for both bundle and field or one only bundle, as that's likely far less used. - π¨πSwitzerland berdir Switzerland
Created an MR as a starting point.
After looking at this, I'm proposing to *not* deprecate and rename field--FIELDNAME.html.twig. That would require us to rename several existing templates, specifically field__comment.html.twig in comment module and it's 10 overrides and the new specific preprocess function and that seems like way more trouble than it's worth. That just means we can only remove that extra check in π Investigate preprocess_field in the comment module Active in D13, but I can very much live with that.
The MR should fail with deprecations on field--comment-body.html.twig in olivero, all others are field type specific or entity type prefixed I think.
- π¨πSwitzerland berdir Switzerland
Well, my suggestion didn't account for the fact that the default field name of the comment field in \Drupal\comment\Tests\CommentTestTrait::addDefaultCommentField() is 'comment', which is exactly what triggers the conflict. it's a comment field, so it's not broken, but of course the deprecation doesn't make a difference between that.
And actually, the default comment field in standard install profile is comment too.
Might need to rename the field type based suggestion after all...
- π¨πSwitzerland berdir Switzerland
I had an idea that I'm not sure is a good one, but it seems to work. Basically, I was thinking that we add some magic to the deprecation logic and if we discover a deprecated suggestion and the next in line is identical, we just skip to that. This means we basically assume that field--comment.html.twig is meant to be about the field type.
I can't think of an edge case where this would be a problem right now, but there might be?
The Needs Review Queue Bot β tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily 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.
- π¬π§United Kingdom catch
#38 sounds like a very nice way of handling this, but I also am unsure whether there's going to be weird edge cases. However even if there are, it might be less disruptive than a complex renaming situation overall.
- π¨πSwitzerland berdir Switzerland
All remaining deprecations were triggered by field--comment-body.html.twig in olivero, renamed that, so if the RNG gods are with us, this might be green.
- π¨πSwitzerland berdir Switzerland
Well, this is awkward, I learned something today. And I keep running into problems here.
By renaming field--comment-body.html.twig to field--comment--comment-body.html.twig, what happens is that this is now triggering preprocess_field_comment functions.
The actual theme suggestions have no assumptions about depth and hierarchy, it's just first match in reverse order wins. Turns out that preprocess discovery doesn't work like that at all. It splits up the template name by "--" and then assumes that earlier parts are it's parents/hierarchy and should be called too. So because the entity type is comment, and the field type is comment, we're running into a different kind of collision.
I've added a workaround to the olivero preprocess, which is essentially the same as comment already has. This is an existing issue and not caused by this directly (except that you have to name your template now in a way to trigger this) But it also kind of defeats the purpose of this issue.
Makes me wonder if we maybe went too far in our approach to preprocess functions and whether it was a good idea to support preprocess for theme suggestions. But that's unlikely to change.
The only other options is that we do in fact rename the field--$type suggestion to field-type-$type. But that's going to cause so many BC issues (preprocess_comment will stop working).
- π¬π§United Kingdom catch
π± [Meta] Make Drupal the first "design-system native" CMS + Unify & simplify render & theme systems Active has on its list getting rid of theme suggestions altogether. I'm not really sure how that fits into things but just noting that they potentially could end up on their way out.