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 several calls to t() in calls to clickLink() 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 clickLink(), except those used by translation-related code (if any).

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#27 3153469-27-9.2.x.patch90.49 KBlongwave
#22 3153469-22-without-deprecation.patch91.07 KBhmendes
#22 3153469-22-with-deprecation.patch91.99 KBhmendes
#20 3153469-20-with-deprecation.patch92.05 KBhmendes
#20 3153469-20-without-deprecation.patch91.13 KBhmendes
#17 3153469-17-without-deprecation.patch90.7 KBlongwave
#17 3153469-17-with-deprecation.patch91.62 KBlongwave
#15 3153469-15-without-deprecation.patch90.17 KBlongwave
#15 3153469-15-with-deprecation.patch91.08 KBlongwave
#13 interdiff.3153469.11-13.txt2.45 KBlongwave
#13 3153469-13.patch92.52 KBlongwave
#11 interdiff.3153469.10-11.txt939 byteslongwave
#11 3153469-11.patch91.09 KBlongwave
#5 3153469-5.patch90.22 KBHardik_Patel_12
#10 3153469-10.patch90.17 KBlongwave
#5 interdiff-2-5.txt4.88 KBHardik_Patel_12
#2 3153469-2.patch90.34 KBHardik_Patel_12
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Hardik_Patel_12 created an issue. See original summary.

Hardik_Patel_12’s picture

Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 for Drupal India Association commented
Status: Active » Needs review
FileSize
3153469-2.patch90.34 KB

Kindly review the patch.

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

If I install Drupal in another language using this patch and run the tests, they will fail.
Not sure about the rationale behind this change?

Status: Needs review » Needs work

The last submitted patch, 2: 3153469-2.patch, failed testing. View results

Hardik_Patel_12’s picture

Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 for Drupal India Association commented
Status: Needs work » Needs review
FileSize
3153469-5.patch90.22 KB
interdiff-2-5.txt4.88 KB

@joseph.olstad , 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. We will need to ensure to leave t() where the test actually tests the translation system.

Solving failed test cases.

Status: Needs review » Needs work

The last submitted patch, 5: 3153469-5.patch, failed testing. View results

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

what if we want to test an install profile for automated tests and that install profile uses a default language other than english?
those tests would break, no?

For such a minuscule performance difference, not sure if this change is worth the effort given that at some point it might be useful to test an install profile test runner in a language other than english.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Title: Remove uses of t() in clickLink(), responseContains() and assertUniqueText() calls » Remove uses of t() in clickLink() calls

I'm going to rescope this a bit tighter as the three methods in the original issue don't seem very closely related, and at the time of writing clickLink() is one of the biggest remaining uses of t() calls.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs work » Needs review
FileSize
3153469-10.patch90.17 KB
longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Issue summary: View changes
FileSize
3153469-11.patch91.09 KB
interdiff.3153469.10-11.txt939 bytes

Let's add a deprecation so we can find the remaining uses.

Status: Needs review » Needs work

The last submitted patch, 11: 3153469-11.patch, failed testing. View results

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs work » Needs review
FileSize
3153469-13.patch92.52 KB
interdiff.3153469.11-13.txt2.45 KB

Fixed the remaining failures.

clickLink already asserts that the link exists, so we can remove some redundancy here too.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Needs work
Issue tags: +Needs reroll
longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
3153469-15-with-deprecation.patch91.08 KB
3153469-15-without-deprecation.patch90.17 KB

First patch includes a deprecation to check that nothing was missed. Second patch is probably the one that should be committed, as we don't strictly need to deprecate this.

The last submitted patch, 15: 3153469-15-with-deprecation.patch, failed testing. View results

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
FileSize
3153469-17-with-deprecation.patch91.62 KB
3153469-17-without-deprecation.patch90.7 KB

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Needs work
Issue tags: +Needs reroll
hmendes’s picture

hmendes CreditAttribution: hmendes at CI&T commented
Status: Needs work » Needs review
FileSize
3153469-20-without-deprecation.patch91.13 KB
3153469-20-with-deprecation.patch92.05 KB

Re-rolled both patches.
Please review.

Status: Needs review » Needs work

The last submitted patch, 20: 3153469-20-with-deprecation.patch, failed testing. View results

hmendes’s picture

hmendes CreditAttribution: hmendes at CI&T commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
3153469-22-with-deprecation.patch91.99 KB
3153469-22-without-deprecation.patch91.07 KB

Sorry, adding new patches, hope its all right now.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Reviewed & tested by the community

I have worked on some patches in this issue but this is mostly a trivial find and replace, and the reroll looks good to me.

I think the "without deprecation" patch is the one to commit - I am not sure we should enforce strings as it might get in the way of downstream users testing with translations enabled.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 9.3.x-dev » 9.2.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

what if we want to test an install profile for automated tests and that install profile uses a default language other than english?
those tests would break, no?

@joseph.olstad this is just not how the tests are written - every browser test installs Drupal again, with a profile specified in the test. The Drupal install itself that you're running tests from has as little as possible to do with this.

If you write a test for an install profile or site where English isn't the default language, then rather than using t() you'd want to specific the correct text in the language under test.

Committed/pushed (the 'without deprecation' version - if we wanted to deprecate we'd need a follow-up issue that only does that) to 9.3.x, thanks!

This doesn't cherry-pick cleanly for 9.2.x, so needs a re-roll there.

  • catch committed 686951d on 9.3.x 
    Issue #3153469 by longwave, hmendes, Hardik_Patel_12: Remove uses of t...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Title: Remove uses of t() in clickLink() calls » [backport] Remove uses of t() in clickLink() calls
longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
3153469-27-9.2.x.patch90.49 KB
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Needs review » Reviewed & tested by the community

Thanks for the re-roll, looks good. Will commit this tomorrow if no objections.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.2.x, thanks!

  • catch committed 87feede on 9.2.x 
    Issue #3153469 by longwave, hmendes, Hardik_Patel_12: Remove uses of t...

Status: Fixed » Closed (fixed)

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