Make it more obvious that a Twig template is overridden

Certainly, it does make sense to use the "overridden" keyword instead of a symbol like a sign. This change can enhance clarity, especially for new Drupal users who may not be familiar with the significance of certain symbols.

Raising a merge request (MR) and providing a screenshot for reference is a proactive approach to communicate the proposed change effectively. It helps stakeholders visualize the suggested modification and provides context for the decision to use the "overridden" keyword.

Ruturaj Chaubey → made their first commit to this issue’s fork.

Added the OVERRIDDEN highlight on the custom theme templates that override the core template.

What about using emoji instead of x and *

Thanks @sumit-k and @Ruturaj Chaubey, great initial MR and improvements! I agree @larowlan, emojis could help faster separate name options, and active templates, so I added that. Also, I updated the MR to exclude contrib themes, since that would defeat the purpose, by adding a truckload of OVERRIDDEN's everywhere, for example if you are using Bootstrap5 as base theme. Now, OVERRIDDEN is only shown for themes not in core/themes or themes/contrib.

Maybe ▪️ (https://emojipedia.org/black-small-square) would work better for the listed file names, since ❌ may be misinterpreted? I have updated the MR.

I'm not convinced the use of OVERRIDDEN adds anything but don't feel strongly about it

What shows if the base template is used?

Having OVERRIDDEN is a great tool, allowing to search for the word in the source, and immediately jump to the relevant position.

Shouldn't we assume that core and contrib themes are in folders called something with "core" or "contrib", and likewise normally, and you put custom themes in for example themes/custom, or similarly named folders without "core" or "contrib"?

The logic in the code is to not include themes in folders with "core" or "contrib" in the path.

str_contains($template_file, '--') won't work, since a base such as Bootstrap5 will introduce ~50 OVERRIDDEN instances, like I already stated in #8.

Or am I missing something?

Wow @gorkagr, that was fast. Thanks!

Test are green now :)

Updating the Issue Summary, with some points about not adding "OVERRIDDEN" for core and contrib themes, but only for user created themes.

Removing tests tag as it was addressed and can be seen here https://git.drupalcode.org/issue/drupal-3420709/-/jobs/800003

I think this is a nice little improvement personally.

I applied the MR locally and verified I'm seeing the green checkmark on templates

LGTM

Left another comment on the MR about the instances when we want overridden to show.

Basically, the goal is to help new users (and everyone else) easier get confirmation, and find an overridden instance in the source in a custom theme that they made.

There will be other overrides by other themes or modules, but they are not relevant for this use case, and should not trigger the key word. We want to single out the one instance (or few instances) in the source, which is relevant to the user.

Instead of checking if the theme is not in "core" or "contrib", we could simply check if the theme is in a folder with the string "custom" in its path. This is after all the recommended method:

It is good practice to place the contributed themes in a sub folder named "contrib" and your own themes in a folder named "custom".

From Drupal theme folder structure →

The words placed in the source could then be "CUSTOM THEME", since that would be precise. And yes, if a user does not put the theme in the themes/custom folder, this feature will not work, but I think that's acceptable.

A template could also be placed in a module under the /custom folder, so the last commit I'd rather say "CUSTOM TEMPLATE" rather than custom theme as it might point to the wrong location

That's a good point @gorkagr, I have updated the MR.

Believe @larowlan question has been answered.

I showed this to one of our junior devs and they said it helped them.

Nice to get confirmation from the target audience that this is an improvement, thanks @smustgrave.

Thanks, the change to check for custom feels less controversial to me.

Tagging for frontend framework manager review.

+1 from me.

Sorry I'm hung up on the custom thing like @larowlan was. I've added a comment on the MR with a suggestion to change this from detecting custom - because that has way too many ways of being wrong in both directions to checking whether the template comes from the active theme.

Changed to see if the template file that is being checked is from the active directory, so in that case it should be an overridden template, shouldnt it?

Best

ok, with my last commit, the test fails.
But if understand correctly the test flow, the active theme ($variables['directory']) is 'core/modules/system/tests/themes/test_theme/', correct?
and because of that, the template of check is provided by the theme too (not the node module itself), so the assert ($this->assertStringContainsString("BEGIN OUTPUT from '$template_filename'", $output, 'Full path to current template file found.');) should be changed too?

+1 on the idea.

I'm not super sold on the green tick (at least that show up as a green check on my system) since the comments are already green by default on FF/chrome and the black square stands out better. But it's just cosmetic stuff and it is better than it was so i don't mind enough to change it.

For "💡 BEGIN CUSTOM TEMPLATE OUTPUT" it should also update the end output comment no: "END CUSTOM TEMPLATE OUTPUT", I don't think we need the emoji for this one.

Implemented @nod_ suggestion and also updated the test as node__1 there uses a template on the theme and not in core/modules/node/templates, so that's why the test was failing. All should be good, unless we need a new test to cover the case where no template is overridden (maybe with the node_edit_form template?)

Thanks everyone for great suggestions and refining the code, I'll update the Issue Summary to match the current patch.

Confirmed still seeing the change as mentioned.

Committed f2ba76f and pushed to 11.x. Thanks!

nod_ → closed merge request !6545

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