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.
Problem/Motivation
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 hundreds of calls to t()
in calls to assertText()
and that removing all these in one go seems to be a suitable way of attacking this problem.
Proposed resolution
Identify and remove all calls to t()
wrapped in calls to assertText()
, except those used by translation-related code (if any).
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#80 | 3145418-80-9.2.x.patch | 330.36 KB | longwave |
#80 | 3145418-80-9.1.x.patch | 330.33 KB | longwave |
Comments
Comment #2
longwaveFirst pass with
As with #3145005: [November 9, 2020] Remove uses of t() in drupalPostForm() calls there are some manual fixes required, but let's see how this runs first.
Comment #3
longwaveIn fact let's expand scope slightly as these can be treated the same:
Comment #4
Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae as a volunteer and commentedThe patch couldn't be applied lately.
Comment #5
mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commentedComment #6
mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commentedRerolled patch #3, please review.
Comment #7
paulocsPatch needs re-roll.
Comment #8
longwaveComment #9
paulocsPatch re-rolled.
Comment #10
longwaveThanks for rerolling, there are still lots of cases I didn't catch automatically with #3, these will likely need manual conversion as they need variables interpolating etc.
Comment #11
paulocsHello!
New patch with changes that had to do manually.
Edit: I don't get why patch has composer error.
Thanks
Comment #12
paulocsFixing remained assertNoText(t()).
Comment #13
paulocsNew patch because it was missing some.
Now are all t() calls fixed.
Thanks.
Comment #14
longwaveWe don't need to introduce a variable here. This can just become
There are lots more cases like this where new variables have been introduced.
Comment #15
longwaveI am also worried this patch is too big to review, but I don't know how to break it down.
Comment #16
mondrakeComment #17
paulocsYes, the patch is big. I took a big time to do it.
Maybe any of the maintainers could say what the best approach so we can keep working on it.
About the variables is okay, we can handle it.
Before to work on the variables, I think we should know if we will split it or not.
Cheers, Paulo.
Comment #18
thallesWe can divide in little patches for components/modules.
Comment #19
longwaveNo, we explicitly aren't allowed to group by module: https://www.drupal.org/core/scope#files
Comment #20
longwaveAs this is all tests, and if we remove the newly introduced variables then all changes will be confined to the same line, then it is probably possible to review with a word diff - the fact that it is all test coverage gives us confidence we haven't broken anything because otherwise the tests should fail.
I suppose one obvious way of breaking it up is to split assertNoText() out into its own separate issue.
Alternatively we could go back to #2 or #3 and initially fix only t() with no string replacement parameters? And then fix the string replacement variants in a followup?
Comment #21
paulocsOkay. I will create a new issue for assertNoText() as a child issue from this one.
Issue #3166349: Remove uses of t() in assertNoText() was created.
Comment #22
longwaveThe spinoff was committed so we can narrow scope here.
Comment #23
paulocsThanks @longwave, I forgot to edit the issue title.
I'll work on it!
Comment #24
paulocsNew patch.
Comment #25
paulocsPatch #24 can't be applied, so I'm attaching a new one.
Comment #27
mondrakeIt would be good to temporarily add a conditional deprecation so that when a t() object is passed in, a deprecation error is thrown. This would give us confidence that there are no more cases left out. Since we do not want to be strict here and drop passing t() altogether, when confirmed the above we can remove the temporary deprecation from the patch, before RTBC.
Comment #28
longwaveFixed test fail, also added the temporary deprecation suggested in #27.
Comment #30
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedWorking on this.
Comment #31
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedHere I have tried to fix failed tests.
Comment #32
Vidushi Mehta CreditAttribution: Vidushi Mehta at gai Technologies Pvt Ltd commentedComment #33
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedSetting status to NW as patch #31 didn't pass the tests.
Comment #34
nikitagupta CreditAttribution: nikitagupta at Srijan | A Material+ Company for Drupal India Association commentedComment #35
nikitagupta CreditAttribution: nikitagupta at Srijan | A Material+ Company for Drupal India Association commentedComment #37
longwave@nikitagupta please upload interdiffs and write comments as to what you changed when you upload new patches - when patches are hundreds of kb as here it is very hard to figure out what has changed otherwise.
Comment #38
SpokjeComment #39
SpokjeComment #40
SpokjeComment #41
SpokjeComment #42
longwaveFixing some failures.
Comment #44
longwaveHopefully this covers all the remaining ones.
Comment #45
mondrakeCan we do the same for assertNoText, just to make sure in the peer issue #3166349: Remove uses of t() in assertNoText() we have not missed anything?
Comment #46
mondrakeComment #47
longwaveSure, though if this fails badly, due to the size of the patch I think we should move fixing this to a followup.
Comment #49
mondrake#47 thanks, agree to fix the couple of residual instnces in a follow up is better.
Let’s remove the @trigger_error from #44 here then, and this is RTBC.
Comment #50
longwaveNeeds a change record to link the deprecation message to.
Comment #51
mondrakeMy suggestion is to just remove all this change:
In #27 I was suggesting to add it while developing the patch to automate checking we address all the conversions, but I am not sure we must enforce this permanently.
assertText
forces casting the argument to string:If we do that we do not need a CR.
Comment #52
shetpooja04 CreditAttribution: shetpooja04 at QED42 commentedRemoving the @trigger_error from #44 as mentioned in #49 and #51
Comment #53
mondrakeComment #54
shetpooja04 CreditAttribution: shetpooja04 at QED42 commentedRemoving the @trigger_error from #44 as mentioned in #49 and #51
Comment #55
mondrakeThis does not look quite right.
this seems out-of-scope here.
Comment #56
mondrakeFiled #3176200: Remove more uses of t() in assertNoText() for #47 #49.
Comment #57
mondrakeAlso, there is one coding standard issue.
Comment #58
sarvjeetsingh CreditAttribution: sarvjeetsingh as a volunteer and at QED42 for Drupal India Association commentedUpdated patch after making changes as per #55 and #57.
Comment #59
mondrakeThank you!
Comment #61
longwaveIn ManageFieldsFunctionalTest I couldn't get the test to work when asserting plain text, the
<em>
markup was getting in the way. I figured that all it is really looking for is that the field is correctly created, and asserting the label field seemed to be a cleaner way of doing this than looking for the page title.I think TrackerNodeAccessTest would also fail if we put the deprecation back in:
When
t()
is used to create the nodes, both titles are TranslatableMarkup instead of bare strings.Comment #62
mondrakeI think here we can use straight
$this->assertSession()->pageTextContains(...)
that strips the tags from the response before making the check.Comment #63
shetpooja04 CreditAttribution: shetpooja04 at QED42 commentedUploading patch for the points mentioned in comments #61 and #62
Please review
Comment #64
mondrakeI would revert all of the TrackerNodeAccessTest changes in #63 - it's not changing the essence of passing a translatable object to drupalCreateNode, and the second argument in assertText must be removed anyway in #3159788: AssertLegacyTrait::assert(No)Text() in functional tests still have a message passed in.
#61 then I'm even more convinced to remove the deprecation trigger here, it would be too strict. If we end up having assertions where there's mistmatch between the arguments in terms of tags included or not, we can address in the final #3139404: Replace usages of AssertLegacyTrait::assertText, that is deprecated.
Comment #65
mondrakeComment #66
shetpooja04 CreditAttribution: shetpooja04 at QED42 commentedReverting changes made in TrackerNodeAccessTest as mentioned in comment #64
Comment #67
mondrakeWe need to revert this one too.
Comment #68
shetpooja04 CreditAttribution: shetpooja04 at QED42 commentedReverted the changes according to #67
Comment #69
mondrakeThank you! Adding interdiff between #54 and #68.
Comment #70
xjmPostponing on #3167880-19: [meta] Convert assertions involving use of xpath to WebAssert, where possible.
Comment #71
mondrakeThis has nothing to do with #3167880: [meta] Convert assertions involving use of xpath to WebAssert, where possible.
Comment #72
xjm@mondrake Oops sorry, got the xpath tab mixed in with the ones #3157960: Replace t() calls with $this->t() in ToolbarAdminMenuTest.php and ToolbarMenuTranslationTest.php. Thanks for catching it.
Comment #73
xjmOnce a committer has reviewed this, we should schedule it as a beta target (sometime about the second week of November).
Comment #75
PasqualleComment #76
longwaveRerolled. Hopefully this can be scheduled for early next week?
Comment #77
mondrakeComment #78
mondrakeComment #79
mondrakererolled
Comment #80
longwave#79 is good for 9.2.x but 9.1.x has diverged slightly so we need separate patches.
Comment #82
catchCommitted/pushed to 9.2.x and 9.1.x, thanks!