br0ken mentioned in #2779135: "Delay" constraint allow to change system-generated password that there are a lot of unnecessary calls to t() in tests.

Per https://www.drupal.org/docs/7/testing/assertions:

Note: Whenever you pass a message into a SimpleTest assertion, do NOT translate the message using t(). (To format a string with variables, use format_string() instead.)

7.x-1.x likely has this problem as well.

CommentFileSizeAuthor
#11 interdiff-2863756-7-9.txt31.25 KBaohrvetpv
#9 password_policy-7.x-1.x-remove_t_from_assert_messages-2863756-9.patch45.13 KBaohrvetpv
#7 password_policy-7.x-1.x-remove_t_from_assert_calls-2863756-7.patch46.19 KBaohrvetpv
#2 password_policy-7.x-2.x-remove_t_from_assert_calls-2863756-2.patch15.33 KBaohrvetpv

Comments

AohRveTPV created an issue. See original summary.

aohrvetpv’s picture

aohrvetpv commented
StatusFileSize
new password_policy-7.x-2.x-remove_t_from_assert_calls-2863756-2.patch15.33 KB
aohrvetpv’s picture

aohrvetpv commented
Status: Active » Needs review
br0ken’s picture

br0ken
Primary language Ukrainian
commented
Issue tags: +DevDaysSeville

I've cloned the repo and have verified that all t() were removed after applying the patch. We even can to not wait for tests to pass :)

br0ken’s picture

br0ken
Primary language Ukrainian
commented
Status: Needs review » Reviewed & tested by the community

  • AohRveTPV committed 9026c7f on 7.x-2.x 
    Issue #2863756 by AohRveTPV, BR0kEN: Unnecessary t() calls in tests
aohrvetpv’s picture

aohrvetpv commented
Version: 7.x-2.x-dev » 7.x-1.x-dev
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new password_policy-7.x-1.x-remove_t_from_assert_calls-2863756-7.patch46.19 KB
br0ken’s picture

br0ken
Primary language Ukrainian
commented
Status: Needs review » Needs work

@AohRveTPV, first argument for methods asserting text, should be processed by t(). Otherwise tests won't work on another language.

aohrvetpv’s picture

aohrvetpv commented
Status: Needs work » Needs review
StatusFileSize
new password_policy-7.x-1.x-remove_t_from_assert_messages-2863756-9.patch45.13 KB
br0ken’s picture

br0ken
Primary language Ukrainian
commented
Status: Needs review » Needs work 
  1. +++ b/tests/password_policy_expiration.test
@@ -366,13 +366,13 @@ class PasswordPolicyExpirationTestCase extends DrupalWebTestCase {
+    $this->assertNoMail("No e-mails sent initially.");

    No need to use double quotes. 

  2. +++ b/tests/password_policy_expiration.test
@@ -366,13 +366,13 @@ class PasswordPolicyExpirationTestCase extends DrupalWebTestCase {
+    $this->assertNoMail("No e-mails sent before first expiration warning day.");

    No need to use double quotes. 

  3. +++ b/tests/password_policy_expiration.test
@@ -381,13 +381,13 @@ class PasswordPolicyExpirationTestCase extends DrupalWebTestCase {
+    $this->assertMail('subject', $warning_subject, "First expiration warning e-mail sent.");

    No need to use double quotes. 

  4. +++ b/tests/password_policy_expiration.test
@@ -381,13 +381,13 @@ class PasswordPolicyExpirationTestCase extends DrupalWebTestCase {
+    $this->assertNoMail("No e-mails sent between first and second expiration warning days.");

    No need to use double quotes. 

  5. +++ b/tests/password_policy_expiration.test
@@ -396,23 +396,23 @@ class PasswordPolicyExpirationTestCase extends DrupalWebTestCase {
+    $this->assertMail('subject', $warning_subject, "Second expiration warning e-mail sent.");

    No need to use double quotes. 

  6. +++ b/tests/password_policy_expiration.test
@@ -396,23 +396,23 @@ class PasswordPolicyExpirationTestCase extends DrupalWebTestCase {
+    $this->assertNoMail("No e-mails sent after expiration warnings and before expiration.");

    No need to use double quotes. 

  7. +++ b/tests/password_policy_expiration.test
@@ -396,23 +396,23 @@ class PasswordPolicyExpirationTestCase extends DrupalWebTestCase {
+    $this->assertNoMail("No e-mails sent after expiration warnings and less than one day before expiration.");

    No need to use double quotes. 

  8. +++ b/tests/password_policy_expiration.test
@@ -396,23 +396,23 @@ class PasswordPolicyExpirationTestCase extends DrupalWebTestCase {
+    $this->assertNoMail("No e-mails sent after expiration.");

    No need to use double quotes. 

  9. +++ b/tests/password_policy_expiration.test
@@ -440,7 +440,7 @@ class PasswordPolicyExpirationTestCase extends DrupalWebTestCase {
+    $this->assertNoMail("No e-mails sent.");

    No need to use double quotes. 

  10. +++ b/tests/password_policy_expiration.test
@@ -467,7 +467,7 @@ class PasswordPolicyExpirationTestCase extends DrupalWebTestCase {
+    $this->assertMail('subject', $warning_subject, "Tokens replaced in subject correctly.");

    No need to use double quotes.

@AohRveTPV, please provide an interdiff when you creating patches. Otherwise it's hard to detect what was changed since last patch.

FYI: placeholders, which starts from !, should be used only in case if you're going to render HTML as is. Also, this kind of placeholders is potentially unsafe (was removed in Drupal 8 - #2506427: [meta] !placeholder causes strings to be escaped and makes the sanitization API harder to understand).

aohrvetpv’s picture

aohrvetpv commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2863756-7-9.txt31.25 KB

Re quotes: I would like to make quoting consistent throughout the code, and not just in tests, so I created a new issue: #2865115: Quote strings in consistent manner.

Drupal coding standards do not require either single quotes or double quotes. To me, it would seem better to use double quotes for every human-readable message because then you do not have to worry about escaping single quotes in contracted words. For instance, in password_policy.install:

$schema['password_policy_history'] = array(
    'description' => "Stores user's old password hashes.",
    'fields' => array(
      'hid' => array(
        'description' => 'Primary Key: Unique history ID.',
        'type' => 'serial',
        'unsigned' => TRUE,
        'not null' => TRUE,
      ),
      'uid' => array(
        'description' => "User's {users}.uid.",
        'type' => 'int',
        'not null' => TRUE,

However, Drupal suggests using single quotes by default, and only using double quotes when a string contains a single quote. This seems to me more error prone, because you have to think more about which quotes to use based on the string. That is, if you use double quotes, you do not have to think about whether the string might contain a single quote. I am willing to switch to this approach though for consistency with Drupal core even though to me it seems inferior to using double quotes by default for human-readable messages.

Re placeholders: We can also fix these separately.

  • AohRveTPV committed 3ef2251 on 7.x-1.x 
    Issue #2863756 by AohRveTPV, BR0kEN: Unnecessary t() calls in tests
aohrvetpv’s picture

aohrvetpv commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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