Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Assigned: Unassigned » xjm
xjm’s picture

Assigned: xjm » Unassigned
Status: Active » Needs review
FileSize
58.52 KB

Just the simple string messages. You can use git diff --color-words with the patch applied locally for easier review.

Lars Toomre’s picture

Status: Needs review » Reviewed & tested by the community

I have reviewed each of the straight t() removals from the assert messages in the #2 patch. All are correct and appropriate. I will leave checking for completeness to the follow-up patch correcting t() to format_string() conversions.

With the bot being green, this is RTBC!

webchick’s picture

Assigned: Unassigned » jhodgdon
jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Active

Committed the patch in #2 to 8.x. Leaving this issue open for follow-ups for t() -> format_string().

Lars Toomre’s picture

Status: Active » Needs review
FileSize
15.72 KB

Here is an untested patch that completes the removal of t() from the node test assert messages or, if appropriate, the conversion of t() to format_string().

With this patch applied, I do not observe any more case of t() being used in node tests using grep. This patch includes the final 27 conversions in this module.

dcam’s picture

Status: Needs review » Needs work

I tested #6 and found two more t()'s around assert messages in NodeRevisionsTest.php on lines 87 and 99.

I could alter the patch myself, but then then we'll have to wait for someone else to test it. @Lars Toomre, if you can make the changes then I'll be sure to come back and re-test. Then we can get this committed.

Lars Toomre’s picture

Status: Needs work » Needs review
FileSize
1.89 KB
16.79 KB

Here is a patch that includes the two items noted in #7.

dcam’s picture

Status: Needs review » Reviewed & tested by the community

I've verified that #8 removes the two t()'s found in #7. I can't find any others that were missed. The patch looks good to me. Marking as RTBC.

webchick’s picture

Assigned: Unassigned » jhodgdon

Tum te tum...

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Patch (to be ported)

Thanks, that one's in! Looks like we need to port #2 + #8.

Lars Toomre’s picture

Thanks @jhodgdon for the review and commit. It is so nice to see the node Tests now complete for D8. Thanks!!

dcam’s picture

Status: Patch (to be ported) » Needs review
FileSize
60.02 KB

Backported #2 and #8 to D7.

Status: Needs review » Needs work

The last submitted patch, 1797200-13-t-node.patch, failed testing.

dcam’s picture

Status: Needs work » Needs review

#13: 1797200-13-t-node.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1797200-13-t-node.patch, failed testing.

dcam’s picture

Status: Needs work » Needs review
FileSize
59.68 KB

I'm not sure why #13 wasn't applying, but I re-backported #2 and #8 to try and get a good patch this time.

Status: Needs review » Needs work

The last submitted patch, 1797200-17-t-node.patch, failed testing.

dcam’s picture

Status: Needs work » Needs review
FileSize
60.03 KB

Let's try this again. I rolled this one on a different system. Maybe I did something to node.test on that other laptop that's causing #13 and #17 to not apply.

Backport of #2 and #8 to D7.

dcam’s picture

#19: 1797200-19-t-node.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1797200-19-t-node.patch, failed testing.

dcam’s picture

Status: Needs work » Needs review
FileSize
60.03 KB

I rerolled the patch in #19.

Status: Needs review » Needs work

The last submitted patch, 1797200-22-t-node.patch, failed testing.

dcam’s picture

Status: Needs work » Needs review
FileSize
60.56 KB
688 bytes

#22 had the same problem described in #19. Sorry about that.

I rerolled #17 again and changed another unneeded t(). The post-reroll change is shown in interdiff.txt.

dcam’s picture

Issue tags: +Novice

Tagging as Novice.

K.MacKenzie’s picture

Status: Needs review » Reviewed & tested by the community
jhodgdon’s picture

Assigned: Unassigned » jhodgdon

Thanks! Assigning to me to commit unless someone else beats me to it.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Fixed

Thanks all! This one is committed to 7.x. Another one of these closed!

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