There is no need to use t() in tests, unless we're testing translations, however in core we do not follow this consistently, which does not set a good example for new contributions.
In #3133726: [meta] Remove usage of t() in tests not testing translation we identified there are several calls to t() from various methods, and removing all calls from submitForm() calls in one go seems to be a suitable way of attacking this problem.
Identify and remove all calls to t() wrapped in calls to submitForm(), except those used by translation-related code (if any).
| Comment | File | Size | Author |
|---|---|---|---|
| #21 | 3187113-21.patch | 10.27 KB | pbbhoge |
| #15 | 3187113-15.patch | 10.27 KB | longwave |
| #8 | no t in tests.png | 113.08 KB | guilhermevp |
Comments
Comment #2
longwaveUnsure if we should actually deprecate this, but it will tell us if I missed anything.
Comment #4
longwaveComment #6
longwaveComment #7
vikashsoni commentedBy this patch #6 we can remove use of t() for submitForm
Comment #8
guilhermevp commentedPatch applies cleanly and works as intended.
RTBC +1
Comment #10
catch@guilhermevp there's no need to upload a screenshot of the patch applying, since the automated test system tells us whether the patch applies or not already. A better way to show that the patch works would be to check that there are no more t() calls inside ::submitForm() - although the new deprecation notice ensures this.
We should backport this to 9.1.x to keep the tests in sync, but without the hunk adding the new deprecation. Looked at committing without just that hunk, but it needs a reroll for other conflicts too.
Comment #11
longwaveNot actually sure this was ready to go, the deprecation notice has a fake node ID as I didn't write a change notice. Do we need one? Should we even be deprecating this?
Comment #13
catchWhoops I glazed over the fake node ID.
I think it's fine to deprecate the behaviour formally, but if we do that we'd actually need to type hint on string in Drupal 10, so we'd at least need a follow-up to add that type hint.
Comment #14
alexpottPHP 8 now has
stringablewhich would be an ideal typehint for this. Not sure we need to break people's tests for this. Could go both ways.Comment #15
longwaveReuploading without the deprecation hunk. The deprecation proved that we have covered all instances, but I don't think we actually need to formally deprecate this - we just want to remove
t()from all core tests where possible to set a good example.Comment #16
catchComment #17
longwaveWhoops this was for 9.2.x, it was reverted from there and needs to go back in.
Comment #19
catchCommitted 439c314 and pushed to 9.2.x. Thanks!
Comment #21
pbbhoge commentedHi @longwave,
Due to issue we are facing Error in PHPunit test case having translation enabled .
Can you please re-open this issue and revert the code as per attached patch ( Reverting code from comment #15 ) ?
Comment #22
timmillwoodWe don't need to revert the whole commit, but we are facing issues when running drupal login on non-english sites. #3222549: Adding t() function in UiHelperTrait::drupalLogin() fixes that.