Problem/Motivation

Right now, EntityViewBuilder always uses $entity_type_id as #theme. #2698909: EntityViewBuilder uses non-existing #theme hooks corrects that a bit and doesn't use it when no such template exists.

However, to view an entity, you still need to implement hook_theme(), define a template, provide a default template and if you want to do it like node, implement quite some preprocess logic, theme suggestions and so on.

Proposed resolution

This proposes to introduce a standard template, with the common preprocess stuff and theme suggestions.

Instead of node.html.twig, you would then use entity--node.html.twig and so on.

That removes all the unecessary glue code that is currently necessary to have useful entity templates.

Previous attempts at standardizing this failed (#2023571: Support preprocessing in EntityViewBuilder, #2270883: Automatically add theme hook suggestions for all entity types), this basically replaces those issues. Those issues were complicated because it affects existing templates, so there is a risk of changes and BC breaks.

Remaining tasks

User interface changes

None

API changes

A new template.

Data model changes

CommentFileSizeAuthor
#39 introduce_generic-2808481-39-interdiff.txt3.7 KBberdir
#39 introduce_generic-2808481-39-do-not-test.patch24.93 KBberdir
#39 introduce_generic-2808481-39.patch28.33 KBberdir
#35 introduce_generic-2808481-35.patch24.22 KBjoelpittet
#35 interdiff.txt1.46 KBjoelpittet
#33 introduce_generic-2808481-33.patch24.22 KBjoelpittet
#33 interdiff.txt392 bytesjoelpittet
#29 interdiff.txt2.62 KBjoelpittet
#29 introduce_generic-2808481-29.patch24.21 KBjoelpittet
#27 introduce_generic-entity-template-2808481-26-interdiff.txt994 bytesberdir
#27 introduce_generic-entity-template-2808481-26.patch24.4 KBberdir
#24 introduce_generic-entity-template-2808481-24-interdiff.txt4.02 KBberdir
#24 introduce_generic-entity-template-2808481-24.patch23.42 KBberdir
#19 introduce_generic-entity-template-2808481-19-interdiff.txt1.48 KBberdir
#19 introduce_generic-entity-template-2808481-19.patch23.84 KBberdir
#17 introduce_generic-entity-template-2808481-17-interdiff.txt653 bytesberdir
#17 introduce_generic-entity-template-2808481-17.patch22.36 KBberdir
#14 introduce_generic-entity-template-2808481-13.patch22.36 KBberdir
#14 introduce_generic-entity-template-2808481-13-interdiff.txt12.52 KBberdir
#14 introduce_generic-entity-template-2808481-11.patch15.41 KBberdir
#11 introduce_generic-entity-template-2808481-11-interdiff.txt1.6 KBberdir
#11 introduce_generic-entity-template-2808481-11.patch15.41 KBberdir
#9 introduce_generic-entity-template-2808481-8-interdiff.txt559 bytesberdir
#9 introduce_generic-entity-template-2808481-8.patch15.38 KBberdir
#7 introduce_generic-entity-template-2808481-7-interdiff.txt794 bytesberdir
#7 introduce_generic-entity-template-2808481-7.patch11.22 KBberdir
#5 introduce_generic-entity-template-2808481-5-interdiff.txt4.07 KBberdir
#5 introduce_generic-entity-template-2808481-5.patch10.45 KBberdir
#3 introduce_generic-entity-template-2808481-3.patch6.82 KBberdir

Comments

Berdir created an issue. See original summary.

berdir’s picture

berdir’s picture

Status: Active » Needs review
StatusFileSize
new6.82 KB

Status: Needs review » Needs work

The last submitted patch, 3: introduce_generic-entity-template-2808481-3.patch, failed testing.

berdir’s picture

Status: Needs work » Needs review
Issue tags: +Dublin2016
StatusFileSize
new10.45 KB
new4.07 KB

Working on label stuff and using it for entity_test.

Status: Needs review » Needs work

The last submitted patch, 5: introduce_generic-entity-template-2808481-5.patch, failed testing.

berdir’s picture

Status: Needs work » Needs review
StatusFileSize
new11.22 KB
new794 bytes
wim leers’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
@@ -159,6 +157,17 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode) {
+    // Add the default #theme key if a template exists for it.
+    if (\Drupal::service('theme.registry')->getRuntime()->has($this->entityTypeId)) {
+      $build['#theme'] = $this->entityTypeId;
+      $build[“#{$this->entityTypeId}”] = $entity;
+    }
+    else {
+      // Otherwise use the generic entity type.
+      $build['#theme'] = 'entity';
+      $build['#entity'] = $entity;
+    }

Genius.

berdir’s picture

I think the page cache bugs were the same problem as fixed in the previous patch, the page title block actually caused additional fails.

The last submitted patch, 7: introduce_generic-entity-template-2808481-7.patch, failed testing.

berdir’s picture

Fixed some more bugs when non-saved entity are saved.

@Wim: It would be more genius if the comments would make sense. Tried to improve it a bit now.

The last submitted patch, 9: introduce_generic-entity-template-2808481-8.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 11: introduce_generic-entity-template-2808481-11.patch, failed testing.

berdir’s picture

Status: Needs review » Needs work

The last submitted patch, 14: introduce_generic-entity-template-2808481-13.patch, failed testing.

The last submitted patch, 14: introduce_generic-entity-template-2808481-11.patch, failed testing.

berdir’s picture

Status: Needs work » Needs review
StatusFileSize
new22.36 KB
new653 bytes

twig fail

Status: Needs review » Needs work

The last submitted patch, 17: introduce_generic-entity-template-2808481-17.patch, failed testing.

berdir’s picture

Status: Needs work » Needs review
StatusFileSize
new23.84 KB
new1.48 KB
rainbowarray’s picture

I'm not clear from the issue summary, does this actually have an impact on node.html.twig? Having a generic entity template seems like a good idea. Just wanted to get clarity on that, since node.html.twig is one of the most-used existing templates.

tstoeckler’s picture

This looks really, really great and is such a big win for so many reasons. Yay!

Some notes:

  1. +    if (\Drupal::service('theme.registry')->getRuntime()->has($this->entityTypeId)) {
    +      $build["#{$this->entityTypeId}"] = $entity;
    +    }
    +    else {
    +      $build["#{$this->entityTypeId}"] = $entity;
    +    }
    

    I think we should leave the code as is. I get that we are doing the same thing for a different reason but this just makes it less readable in my opinion. We could add a comment explaining the situation above the current line, but I don't feel strongly about that.

  2. +    $this->assertContains($expected_rendered_name_field_1, $markup, "The markup returned by the $formatter formatter is correct for an item with a saved entity.");
    +    $this->assertContains($expected_rendered_body_field_1, $markup, "The markup returned by the $formatter formatter is correct for an item with a saved entity.");
    ...
    +    $this->assertContains($expected_rendered_name_field_2, $markup, "The markup returned by the $formatter formatter is correct for an item with a saved entity.");
    +    $this->assertContains($expected_rendered_body_field_2, $markup, "The markup returned by the $formatter formatter is correct for an item with a saved entity.");
       }
     

    If we're going to have an assertion message there, can we put name and body there, respectively, to differentiate between the two?

  3. +    $variables['url'] = $entity->toUrl('canonical', array('language' => $entity->language()))->toString();
    

    Why is passing the language explicitly necessary? Seems like that should be added automatically, no?

    Very minor: But we could leave out the toString() to enable other preprocess functions to modify the URL and Twig would still print it.

    +  $title_key = $entity->getEntityType()->getKey('label');
    +  if ($title_key && isset($variables['elements'][$title_key])) {
    

    Pretty minor, but it would be cool to use EntityType::hasKey(). I guess that would make it slightly more verbose so feel free to ignore this, as well.

Sorry, only got until the theme suggestions for now with the review.

berdir’s picture

#20: No, none. This is just an example use node as $arbitrary_entity_type that does provide its own template. We could decide in 9.x to drop node.html.twig but it has its own special things so likely not.

#21
1. not sure what you mean. imho it makes sense to explicitly keep the type in both cases as one is a BC later that we want to remove eventually, IMHO.

2. Yeah, will add that. or just remove that, that's just copy & pasted I was lazy. the default message might be enough?

3. because that's how toUrl() works. I agree, it would be nice if it does that but it doesn't. this is copied from node preprocess and necessary. it defaults to the current language, which might not be the same as the node language. You're welcome to open a separate issue to make that the default behavior.

tstoeckler’s picture

Ahh... some of my review. Here I go again...

Re #22.1: Fair enough, not worth arguing about in either case
Re #22.2: Sure, I like not passing a message parameter at all (that's what you meant, right?)
Re #22.3: Ahh wow, I was sure we had fixed that at some point. I thought there was also an issue about this already, but I can't find it right now. We could still drop the ->toString() but that's pretty minor.

Rest of the review:

  1. +  $suggestions[] = 'entity__' . $entity_type_id . '__' .$entity->id();
    +  $suggestions[] = 'entity__' . $entity_type_id . '__' .$entity->id() . '__' . $sanitized_view_mode;
    +

    I realize that you took this over from node, but I don't think we should be providing ID-specific template suggestions. That just leads to all the usual problems of referencing serial IDs in code. Also, we can always add it later, but if we add it now, we cannot remove it later, so...

    Also (if we do keep it), concatenation white-space error before the entity ID.

  2. + * - label: The title of the entity.
    

    I think this should be The label of the entity. for consistency. If we want to clarify this distinction we could also add This could be the title or name of the entity, depending on the entity type. (or something like that).

  3. + * - title_attributes: ...
    + * - title_prefix: ...
    + * - title_suffix: ...
    

    I know that we use this in a lot of other places in core so far, but it does strike me as strange to have all these "title*" things that are surrounding a thing called "label". Maybe we could just use "label*" in code, and then somehow copy one over to the other in preprocess? I'm fine if we leave it that way for consistency or other reasons, I just wanted to bring it up in case there is some way to improve this that is not too involved.

  4. +<article{{ attributes }}>
    

    I don't really know much about HTML5, but is that something we can get away with on a generic level? (Genuine question, I'm not saying it's not the case, I just don't know.)

  5. +  {% if label and view_mode != "full" %}
    

    It would be cool to add a comment here, something like "In the full view mode the entity label is assumed to be displayed as the page title, so we do not display it here.". Especially because there are couple assumptions that go into that, that may not be true on any given sites. (I.e. you could use the "full" view mode for something else, or you could not provide a page title block. Edge cases surely, but a comment doesn't hurt.)

  6. -    $this->assertTrue(strpos(trim((string) $result[0]), $entities[0]->label()) !== FALSE, 'The rendered entity appears in the header of the view.');
    +    $this->assertTrue(strpos(trim($result[0]->asXML()), $entities[0]->label()) !== FALSE, 'The rendered entity appears in the header of the view.');
    

    Do you want to enlighten us, why this was necessary, or do you not want to talk about it? ;-) (Not being sarcastic, I could totally understand...)

  7. -    $this->assertEqual($build['label']['#markup'], $values[$current_langcode]['name'], 'By default the entity is rendered in the current language.');
    +    $this->assertEqual($build['label']['#plain_text'], $values[$current_langcode]['name'], 'By default the entity is rendered in the current language.');
    

    This one I actually would like to understand, I couldn't really figure this out from a cursory look around.

berdir’s picture

Removed the messages

1. Agreed, removed
2. Changed
3. Not sure yet
4. I think article is a safe default: http://html5doctor.com/the-article-element/. We could also make it somehow configurable through the render array/preprocess.
5. Added your suggested comment, not sure yet about the exact wording and formatting of multi-line comments in twig.
6. As discussed, (string) doesn't work for nested elements apparently, so I have to switch to get all the inner markup
7. #plain_text is what actually gets set, the existence of a #theme above changes what kind of properties are set exactly on the contained elements.

Status: Needs review » Needs work

The last submitted patch, 24: introduce_generic-entity-template-2808481-24.patch, failed testing.

berdir’s picture

Issue summary: View changes
berdir’s picture

Status: Needs work » Needs review
StatusFileSize
new24.4 KB
new994 bytes

Fixed that test. Will ping @timplunkett to check if that change doesn't somehow change the meaning of the test. This happens because I made name configurable on the display. It's not really used yet, but I might add some more tests to make sure that we can hide/show the label through that.

tim.plunkett’s picture

I think you can delete that line from EntityDisplayTest. It meant to prove that no fields were being shown yet, but that's a fragile assumption anyway as this proves, and the following assertions about the type and region are more important IMO.

joelpittet’s picture

StatusFileSize
new24.21 KB
new2.62 KB

This is looking cool thanks @Berdir! Just a quick change to what we've done with duplicate markup in Classy with only class changes.

And some small typos.

Status: Needs review » Needs work

The last submitted patch, 29: introduce_generic-2808481-29.patch, failed testing.

berdir’s picture

That doesn't seem to work, looks like that's an endless loop?

joelpittet’s picture

Whoa strange we have that pattern in a number of other classy templates, I'll look into that at the airport tomorrow morning. Maybe needs a @stable namespace but should work without it IIRC

joelpittet’s picture

Status: Needs work » Needs review
StatusFileSize
new392 bytes
new24.22 KB

Yup, all those other ones have the template in their current directory. Needs the namespace.

tstoeckler’s picture

+  {% if label and view_mode != "full" %}
+    {{ title_prefix }}
+    <h2{{ title_attributes }}>
+      {{ label }}
+    </h2>
+    {{ title_suffix }}
+  {% endif %}
+

Missed this in the last review: I think this will break contextual links on the full entity page. Someone should verify that, though. For nodes, for example, the title_prefix and title_suffix are printed always (i.e. they are outside of the if).

joelpittet’s picture

StatusFileSize
new1.46 KB
new24.22 KB

@tstoeckler you are correct, that's why we print them on the outside of the if in node.html.twig , nice find.
@see core/modules/contextual/contextual.module:135

joelpittet’s picture

@Berdir I didn't tackle #28 from @tim.plunkett. Just so the comment doesn't get lost.

berdir’s picture

Status: Needs review » Needs work
fabianx’s picture

As this wants to replace the suggestion issues, we should really take the BC break opportunity to 'type' or 'name' the suggestions.

e.g.

entity--node--view-mode-full.html.twig

instead of

entity--node--full.html.twig

In case we decided we would allow _ in templates, we could also use:

entity--node--view-mode_full--bundle_article.html.twig

which would much more match (pseudo code):

list(node*.html.twig) | filter(type = 'node', view-mode == full, bundle == article)

and as such would make things hopefully simpler to work with suggestions in the long term.

I think it would still be okay for the main type to be non-prefixed, but entity--type_node could also work obviously.

--

And for BC reasons only the new entity template using things could use the new suggestion patterns for now, so we could slowly transition over.

berdir’s picture

Status: Needs work » Needs review
StatusFileSize
new28.33 KB
new24.93 KB
new3.7 KB

Ok, here's an update.

* This now includes #2698909: EntityViewBuilder uses non-existing #theme hooks, it is RTBC and contains a test that we can extend here.
* removed the line from the test
* updated theme suggestions, works for me.
* the last part in the interdiff updates the test from that other issue, instead of no template, we now check for the entity type and also the theme suggestions.

The last submitted patch, 39: introduce_generic-2808481-39.patch, failed testing.

fabianx’s picture

Don't have time for a detailed review, but this looks really good to me :).

markhalliwell’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
@@ -159,6 +169,22 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode) {
+    // Use the entity specific #theme key if a template exists for it.
+      $build['#theme'] = $this->entityTypeId;
...
+      // Otherwise use the generic entity theme hook.
+      $build['#theme'] = 'entity';

+++ b/core/modules/system/system.module
@@ -248,6 +252,60 @@ function system_hook_info() {
+function system_theme_suggestions_entity(array $variables) {

The theme hook suggestions are only applied to entity theme hooks. For entities that do have their own theme hook, they're not always applied (e.g. user).

I think this standardize this more somehow rather than continue letting entities that have custom theme hooks slip through the cracks on things like this.

Maybe this should incorporate some of #2270883: Automatically add theme hook suggestions for all entity types as well? I just think it would probably benefit us all if this topic was treated as a whole rather than in pieces.

markhalliwell’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/system.module
@@ -248,6 +252,60 @@ function system_hook_info() {
+    $suggestions[] = 'entity__' . $entity_type_id . '__bundle_' . $entity->bundle() . '__view_mode_' . $sanitized_view_mode;

Also, explicitly specifying __bundle_ and __view_mode_ as part of the theme hook suggestion is too odd, verbose and not done elsewhere in core AFAIK.

Using node as an example (just for brevity):

entity__node__bundle_page__view_mode_teaser
entity--node--bundle-page--view-mode-teaser.html.twig

vs.

entity__node__page__teaser
entity--node--page--teaser.html.twig

catch’s picture

The problem is when you have a view mode and a bundle named the same, i.e.

entity--node--bundle-gallery--view-mode-gallery.html.twig

Becomes:

entity--node--gallery.html.twig (view mode)
entity--node--gallery.html.twig (bundle)
berdir’s picture

Yes, that is the reason. I don't really care if that's there.

I don't think we should include anything for existing templates here. This is a lot tricker than you might think, because it could change the order for suggestions that contrib/custom added and suddenly different templates might be used.

If we only do this for the new entity template here then there is no risk for existing entity types with own templates to suddenly change somehow.

andypost’s picture

Related issues: +#2229355: Field template suggestions are colliding
markhalliwell’s picture

Re #44:

I'm sorry, but I completely disagree with that line of reasoning.

Naming a bundle and view mode the same thing, while possible, has always been a "thing" in Drupal. Doing so is just a symptom showing the lack of naming/architecture/foresight.

Also, if that kind of distinction is actually necessary, FE devs would actually use both names in the suggestion and just get specific (which is kind of the whole point of adding the last suggestion in the first place):

entity--node--gallery--gallery.html.twig (bundle and view mode)

In reality though, no one would actually really name them both the same. These names should describe what it is they're trying to accomplish:

  • bundle: (gallery) - A collection of things
  • view_mode: (gallery) - What is displaying said collection or the type of display. This should actually be something like slideshow, carousel or popup

Then the suggestion becomes more meaningful:

entity--node--gallery--slideshow.html.twig (bundle and view mode)

---

Re #45:

No, it's not really that tricky.

Adding standardized base entity suggestions won't hurt anything.

I'm not suggesting that this issue be used to replace existing implementations (like node, field, etc.) unless it actually makes sense to do so.

If a module (like field, in this case) decides to override/supply additional suggestions on top of the base suggestions (even if they're technically dups of the base suggestions), then those provided suggestions would still take precedence over the base entity suggestions (that's how theme suggestions work).

As I stated above, this issue should be working towards standardizing all base entity theme hook suggestions, not worrying about special edge cases (which is already handled in the module's unique hook_theme_suggestion_alter() implementation).

rainbowarray’s picture

For what it's worth, I've run into a lot of thorny problems trying to get suggestions to work well once you have a few of them in a template name. Having a prefix for the type of thing would probably go a long way to minimizing that problem. Longer file names is a tradeoff I'd be willing to take to avoid really challenging debugging.

markhalliwell’s picture

Longer file names is a tradeoff I'd be willing to take to avoid really challenging debugging.

Well I'm not. They're already too long. Also, debugging has gotten easier, not harder (twig debug/xdebug.... all anyone really needs). The only reason that many entities don't show up with their possible theme hook suggestions is because they technically don't exist, which is what this issue is/should be about.

---

Honestly, lack of architecture/planning isn't reason enough to justify complicating theme hook suggestions just so we can "comfort n00bs". If you're dealing with theme hook suggestions, you should have a semi-firm grasp on how templates work in Drupal and some very fundamentals in that are before even continuing down this path.

Is one going to run into issues, sure... it's called a learning curve for a reason. We can't coddle every pain point for the sake of "DX", there needs to be a balance and this is one of them. As I said above, that is why we provide both the bundle and view mode in the same theme hook suggestion... for granularity, when it's actually needed.

There's enough documentation to guide people to where they need to go. The least of which is creating your own custom theme hook suggestions based on any sort of criteria you might want/need.

No, this issue is about providing some sane, basic entity theme hook suggestions based on established community patterns (one where "prefixes" aren't currently a part of).

---

Also, node/field would not be able to move to this "entity specific bundle/view mode based" solution until 9.x (officially) as it would constitute a relatively major BC break.

Sure, we could add them simply as "base suggestions" (as I mentioned above) and then add back the legacy cruft on top of it... but I'm really tired of "oh, let's just keep piling crap on top of the theme system... it can handle it" mentality. It's been old. For a long time.

Regardless, introducing a whole new concept of "prefixed theme hook suggestions" in this issue is major scope creep. This entire topic should be moved to another issue where it can be discussed and hashed out by people who actually use these on a day to day basis.

manuel garcia’s picture

We do need to have the entity - bundle - view-mode keywords in the template suggestions, else we are surely going to run into problems like described above, let me try to explain it further here:

entity--node--gallery.html.twig (sets up to theme all bundle gallery)
entity--node--gallery.html.twig (sets up to theme all view-mode gallery)

Drupal would have no way to know which is which.

There's also a plus side to using these keywords, it makes the template files self-documenting, it makes it possible to know what each template actually applies to just by reading its file name, something that otherwise you'd have to explicitly document somewhere or spend time debugging all the time. This makes for easier on-boarding of themers to an ongoing project, or to yourself 6 months down the road.

markhalliwell’s picture

Why would anyone create a bundle and view mode with the same name?

Seriously, no one does this (or at least they really shouldn't).

It's literally a naming anti-pattern (which is why "bundle" and "view mode" are two separate names/terms, they don't do the same thing).

Again, all of this is beside the point.

Any discussion regarding "prefixing theme suggestions" should be opened up in a separate issue. Period.

It has much larger implications and quite frankly, naming things is difficult enough. This issue shouldn't assume/impose this new "mentality" before it's been logically thought out as an entirely separate topic (one that I have not seen or haven't been able to find, if it exists).

berdir’s picture

Having the same name isn't the main point. It is self-documentation as #50 says. You might know that the bundle is first, but non-experienced themers will not, and then it will be much clearer for them what the template is for.

> Seriously, no one does this (or at least they really shouldn't).

There is a bit difference between you shouldn't do it and no one does it. And I don't think it is that uncommon, actually.

And no, we can't open a separate issue. It is something we need to decide *before* committing this, once it is in, it is an API change to change it again.

Note that #2698909: EntityViewBuilder uses non-existing #theme hooks is blocking this, I will work on this again when that issue is in.

markhalliwell’s picture

It is self-documentation as #50 says.

Great, so we're adding documentation to filenames now too? This is unbelievable.

I'm sure our tiny IDE project file trees will love even longer filenames that we can't view. Or that we'll enjoy being able to see only one filename at a time in the tabs above? Please..... leave the "documentation" inside the file itself.

And I don't think it is that uncommon, actually.

Yes, because module/core developers are theming every day. This is pure speculation. I'm telling you (as a themer) that this really isn't all that important, especially now that there is twig debug.

And no, we can't open a separate issue. It is something we need to decide *before* committing this, once it is in, it is an API change to change it again.

Kind of my whole point. This should be flushed out before this issue, not during it... but it seems that it's being discussed here anyway...

---

I'd like to remind everyone that core/BE-devs have a habit of adding crap to the FE that isn't actually needed because they "think" we need it. This has the potential of becoming the next "divitis" disaster.

Please, for the love of all things simple and clean.... please stop telling us what we "think" we need.

fabianx’s picture

I can see that bundle- and view-mode might be a little long as prefixes.

On the other way we need a way to distinguish that and other issues are blocked on adding more theme suggestions on that exact issue. It is also an open bug report right now.

My suggestion therefore is to use one letter prefixes, like:

entity--node--v-full--b-article.

That is just 2 characters more per theme suggestion, solves the problem and is except for some weirdness (b? v?) pretty simple to understand.

So the summary is:

We need a way to avoid conflicts and the question is not if we want to implement that (also from feedback from other themers), but how (and what e.g. we could decide that bundle is so uncommon we could prefix it, but view_mode is so common and would not collate with entity_type to leave it).

I'll also ask around in #twig so we can discuss that somewhat and get some community consensus on it.

Edited:

Consensus could also be to just prefix little and less used things and leave out prefix for more common things.

catch’s picture

I thought about one-letter prefixes as well and think that's a good compromise, +1 for #54.

@markcarver contrib modules often provide entity bundles and sometimes view modes, so it's not just a case of 'lack of architectural foresight', it's also just having two different modules installed sometimes. On top of that bundles can't be renamed once created, and sites develop over time, so lack of architectural foresight is extremely common.

markhalliwell’s picture

one-letter prefixes

I too thought about this, but thought it'd get shot down because it wasn't "clear" what they stood for. Also, the more I think about it, I'm not sure we want to go this route either. This is something that the community has consciously or subconsciously attempted to move away from: using ambiguous "naming" techniques for the sake of brevity. In fact, it's become even less of a thing in 8.x where classes are all spelled out. I think adding this would just be another "drupalism".

---

I get that not everyone will agree with me. I'm not asking anyone to. I'm just saying that in the years of doing theming these kinds of naming conflicts, at this upper level (entities, not really including fields here), has never been an issue. It just doesn't happen.

That being said, I get that people want to "solve" this and we should. It just needs to actually be discussed, not thrown in an issue so we lock ourselves in as @Berdir pointed out in #52.

---

After discussing this in #twig for a bit, @markconroy mentioned using directories:

You know what might be nice (but probably can’t happen until drupal 9), if all node.html.twig files were called just that and the folder structure of your theme decided what was needed.

so we have:

templates
— node 
— — bundle 
— — — article
— — — — node.html.twig (would = what we now call node—article.html.twig)
— — — view mode
— — — — node.html.twig (would = what we now call node—article—full.html.twig)
— — view mode
— — — full
— — — — node.html.twig (would = what we now call node—full.html.twig)

@Fabianx and I went back and forth a bit as well trying to hash this concept out logically. It seems doable, but if we're going to go that route I'd suggest not naming everything node.html.twig. Naming everything node.html.twig has it's downsides; namely: which open node.html.twig is it?

Instead, I propose something like the following (example from https://www.drupal.org/node/2358785):

<!-- FILE NAME SUGGESTIONS:
   * node/view/frontpage--page-1.html.twig
   * node/view/frontpage.html.twig
   * node/bundle/article--teaser.html.twig
   * node/bundle/article.html.twig
   * node/view_mode/teaser.html.twig
   * node--1.html.twig
   x node.html.twig
-->

This would allow smaller filenames, while keeping the distinction of specific bundles, view_modes and any other "type" of suggestion.

Personally, I really like the directory idea. IDEs (like PHPStorm) already give you the path for the currently selected file above the tabs and you have that directory tree in the left (so these names are visible).

It just breaks it up so it's not all in the filename, because those can get ridiculously long: e.g. entity—node—bundle-gallery—view-mode-gallery.html.twig.

We already scroll too much, I'd rather do it vertically than horizontally. It also helps with typos: how many times have any of us hunt and pecked why a certain template isn't being detected only to finally figure out that it was because of an extra (or lack thereof) hyphen?

---

I think my point about making this topic a separate issue/plan is needed. It's clear that we need to figure out this "problem" once and for all in Drupal. There's way too many inconsistencies to simply ignore this topic or just shove it in an issue like this.

catch’s picture

I too thought about this, but thought it'd get shot down because it wasn't "clear" what they stood for. Also, the more I think about it, I'm not sure we want to go this route either. This is something that the community has consciously or subconsciously attempted to move away from: using ambiguous "naming" techniques for the sake of brevity.

This is vs. not having a prefix at all because it would be too long?

The problem with not discussing this here is that once we add a theme suggestion in a stable release, we can't remove it again - we can move it to Stable and deprecate it, but that's the limit. Field templates are afaik the only other place in core with this conflict.

catch’s picture

I've opened #2831144: [policy] Bundle / field name / view mode theme suggestions can conflict for the wider discussion. Postponing this issue on that.

fabianx’s picture

Title: Introduce generic entity template with standard preprocess and theme suggestions » [PP-1] Introduce generic entity template with standard preprocess and theme suggestions
Status: Needs work » Postponed
catch’s picture

Title: [PP-1] Introduce generic entity template with standard preprocess and theme suggestions » Introduce generic entity template with standard preprocess and theme suggestions
rainbowarray’s picture

I know this is being postponed, I just wanted to leave a few pieces of feedback on the various suggestions.

Theme suggestions are one of the thornier problems I have run into in Drupal 8. It's clear others have as well. Everybody is trying to work out what are some possible ways to deal with that, and there are probably multiple valid ways. So, kudos to all who have made suggestions on... suggestions.

I think this is more than just people being new to theming, and doing the wrong thing—like using the same machine name for a content type and a view mode. While that may well happen, I've run into other confusing theme suggestion issues. I've been working with Drupal theming for most of a decade now and still having issues, so I don't think it has much to people being new to this.

It seems to me that the theme suggestion is currently highly dependent on:

1) The order in which each suggestion is added.

2) The order in which a particular suggestion fragment is added to a template suggestion.

I haven't exactly pinned down where this is breaking down, but I know that if you start getting three or four fragments into a template suggestion, it's very easy for things to go amiss.

None of this is easy to debug, because you have to track down where the suggestions are added, along with any alters, and for many, the code is not always easy to figure out at first glance.

I certainly understand the concerns with IDEs. It can be tough working with long filenames. No doubt. Making filenames longer could make that harder.

To me, using nested directories complicates working with templates. As the example above shows, you could have two templates for the same view mode appearing in entirely different directories. That takes away my ability to organize my templates in my theme as I see fit. If I want to organize multiple templates for a single component into one directory, I could not do so. As we move Drupal in the direction of component-based theming, that feels like a step backwards.

The modified version Mark Carver proposes above avoids long filenames. However, it results in lots of different file names for node templates. I know a long file name can be challenging. Personally I'd find it more confusing to have a node template that doesn't have the word node in it, when I am looking at a list of open tabs. There are probably ways to mitigate that by setting up your IDE properly, and looking at the right place in the screen.

These are all different sets of tradeoffs. I personally like explicitly putting in a full suggestion fragment type prefix into the filename, because it makes it very clear what is going on. Does it make the filename pretty long? Yes. But I think it's clear, and it allows for a good amount of flexibility for how you organize template names in your theme. I think it would also go a long way towards making suggestion names less dependent on the order of suggestion fragments. I mean, the order may still be required, but hopefully it would be easier to parse the suggestion name and figure out which fragment type should be looked at for each part of the suggestion.

The abbreviated suggestion fragment prefix would probably be okay. It saves a few characters, that's true. So, shorter filenames. But it's something that requires clear documentation and people finding the right documentation page while trying to learn this. I think a longer filename where it's easier to understand what's going on is a better tradeoff.

Long file names are a pain. I'd hope there's ways to mitigate that with how you set up your IDE—or ways to tweak your IDE when you need to look at a really, really long filename, which won't happen all the time. Sometimes, but not all the times.

And again, to be clear, these are all different sets of tradeoffs. I understand how some would value certain benefits and find certain downsides more painful. For me, long file name with explicit prefixes strikes the best balance.

Anyhow, appreciate all the suggestion suggestions everyone.

berdir’s picture

Using folders for suggestions is IMHO definitely out of scope here and can be discussed elsewhere. And the order itself is fairly simply, the first matching template is used. That can get complicated with multiple hooks and alters all adding suggestions of course. but it you can see which one is used pretty well with the provided debug output. Also not related to this issue :)

#2698909: EntityViewBuilder uses non-existing #theme hooks has been committed now, so from that perspective, this issue is unblocked. I fear that the new issue for the theme suggestions naming will stay open for a long time and blocking any progress here on this issue. We could leave out theme suggestions here and do everything else, but it will then be by far not as useful and out-of-the-box-working-ish as with suggestions.

So.. how do we get to the point of making a descision here? I think we have all the options laid out:

a) No prefixes, like on existing theme suggestions => status quo, with all the advantages (shorter) and disadvantages (not self-explaining, possible conflicts)
b) Full prefixes, bundle-, view-mode-, .. (longer but somewhat self-explaining and no risk of conflicts)
c) shortened prefixes, b-, v-, (shorter, not really self-explaining anymore unless you know the abbreviations, no conflicts)

Personally I'm fine with a or b, I really don't care. Not sure if c) isn't actually harder to understand than a) and whether it is worth doing that just to avoid conflicts.

Will crosspost this part into the other issue.

tstoeckler’s picture

We could leave out theme suggestions here and do everything else, but it will then be by far not as useful and out-of-the-box-working-ish as with suggestions.

Well it is still infinitely more useful than now where there is no template at all by default, so I think that's actually a great idea. I think this is an example of not letting perfect get in the way of better. It's certainly better, there is no doubt about it. And the non-suggestion part actually does contain quite some "interesting" parts for example with the label, which I think we have sufficiently solved now, so it would be a shame to just let this sit there because of the suggestions.

Non unpostponing yet, would like some confirmation that I am not on my own with this opinion.

fabianx’s picture

Title: Introduce generic entity template with standard preprocess and theme suggestions » [PP-1] Introduce generic entity template with standard preprocess and theme suggestions

It would be a shame to not take the opportunity to do the change.

Else we have even more BC to bag around.

markhalliwell’s picture

This issue was postponed on the following parent, but it was never added.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dpopdan’s picture

I also had some trouble with the fact that suggestions are not implemented for all entity types. I made a module that provides the option to extend the entity theme suggestions by a plugin and also created a base plugin that defines the most common theme suggestions like we have on nodes. See https://www.drupal.org/project/entity_theme_suggestions.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

knurg’s picture

I would really like to see this in core. The handling for custom entities currently is a mess...

tstoeckler’s picture

I still stand by #63 and the lack of activity here kind of proves my point. Not unpostponing myself, though, as I don't to step on any toes.

stevekeiretsu’s picture

I am even less inclined to step on toes than tstoeckler, but I am certainly happy to give them a "me too".

Anecdote time: I am currently building a large greenfield d8 project where the decision was made early on to implement almost everything as a custom entity (so we could keep our business logic in D8/OOP/PSR-4-land as much as possible), scaffolding them with drupal console. Until now I have been focused on the back-end, last week a new dev from a D7 background was added to the team and charged with looking at the front end. I pointed them at the customentityname.twig.html console had generated and off they went. Come PR time I find that they had found {{content}} producing absolutely nothing so had given up on that template, and created their own theme hook, feeding it $variables by way of manual glue code fishing entity field values from ['0']['value'].....

Clearly this is not exactly the sort of D8 best practice you'd want to encourage, but at the moment I would say it is fairly understandable, given what core, d.o docs and console do (and do not) offer.

It was only via this stackoverflow thread that I was able to find the necessary "missing ingredient" (the 'foreach elements') , and from Berdir's comment there I found this thread.

From a quick skim it appears providing this basic 'foreach elements' has been held up due to (the broader scope issue of) wishing to extend functionality of entity related theme hook suggestions, and as such tstoeckler's "perfect being the enemy of good" aphorism appears spot on to this outsider... Although I must admit not being knowledgeable enough about core to know if it actually possible to provide A without tackling B.

If it is, I would definitely encourage doing so. As your average drupal dev in the field, I would definitely rather have had my custom entity twig spit out *something* from {{content}}, even if all the fancy theme suggestions weren't working, than to find nothing working at all 'out of the box'.

If it isn't, I do have some other potential 'low hanging fruit' suggestions to improve the developer experience in the meantime:

  • The "official" docs for custom content entities make no mention of how to populate {{content}} as far as I can see - could the "missing ingredient" snippets from stackoverflow be added here?
  • Create an issue on drupal console such if the "drupal geco" user chooses to generate a .twig.html for their entity, the necessary snippets are added to the corresponding .module file? They can always be prefaced with warning commentary, as found elsewhere in console boilerplate, if there is concern such generated snippets will need removing or changing if and when this issue delivers equivalent in core.

(I realise console is a separate project so any actual decision/work on this would need raising there not here, but I thought I would first post here as the central place for people with the birds eye view of overall DX, in case they are adamant that console is the wrong place to tackle this. However, if a hard line is taken that generating such 'workaround' snippets in console is not acceptable, I would be inclined to retort with an equally hard line, that the generation of the twig.html for custom entities should then be removed completely, because it is better not to provide anything than to provide half-a-something which looks like it should work, but actually doesn't unless you look on stackexchange. So I feel like console might need an issue raising either way...)

borisson_’s picture

Title: [PP-1] Introduce generic entity template with standard preprocess and theme suggestions » Introduce generic entity template with standard preprocess and theme suggestions
Status: Postponed » Needs work

The issue this was postponed on (#2831144: [policy] Bundle / field name / view mode theme suggestions can conflict) hasn't seen a lot of activity. I agree with @tstoeckler that we should strip out the theme suggestion hooks from this issue, so we can get this one going again. In this case, I don't think we should be looking at a perfect solution for this one yet.

Setting this back to Needs work, the first thing that should happen here is probably a reroll of the patch.

berdir’s picture

> I agree with @tstoeckler that we should strip out the theme suggestion hooks from this issue, so we can get this one going again. In this case, I don't think we should be looking at a perfect solution for this one yet.

The problem with not adding them initially is a) that adding them later could have unexpected side effects (e.g. due to different order of this and custom suggestions, it might match differently) and b), without suggestions, a single global entity template really isn't that useful :-/.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture

andypost’s picture

Additionally it needs to remove olivero_theme_suggestions_user_alter

https://git.drupalcode.org/project/drupal/-/blob/11.x/core/themes/oliver...

catch’s picture

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.