Make it more obvious that a Twig template is overridden

Created on 11 February 2024, 11 days ago
Updated 19 February 2024, 3 days ago

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' -->
    

    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' -->
    

Merge request link

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Feature request
Status

RTBC

Version

11.0 🔥

Component
Theme 

Last updated about 2 hours ago

Created by

🇩🇰Denmark ressa Copenhagen

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @ressa
  • First commit to issue fork.
  • Merge request !6545Added overridden Keyword → (Open) created by sumit-k
  • Status changed to Needs review 11 days ago
  • 🇮🇳India sumit-k

    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.

  • Pipeline finished with Failed
    11 days ago
    Total: 568s
    #92679
  • 🇮🇳India Ruturaj Chaubey Pune, India

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

  • Pipeline finished with Success
    11 days ago
    Total: 521s
    #92720
  • 🇮🇳India Ruturaj Chaubey Pune, India

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

  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    What about using emoji instead of x and *

  • Pipeline finished with Failed
    11 days ago
    Total: 495s
    #92983
  • 🇩🇰Denmark ressa Copenhagen

    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.

  • 🇩🇰Denmark ressa Copenhagen
  • 🇩🇰Denmark ressa Copenhagen

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

  • Pipeline finished with Failed
    10 days ago
    Total: 473s
    #93260
  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    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?

  • 🇩🇰Denmark ressa Copenhagen

    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?

  • First commit to issue fork.
  • Pipeline finished with Canceled
    10 days ago
    Total: 264s
    #93406
  • Pipeline finished with Failed
    10 days ago
    Total: 479s
    #93412
  • Pipeline finished with Success
    10 days ago
    Total: 480s
    #93416
  • 🇩🇰Denmark ressa Copenhagen

    Wow @gorkagr, that was fast. Thanks!

  • 🇧🇪Belgium gorkagr

    Test are green now :)

  • 🇩🇰Denmark ressa Copenhagen

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

  • Status changed to RTBC 9 days ago
  • 🇺🇸United States smustgrave

    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

  • Status changed to Needs review 8 days ago
  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

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

  • 🇩🇰Denmark ressa Copenhagen

    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.

  • Pipeline finished with Success
    8 days ago
    Total: 630s
    #95308
  • 🇧🇪Belgium gorkagr

    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

  • 🇩🇰Denmark ressa Copenhagen

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

  • Pipeline finished with Success
    8 days ago
    Total: 481s
    #95624
  • Status changed to RTBC 8 days ago
  • 🇺🇸United States smustgrave

    Believe @larowlan question has been answered.

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

  • 🇩🇰Denmark ressa Copenhagen

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

  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

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

    Tagging for frontend framework manager review.

    +1 from me.

  • Status changed to Needs review 4 days ago
  • 🇬🇧United Kingdom alexpott 🇪🇺🌍

    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.

  • 🇧🇪Belgium gorkagr

    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

  • Pipeline finished with Failed
    4 days ago
    Total: 314s
    #98450
  • Pipeline finished with Failed
    4 days ago
    Total: 584s
    #98455
  • 🇧🇪Belgium gorkagr

    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?

  • Status changed to Needs work 4 days ago
  • 🇫🇷France nod_ Lille

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

  • Status changed to Needs review 4 days ago
  • 🇧🇪Belgium gorkagr

    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?)

  • Pipeline finished with Success
    4 days ago
    Total: 546s
    #98566
  • 🇩🇰Denmark ressa Copenhagen

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

  • Status changed to RTBC 3 days ago
  • 🇺🇸United States smustgrave

    Confirmed still seeing the change as mentioned.

Production build https://api.contrib.social 0.61.6-2-g546bc20