Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
This is a sub-task of #500866: [META] remove t() from assert message focused on the A-C include files from the system module sub-systems.
In D8, per http://drupal.org/simpletest-tutorial-drupal7#t, best practice is to remove t() from assert messages in tests. When necessary, t() should be replaced with format_string().
This issue is to correct the Ajax, Batch, Bootstrap, Bundle, Cache, and Common sub-system tests for the above. There are approximately 400 changes needed.
Comment | File | Size | Author |
---|---|---|---|
#38 | 1797120-38-system-asserts.patch | 134.47 KB | dcam |
#35 | 1797120-35-system-asserts.patch | 134.48 KB | dcam |
#25 | system-asserts-1797120-25.patch | 133.76 KB | dcam |
#23 | 1797120-23-t-assert-A-C.patch | 9.45 KB | Lars Toomre |
#3 | t-assert-1797120-3.patch | 168.25 KB | Lars Toomre |
Comments
Comment #1
Lars Toomre CreditAttribution: Lars Toomre commentedHere is an initial untested patch for this issue.
Comment #3
Lars Toomre CreditAttribution: Lars Toomre commentedDarn stray character from editing manually... This patch is also untested locally.
Approximately 40 test files were touched by this patch. Let's see what the bot thinks.
Comment #4
xjmGreen, nice. I'll take a shot at reviewing it.
Comment #5
Lars Toomre CreditAttribution: Lars Toomre commentedOh other thing to point out @xjm, I corrected some text strings to use single quotes rather than double quotes where there was no single quote in the message text. I think that is permissible as I go through reading each of the tests one by one. *smiles*
Comment #6
xjmSure, I think that's fine so long as we don't require it reviewing other issues.
Also note #1681890: Use format_string() instead of t() for assertion message text. In this case I think it's fine since you've already rolled the patch, but for additional ones, we can split it into two smaller patches.
Comment #7
Lars Toomre CreditAttribution: Lars Toomre commentedI have been handling them together as I have had to review each assertion message. If there are only a few, I would prefer to catch them rather having to go back through all of the tests looking for just that issue. Fair enough?
Comment #8
xjmYep, that works for me. I added an asterisk to the issues in the meta that have
format_string()
covered already.And, I reviewed this whole patch. I think I sort of went into a trance, but I didn't find a single error. Nice work!
Comment #9
xjmOh, assigning to @jhodgdon since it's a cleanup patch.
Comment #10
Lars Toomre CreditAttribution: Lars Toomre commentedThank you... That was done as I watched Monday night's NFL football game. Great way to pass time during commercials; reviewing assert messages!! LOL
Comment #11
Lars Toomre CreditAttribution: Lars Toomre commentedWhoops... cross-post! Not sure how to correct the assignment.
Comment #12
xjmFixed. :)
Comment #13
jhodgdonAs I was paging through the diff, I found one more t() that needs to be fixed in
core/modules/system/lib/Drupal/system/Tests/Bootstrap/PageCacheTest.php
and there may be more...
Also, this set of changes caught the $group parameter (why that is even there I don't know???) but not the actual assertion messages:
But I didn't see any problems with the cleanups in the patch, so I committed it and am now setting back to "needs work" for follow-ups.
Comment #14
jhodgdonforgot to unassign
Comment #15
xjmWhoops, yeah, the hunk in #13 should be reverted. I caught that in one of the other patches, but not this one. The group strings technically should still have
t()
.Comment #16
jhodgdonReally? I don't think the group strings should have t() in them. The objective of not having t() in test assertions is that tests are run in English by developers. The groups are just as stupid to translate as the messages, right?
Comment #17
Lars Toomre CreditAttribution: Lars Toomre commentedI orginally thought that group parameters should be taken out, but now am confused. I guess I really do not understand how the group parameters are used, especially since they are used in a small portion of all tests.
I just posted an update to #1797410-5: Remove t() from asserts for G-L includes system tests that put them back in.
Comment #18
jhodgdonYeah, I have no idea why all the test assertion functions have a group parameter, and I have no idea why we are using them or what for, and I definitely don't think they should be translated if we are using them... I'll cross-reference this on the other issue but we should decide before we go around re-translating them...
Comment #19
jhodgdonOK, I talked briefly about this with xjm in IRC just now. We have agreed (hopefully others will too):
- The group names are not translated (or shouldn't be) in getInfo() so they shouldn't be translated when passing a string into an assertion for $group either.
- However, removing t() from the $group is out of scope for this meta-issue.
- Also, we probably don't need to pass $group into any test assertions in Core.
- However, removing $group parameters to test assertions entirely is also out of scope for this meta-issue.
- That said, if t() was removed by mistake from some $group parameters in some of the patches, let's not follow-up by putting it back in.
So... in summary: let's leave the commit above as-is, but fix the assertions mentioned in #13.
Comment #20
Lars Toomre CreditAttribution: Lars Toomre commentedMay I recommend as we encounter t() in getInfo() or t() around group that we make those changs? Quite frankly, I shudder at the thought of manually examing so many test files again. From looking at so many tests, there are not many of them (<20% I would guess as a max). Fair enough?
Comment #21
jhodgdonI will be happy to commit patches that remove t() from $group parameters.
Comment #22
jhodgdonreally unassigning this time.
Comment #23
Lars Toomre CreditAttribution: Lars Toomre commentedAttached is a follow up patch that catches the last couple of t() asserts as well as what @jhodgdon noted in #13.
Note: I checked the all of the tests in A-C Tests with grep now that the bulk of the changes were applied. I am pretty sure that this now catches all of the t() around message asserts. Also did not find any t() in any getInfo() parameters in this group.
Comment #24
jhodgdonI committed this small patch too, and I guess it's ready for backport now.
Backport note: When this issue is backported to D7, it should include things testing actions.inc that were included in a couple of other 8.x issues (things that were moved to modules in d8):
#1798954: Remove t() from test asserts in Action module [part of system tests in d7]
#1798960: Remove t() from test asserts in Ban module [system module include in D7]
Also, both the patch in #23 and the earlier patch in #3 need to be included.
Comment #25
dcam CreditAttribution: dcam commentedBackported #3 and #23 to D7.
Comment #27
dcam CreditAttribution: dcam commented#25: system-asserts-1797120-25.patch queued for re-testing.
Comment #29
dcam CreditAttribution: dcam commented#25: system-asserts-1797120-25.patch queued for re-testing.
Comment #30
dcam CreditAttribution: dcam commented#25: system-asserts-1797120-25.patch queued for re-testing.
Comment #31
dcam CreditAttribution: dcam commentedTagging as Novice.
Comment #32
jhodgdon#25: system-asserts-1797120-25.patch queued for re-testing.
Comment #34
star-szrTagging for reroll.
Comment #35
dcam CreditAttribution: dcam commentedRerolled #25.
Comment #36
jhodgdonbump! We only have a couple of these left. Can someone review this patch?
Comment #37
lazysoundsystem CreditAttribution: lazysoundsystem commentedSorry, didn't see this was waiting - it's very nearly there, just three remaining:
The t() has been removed from the group, but not the message.
Comment #38
dcam CreditAttribution: dcam commentedFixed the asserts shown in #37.
Comment #39
dcam CreditAttribution: dcam commentedStatus...
Comment #40
lazysoundsystem CreditAttribution: lazysoundsystem commentedThanks, those three from #37 are now fixed. RTBC.
Comment #41
jhodgdonThanks all! Committed to 7.x.