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
- Replace x and * with emojis
-
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
| Comment | File | Size | Author |
|---|
Issue fork drupal-3420709
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:
- 3420709-make-it-more
changes, plain diff MR !6545
Comments
Comment #4
sumit-k commentedCertainly, 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.
Comment #6
ruturaj chaubeyAdded the
OVERRIDDENhighlight on the custom theme templates that override the core template.Comment #7
larowlanWhat about using emoji instead of x and *
Comment #8
ressaThanks @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,OVERRIDDENis only shown for themes not incore/themesorthemes/contrib.Comment #9
ressaComment #10
ressaMaybe ▪️ (https://emojipedia.org/black-small-square) would work better for the listed file names, since ❌ may be misinterpreted? I have updated the MR.
Comment #11
larowlanI'm not convinced the use of OVERRIDDEN adds anything but don't feel strongly about it
What shows if the base template is used?
Comment #12
ressaHaving 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?
Comment #14
ressaWow @gorkagr, that was fast. Thanks!
Comment #15
gorkagr commentedTest are green now :)
Comment #16
ressaUpdating the Issue Summary, with some points about not adding "OVERRIDDEN" for core and contrib themes, but only for user created themes.
Comment #17
smustgrave commentedRemoving 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
Comment #18
larowlanLeft another comment on the MR about the instances when we want overridden to show.
Comment #19
ressaBasically, 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:
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/customfolder, this feature will not work, but I think that's acceptable, since it's not a crucial feature.Comment #20
gorkagr commentedA 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
Comment #21
ressaThat's a good point @gorkagr, I have updated the MR.
Comment #22
smustgrave commentedBelieve @larowlan question has been answered.
I showed this to one of our junior devs and they said it helped them.
Comment #23
ressaNice to get confirmation from the target audience that this is an improvement, thanks @smustgrave.
Comment #24
larowlanThanks, the change to check for custom feels less controversial to me.
Tagging for frontend framework manager review.
+1 from me.
Comment #25
alexpottSorry 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.
Comment #26
gorkagr commentedChanged 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
Comment #27
gorkagr commentedok, 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?Comment #28
gorkagr commentedComment #29
nod_+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.
Comment #30
gorkagr commentedImplemented @nod_ suggestion and also updated the test as
node__1there 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?Comment #31
ressaThanks everyone for great suggestions and refining the code, I'll update the Issue Summary to match the current patch.
Comment #32
smustgrave commentedConfirmed still seeing the change as mentioned.
Comment #35
nod_Committed f2ba76f and pushed to 11.x. Thanks!