Problem/Motivation

To help new Drupal users get started with theming, it would be nice if it was advertised more prominently, when a template is actively overridden by the user in a sub-theme, since it can easily be missed.

It could also be used to do a search for a key word, for example "CUSTOM TEMPLATE" to quickly find the relevant position in the source.

Note: This indication should not be enabled for overrides in core or contrib themes, since with for example Bootstrap5, this would result in 50 "CUSTOM TEMPLATE" instances, defeating the purpose.

Steps to reproduce

Add a Twig template override and not immediately see that it was successful.

Proposed resolution

  1. Replace x and * with emojis
  2. Make it clearer that a Twig override attempt was successful, by inserting a single instance of the word "BEGIN CUSTOM TEMPLATE", where it happened.

    This is the current format:

    <!-- THEME DEBUG -->
    <!-- THEME HOOK: 'block' -->
    <!-- FILE NAME SUGGESTIONS:
       x block--b5subtheme-content.html.twig
       * block--system-main-block.html.twig
       * block--system.html.twig
       * block.html.twig
    -->
    <!-- BEGIN OUTPUT from 'themes/custom/b5subtheme/block--b5subtheme-content.html.twig' -->
    

    Original Twig engine

    Maybe add some visual indications that this is using a custom template, as well as a key word like CUSTOM TEMPLATE?

    <!-- THEME DEBUG -->
    <!-- THEME HOOK: 'block' -->
    <!-- FILE NAME SUGGESTIONS:
       ✅ block--b5subtheme-content.html.twig
       ▪️ block--system-main-block.html.twig
       ▪️ block--system.html.twig
       ▪️ block.html.twig
    -->
    <!-- 💡 BEGIN CUSTOM TEMPLATE OUTPUT from 'themes/custom/b5subtheme/block--b5subtheme-content.html.twig' -->
    

    Emojis and CUSTOM TEMPLATE added

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3420709

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

ressa created an issue. See original summary.

sumit-k made their first commit to this issue’s fork.

sumit-k’s picture

Status: Active » Needs review
StatusFileSize
new39.66 KB

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.

ruturaj chaubey’s picture

Issue tags: +Needs tests
StatusFileSize
new60.27 KB

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

larowlan’s picture

What about using emoji instead of x and *

ressa’s picture

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.

ressa’s picture

Issue summary: View changes
ressa’s picture

Issue summary: View changes
StatusFileSize
new78.71 KB

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

larowlan’s picture

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?

ressa’s picture

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, 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 theme such as Bootstrap5 will introduce ~50 OVERRIDDEN instances, like I already stated in #8.

Or am I missing something?

gorkagr made their first commit to this issue’s fork.

ressa’s picture

Wow @gorkagr, that was fast. Thanks!

gorkagr’s picture

Test are green now :)

ressa’s picture

Issue summary: View changes

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

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests +Needs Review Queue Initiative

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

larowlan’s picture

Status: Reviewed & tested by the community » Needs review

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

ressa’s picture

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, since it's not a crucial feature.

gorkagr’s picture

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

ressa’s picture

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

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Believe @larowlan question has been answered.

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

ressa’s picture

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

larowlan’s picture

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

Tagging for frontend framework manager review.

+1 from me.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

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.

gorkagr’s picture

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

gorkagr’s picture

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?

gorkagr’s picture

Status: Needs review » Needs work
nod_’s picture

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

gorkagr’s picture

Status: Needs work » Needs review

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?

ressa’s picture

Issue summary: View changes
StatusFileSize
new70.42 KB

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

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Confirmed still seeing the change as mentioned.

  • nod_ committed f2ba76f1 on 11.x
    Issue #3420709 by gorkagr, ressa, Ruturaj Chaubey, larowlan, alexpott:...

  • nod_ committed e5cd97f1 on 10.3.x
    Issue #3420709 by gorkagr, ressa, Ruturaj Chaubey, larowlan, alexpott:...
nod_’s picture

Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed f2ba76f and pushed to 11.x. Thanks!

Status: Fixed » Closed (fixed)

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