Problem/Motivation
Several templates (such as menu.html.twig
) use a twig macro
, and include a link to Twig docs for more info about the macro
tag.
This Twig docs link goes to Twig 1.x docs:
https://twig.symfony.com/doc/1.x/tags/macro.html
Drupal 10 uses Twig 3.x, so let's update that link!
https://twig.symfony.com/doc/3.x/tags/macro.html
(...or remove it! -- see Proposed resolution)
NOTE: There are a few instances of menu.html.twig
with the 1.x docs link (system module template, and many/all the overrides of menu.html.twig provided by themes that come with Drupal core). (menu.html.twig is just one example)
Steps to reproduce
Again, using menu.html.twig as reference: Look at any instance of menu.html.twig in the Drupal project. For example:
core/modules/system/templates/menu.html.twig
Proposed resolution
Option 1
Update the link wherever it exists! -- NOTE: The 1.x link is not broken, and there's a link to the 2.x and 3.x docs on the 1.x docs (though it doesn't take you directly to the macro
documentation, just the homepage for each major version's docs). In other words, this outdated link situation is not an emergency.
Option 2
Remove the link...? -- we've been on Twig 2.x for a while and no one's created an issue (!! that I found, important caveat !!), maybe that means no one uses this link?
Remaining tasks
Let's be extra clear/methodical with this low-priority novice issue.
- Seek guidance on replacing vs. removing this link (i.e. on Drupal Slack > #contribute channel, unless someone comments right on this issue thread).
- Track down all instances (i.e. on a clean copy of Drupal 10, grep for the 1.x macro documentation URL). Optionally, add the list of instances to this issue summary.
- Change all those instances (replace or remove, based on decision in step 1).
User interface changes
None.
API changes
None.
Data model changes
None.
Release notes snippet
N/A
Comment | File | Size | Author |
---|---|---|---|
#10 | image (8).png | 160.2 KB | Libbna |
Issue fork drupal-3298717
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
Comment #2
alisonComment #3
alisonComment #4
alisonI posted on Drupal Slack > #contribute just now, to try to get input on replacing vs. removing the link (and just to share this novice issue).
Comment #5
longwaveFor what it's worth I think the link is useful to keep. I have personally clicked it when editing menu templates as I didn't understand what Twig macros were, and I don't think they are that commonly used.
I didn't raise an issue because I didn't even notice the version number in the URL, and as far as I know macros work the same way in all versions of Twig.
Therefore I think we should go for option 1 and update the link to the relevant version.
Comment #6
Libbna CreditAttribution: Libbna as a volunteer and at QED42 for Drupal India Association commentedI also think we should go for option 1. The link can be helpful to know about macros while working with them as @longwave said.
So I will update the link and raise an MR.
Comment #8
Libbna CreditAttribution: Libbna as a volunteer and at QED42 for Drupal India Association commentedI have updated the link only in
menu.html.twig
files. Do I have to update in all the files or just these files only?Comment #9
alisonLooks good in my initial review, thank you @Libbna! (and thank you @longwave for chiming in!)
@Libbna: I'm actually not sure, did you find any other instances of the outdated docs link for twig macro when you grepped Drupal core on a clean copy of Drupal 10? -- or, if you didn't grep yet, I recommend doing that. Optionally (but helpfully), you could add a list of instances to the issue summary (or a list of all the themes you updated, instead of a list of files).
Comment #10
Libbna CreditAttribution: Libbna as a volunteer and at QED42 for Drupal India Association commentedHi @alisonjo315. Yes I have grepped the drupal 10 code and these are the files in which 1.x twig macro link is mentioned. And I have already changed the link to 3.x in
menu.html.twig
files in my last MR.I will soon update the other files also.
Comment #11
Libbna CreditAttribution: Libbna as a volunteer and at QED42 for Drupal India Association commentedI have updated the files with 3.x macro twig document link. Ready for review.
Comment #12
alison@Libbna Looks fantastic to me -- good finds in those other templates 🙌
I queued a "retest" for the failed test, and I'm updating the issue summary to be less specific about affected templates.
As long as tests pass, I think we'll be good to go!
Comment #13
Libbna CreditAttribution: Libbna as a volunteer and at QED42 for Drupal India Association commentedThanks @alisonjo315. But what is the reason for failed tests? I am not able to understand.
Comment #14
alisonIt was confusing to me, too! I got some help on Drupal Slack #contribute, and had a try at updating the tests to fix the issue -- we'll see what happens 🤞🤞
(basically, there are md5 hashes in ConfirmClassyCopiesTest that don't match anymore -- not sure why they're there, BUT, hopefully the changes I made will get those tests to pass! 🤞🤞)
Comment #15
alisonWoo hoo! Tests pass! I can't be the reviewer anymore, since I'm the author of the latest thing, but I'll post on #contribute again, and maybe we can get this committed soonish -- especially before any hashes change again!...
Comment #16
longwaveThank you - this also looks good to me so marking RTBC.
The hashes are there to help decouple from the classy theme, which will eventually be removed from core, but ensure that until this is completed any changes are propagated to other themes correctly.
Comment #18
catchCommitted/pushed to 10.1.x and cherry-picked to 10.0.x, thanks!
This doesn't apply to 9.5.x, I'd be happy to backport it but also fine to just fix in 10.0.x, so leaving there but re-open if you really want to backport.