Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bdragon’s picture

I'm chasing something in the code here. I have a hunch.

markhalliwell’s picture

bdragon’s picture

FileSize
1.85 KB

So... I did a thing. Thoughts?

bdragon’s picture

FileSize
1.97 KB

Woo!

bdragon’s picture

Status: Active » Needs review
FileSize
2.11 KB

Now with less sucky comments. Also, let's run the test.

Status: Needs review » Needs work

The last submitted patch, 5: 2280955-actual-fixed-comments.patch, failed testing.

bdragon’s picture

Status: Needs work » Needs review
FileSize
1.42 KB

Oh, right, the test isn't disabled in HEAD. :P

Status: Needs review » Needs work

The last submitted patch, 7: 2280955-vs-head.patch, failed testing.

bdragon’s picture

Status: Needs work » Needs review
FileSize
1.42 KB

Whoops, a ! snuck in.

Status: Needs review » Needs work

The last submitted patch, 9: 2280955-vs-head-without-logic-bug.patch, failed testing.

bdragon’s picture

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

bdragon’s picture

Assigned: Unassigned » bdragon
Status: Needs work » Needs review
FileSize
3.5 KB

As I surmised, it was indeed due to the translations having the wrong names baked into them.

bdragon’s picture

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

joelpittet’s picture

Assigned: bdragon » markhalliwell
Issue tags: +Twig, +sprint
FileSize
3.42 KB

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

joelpittet’s picture

Also, 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!

chx’s picture

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

markhalliwell’s picture

I'll take a look when I get to the sprint room later this afternoon.

markhalliwell’s picture

Status: Needs review » Reviewed & tested by the community

This looks ok to me. The change record will have to be updated to reflect this change: https://drupal.org/node/2047135

Note: When using complex tokens (ie: arrays/objects) the placeholder name used for translation msgids is the key name in the token (after the . dot). In the above example, the msgid would read: Submitted by @name on @date, not Submitted by @author.name on @node.date.

markhalliwell’s picture

Also, we'd also have to double check any existing twig templates to ensure they're not using the old format.

joelpittet’s picture

Assigned: markhalliwell » Unassigned

Thanks @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?

joelpittet’s picture

After commit need record change @ https://drupal.org/node/2047135
As per @alexpott's suggestion on irc.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs change record updates

Committed e0d17ee and pushed to 8.x. Thanks!

Please update the original change notice.

joelpittet’s picture

Updated change record to remove the note about complex tokens.
https://drupal.org/node/2047135

Status: Fixed » Closed (fixed)

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