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.

  1. Seek guidance on replacing vs. removing this link (i.e. on Drupal Slack > #contribute channel, unless someone comments right on this issue thread).
  2. 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.
  3. 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

CommentFileSizeAuthor
#10 image (8).png160.2 KBLibbna

Issue fork drupal-3298717

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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alisonjo315 created an issue. See original summary.

alison’s picture

Issue summary: View changes
alison’s picture

Issue summary: View changes
alison’s picture

I 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).

longwave’s picture

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

Libbna’s picture

Assigned: Unassigned » Libbna

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

Libbna’s picture

Status: Active » Needs review

I have updated the link only in menu.html.twig files. Do I have to update in all the files or just these files only?

alison’s picture

Looks 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).

Libbna’s picture

FileSize
160.2 KB

Hi @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.

Libbna’s picture

Assigned: Libbna » Unassigned

I have updated the files with 3.x macro twig document link. Ready for review.

alison’s picture

Assigned: Unassigned » alison
Issue summary: View changes

@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!

Libbna’s picture

Thanks @alisonjo315. But what is the reason for failed tests? I am not able to understand.

alison’s picture

It 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! 🤞🤞)

alison’s picture

Woo 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!...

longwave’s picture

Status: Needs review » Reviewed & tested by the community

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

  • catch committed b876ae0 on 10.0.x
    Issue #3298717 by Libbna, alison, longwave: Twig macro documentation...
  • catch committed f5eda64 on 10.1.x
    Issue #3298717 by Libbna, alison, longwave: Twig macro documentation...
catch’s picture

Version: 10.1.x-dev » 10.0.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/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.

Status: Fixed » Closed (fixed)

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