Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
A test has been commented out in the parent. It needs a real fix.
Comment | File | Size | Author |
---|---|---|---|
#14 | 2280955-twig-node-trans-14.patch | 3.42 KB | joelpittet |
#12 | 2280955-vs-head-updated-translation.patch | 3.5 KB | bdragon |
Comments
Comment #1
bdragon CreditAttribution: bdragon commentedI'm chasing something in the code here. I have a hunch.
Comment #2
markhalliwellhttps://github.com/fabpot/Twig/issues/1419
Comment #3
bdragon CreditAttribution: bdragon commentedSo... I did a thing. Thoughts?
Comment #4
bdragon CreditAttribution: bdragon commentedWoo!
Comment #5
bdragon CreditAttribution: bdragon commentedNow with less sucky comments. Also, let's run the test.
Comment #7
bdragon CreditAttribution: bdragon commentedOh, right, the test isn't disabled in HEAD. :P
Comment #9
bdragon CreditAttribution: bdragon commentedWhoops, a ! snuck in.
Comment #11
bdragon CreditAttribution: bdragon commentedI suspect the fails are actually just because I forgot to update the translations to account for the corrected variable names. (the trans code was previously using ambiguous naming that could cause conflicts to happen.)
Comment #12
bdragon CreditAttribution: bdragon commentedAs I surmised, it was indeed due to the translations having the wrong names baked into them.
Comment #13
bdragon CreditAttribution: bdragon commentedThis is now ready to go pending review, and can be committed independently or rolled into #1825952. (in which case uncommenting the test again there is in order.)
Comment #14
joelpittetThanks @bdragon this looks awesome to me, just rewriting a comment for the 80char limit, hope the rework, works for you?
In the mean time I'm assigning to @Mark Carver to have a second pass to see if this will work for him as well.
Comment #15
joelpittetAlso, applied and tested against autoescape patch locally with the failing test uncommented and all the tests pass so that's the awesome of the awesome!
Comment #16
chx CreditAttribution: chx commentedWhen you guys are ready, if the patch is so small, merge this patch into parent and close this issue. I haven't expected a solution to emerge this quickly. Thanks guys.
Comment #17
markhalliwellI'll take a look when I get to the sprint room later this afternoon.
Comment #18
markhalliwellThis looks ok to me. The change record will have to be updated to reflect this change: https://drupal.org/node/2047135
Comment #19
markhalliwellAlso, we'd also have to double check any existing twig templates to ensure they're not using the old format.
Comment #20
joelpittetThanks @Mark Carver, Just checked, there are no complex variables in any existing twig templates in core using the trans tag outside of the tests.
Should we just update that change record or create a new one? Maybe a question for @xjm?
Comment #21
joelpittetAfter commit need record change @ https://drupal.org/node/2047135
As per @alexpott's suggestion on irc.
Comment #22
alexpottCommitted e0d17ee and pushed to 8.x. Thanks!
Please update the original change notice.
Comment #23
joelpittetUpdated change record to remove the note about complex tokens.
https://drupal.org/node/2047135