Remove t() from asserts from Form sub-system tests [#1797318]

Comment	File	Size	Author
#16	1797318-16-t-assert-form.patch	42.86 KB	dcam

#11	1797318-11-t-assert-form.patch	52.77 KB	Lars Toomre

#8	1797318-8-t-assert-form.patch	52.16 KB	Lars Toomre

#6	t-assert-form-1797318-6.patch	52.56 KB	Lars Toomre

#6	interdiff-1797318-3-6.txt	5.81 KB	Lars Toomre
#3	t-assert-form-1797318-3.patch	53.67 KB	Lars Toomre

Comment #1

Lars Toomre CreditAttribution: Lars Toomre commented 27 September 2012 at 23:42

Title:	Remove t() from asserts from File sub-system tests	» Remove t() from asserts from Form sub-system tests
Assigned:	Unassigned	» Lars Toomre

Correcting title... Also noting that there needs to be testUserRegistrationRebuild() looked at closely. It appears that someone took out a t() around 'Rebuild' when they should not have.

Log in or register to post comments

Comment #2

Lars Toomre CreditAttribution: Lars Toomre commented 27 September 2012 at 23:58

It appears that there are more than a few instances in the tests for this sub-system where the $submit parameter in drupalPost is not wrapped in t(). This should be followed up on as a part of cleaning up this sub-systems tests.

Log in or register to post comments

Comment #3

Lars Toomre CreditAttribution: Lars Toomre commented 28 September 2012 at 00:52

Status:

Active

» Needs review

File	Size
t-assert-form-1797318-3.patch	53.67 KB

Here is an initial untested patch for this issue. This patch includes format_string() conversions as well.

Log in or register to post comments

Comment #4

xjm

she/her

English

CreditAttribution: xjm commented 28 September 2012 at 03:43

Status:	Needs review	» Needs work
Issue tags:		+Needs backport to D7

This looks good for the most part. I found two things to fix:

+++ b/core/modules/system/lib/Drupal/system/Tests/Form/EmailTest.phpundefined
@@ -39,8 +39,8 @@ class EmailTest extends WebTestBase {
-    $this->assertRaw(t('The e-mail address %mail is not valid.', array('%mail' => 'invalid')));
-    $this->assertRaw(t('!name field is required.', array('!name' => 'Address')));
+    $this->assertRaw(format_string('The e-mail address %mail is not valid.', array('%mail' => 'invalid')));
+    $this->assertRaw(format_string('!name field is required.', array('!name' => 'Address')));

This change is not correct. These assertions have no overridden message; the string in question is part of the test.

+++ b/core/modules/system/lib/Drupal/system/Tests/Form/TriggeringElementTest.phpundefined
@@ -43,7 +43,7 @@ class TriggeringElementTest extends WebTestBase {
-    $this->assertText('There is no clicked button.', t('$form_state[\'triggering_element\'] set to NULL.'));
+    $this->assertText('There is no clicked button.', format_string('$form_state[\'triggering_element\'] set to NULL.'));

These don't actually need to use format_string() since there are no variables. (The $ is part of the message string.) I count eight assertions like this in this class.

Please provide an interdiff when rerolling the patch. Thanks!

Log in or register to post comments

Comment #5

Lars Toomre CreditAttribution: Lars Toomre commented 28 September 2012 at 04:30

My understanding is that we had to use format_string when we have escaping in a string. If that is wrong, happy to change.

Log in or register to post comments

Comment #6

Lars Toomre CreditAttribution: Lars Toomre commented 28 September 2012 at 17:33

Status:

Needs work

» Needs review

File	Size
interdiff-1797318-3-6.txt	5.81 KB
t-assert-form-1797318-6.patch	52.56 KB

Here is a revised patch that addresses the issues in #4. Also attached is an interdiff (which happens to be my first ever have created so I hope it makes sense).

Log in or register to post comments

Comment #7

dcam CreditAttribution: dcam commented 19 October 2012 at 04:40

Status:	Needs review	» Needs work
Issue tags:		+Needs reroll

#6 no longer applies due to changes to ElementsVerticalTabsTest.php introduced by #1742438: Vertical tabs header and container rendered even when no vertical tabs. It needs a reroll, but I'm not sure if #1742438 should be considered as a blocker to this issue. It had to be moved back to 8.x due to an issue that was created. So there are likely more test changes coming down the pipe.

Log in or register to post comments

Comment #8

Lars Toomre CreditAttribution: Lars Toomre commented 19 October 2012 at 17:14

Issue tags:

-Needs reroll

File	Size
1797318-8-t-assert-form.patch	52.16 KB

Thanks for looking at this @dcam. Attached is a a re-roll as #7 noted.

It appears that #1742438: Vertical tabs header and container rendered even when no vertical tabs is not yet ready for a commit to D8. Hence, I think that if this patch is reviewed and marked as RTBC, it can proceed. If the other issue gets committed first, this of course can be re-rolled.

I would like to see another 117 calls to t() eliminated. Thanks in advance for taking a look at this.