Problem observed by BR0kEN in #2863756-10: Unnecessary t() calls in tests. Tests are using both double-quoted and single-quoted strings for messages. Single quotes and double quotes are used interchangeably for strings throughout the code.

Coding standards do not require the use of either, but advise consistency. From https://www.drupal.org/docs/develop/standards/coding-standards#quotes :

Drupal does not have a hard standard for the use of single quotes vs. double quotes. Where possible, keep consistency within each module, and respect the personal style of other developers.

With that caveat in mind, single quote strings should be used by default. Their use is recommended except in two cases:

1. Deliberate in-line variable interpolation, e.g. "$header".
2. Translated strings where one can avoid escaping single quotes by enclosing the string in double quotes. One such string would be "He's a good person." It would be 'He\'s a good person.' with single quotes. Such escaping may not be handled properly by .pot file generators for text translation, and it's also somewhat awkward to read.

CommentFileSizeAuthor
#17 interdiff-2865115-16-17.txt468 bytesl0ke
#17 password_policy-7.x-2.x-quote_strings_consistently-2865115-17.patch6.8 KBl0ke
#16 interdiff-2865115-14-16.txt994 bytesl0ke
#16 password_policy-7.x-2.x-quote_strings_consistently-2865115-16.patch6.8 KBl0ke
#15 interdiff-2865115-8-15.txt2.11 KBaohrvetpv
#15 password_policy-7.x-1.x-quote_strings_consistently-2865115-15.patch39.97 KBaohrvetpv
#14 password_policy-7.x-2.x-quote_strings_consistently-2865115-14.patch6.83 KBaohrvetpv
#14 interdiff-2865115-6-14.txt835 bytesaohrvetpv
#8 interdiff-6-8-1.x.txt16.87 KBl0ke
#8 password_policy-consistent-quote-strings-2865115-8-1.x.patch39.97 KBl0ke
#6 interdiff-4-6-2.x.txt1.42 KBl0ke
#6 password_policy-consistent-quote-strings-2865115-6-2.x.patch6.61 KBl0ke
#6 password_policy-consistent-quote-strings-2865115-6-1.x.patch23.46 KBl0ke
#4 password_policy-consistent-quote-strings-2865115-4.patch5.34 KBl0ke

Comments

AohRveTPV created an issue. See original summary.

aohrvetpv’s picture

aohrvetpv commented
Issue summary: View changes
l0ke’s picture

l0ke
Primary language English
Location Ukraine
commented
Assigned: Unassigned » l0ke

Will work on it. Just to confirm:
The quoting in password_policy.test file be fixed in #2863756: Unnecessary t() calls in tests and here we need to check all other files, am I right?

l0ke’s picture

l0ke
Primary language English
Location Ukraine
commented
Status: Active » Needs review
StatusFileSize
new password_policy-consistent-quote-strings-2865115-4.patch5.34 KB 
-  $condition = t("nh.hid = p.hid AND nh.name='@name' AND nh.timeframe = '@timeframe'", array('@name' => $policy_name, '@timeframe' => $notice_int));
+  $condition = "nh.hid = p.hid AND nh.name='{$policy_name}' AND nh.timeframe = '{$notice_int}'";

Completely not properly used t() function here, so replaced with inline variables.

Didn't do any changes is password_policy.test

Status: Needs review » Needs work

The last submitted patch, 4: password_policy-consistent-quote-strings-2865115-4.patch, failed testing.

l0ke’s picture

l0ke
Primary language English
Location Ukraine
commented
Status: Needs work » Needs review
StatusFileSize
new password_policy-consistent-quote-strings-2865115-6-1.x.patch23.46 KB
new password_policy-consistent-quote-strings-2865115-6-2.x.patch6.61 KB
new interdiff-4-6-2.x.txt1.42 KB

Oh, didn't notice it's for 1.x.

  1. Attaching patch for 1.x
  2. #4 was for 2.x, and attaching patch with a bit more changes to it
aohrvetpv’s picture

aohrvetpv commented

Thanks very much for the patches! I think the schema descriptions containing single quotes should use double quotes per (2) from the Drupal Coding Standards section on quotes:

2. Translated strings where one can avoid escaping single quotes by enclosing the string in double quotes. One such string would be "He's a good person." It would be 'He\'s a good person.' with single quotes. Such escaping may not be handled properly by .pot file generators for text translation, and it's also somewhat awkward to read.

These strings are not translated directly using t(), but my understanding is the Schema API does translate them.

I was hoping to also change the .test strings in this issue so that we can fix all the strings in one commit (per branch).

l0ke’s picture

l0ke
Primary language English
Location Ukraine
commented
StatusFileSize
new password_policy-consistent-quote-strings-2865115-8-1.x.patch39.97 KB
new interdiff-6-8-1.x.txt16.87 KB

Patch for 2.x has everything.

As for 1.x

  • No way this descriptions are translated, these are table/field COMMENT property in database.
  • There also was an inconsistency in JS, while there are no strict standard for quotation in JS it is preferable to be consistent. (e.g. #2548195: Only use single quotes in JavaScript and update .eslintrc).
    So I did changes to JavaScript also.
  • And here is updated patch with changes to password_policy.test
aohrvetpv’s picture

aohrvetpv commented

This change concerns me:

-  $condition = t("nh.hid = p.hid AND nh.name='@name' AND nh.timeframe = '@timeframe'", array('@name' => $policy_name, '@timeframe' => $notice_int));
+  $condition = "nh.hid = p.hid AND nh.name='{$policy_name}' AND nh.timeframe = '{$notice_int}'";

Substituting variables without sanitizing may allow for SQL injection. t() is probably wrong, but I suspect this isn't the proper way to substitute for queries.

aohrvetpv’s picture

aohrvetpv commented

Unrelated issue:

password_policy.module:      'symbol_count_symbols' => '!@#$%^&*()_+=-|}{"?:><,./;\'\\[]',

password_policy.test:        'symbol_count_symbols' => '!@#$%^&*()_+=-|}{"?:><,./;\'\\[]',

plugins/constraint/symbol_count.inc:    'symbol_count_symbols' => '!@#$%^&*()_+=-|}{"?:><,./;\'\[]',

It looks like the symbol count constraint does not count backslashes due to a missing escape character.

aohrvetpv’s picture

aohrvetpv commented

I was wrong in #10. It looks like preg_quote() takes care of the escaping.

However I noticed ~ and ` are not counted as symbols. Shouldn't they be?

aohrvetpv’s picture

aohrvetpv commented

Re #9, I think the proper way to substitute is to use placeholders and the arguments parameter to leftJoin(). See: https://api.drupal.org/api/drupal/includes!database!select.inc/function/...

aohrvetpv’s picture

aohrvetpv commented

Created issue for #11:
#2865506: Symbol count constraint does not count backtick and tilde

aohrvetpv’s picture

aohrvetpv commented
StatusFileSize
new interdiff-2865115-6-14.txt835 bytes
new password_policy-7.x-2.x-quote_strings_consistently-2865115-14.patch6.83 KB

Made change suggested in #12.

aohrvetpv’s picture

aohrvetpv commented
StatusFileSize
new password_policy-7.x-1.x-quote_strings_consistently-2865115-15.patch39.97 KB
new interdiff-2865115-8-15.txt2.11 KB

Changed a few more strings to use single quotes.

l0ke’s picture

l0ke
Primary language English
Location Ukraine
commented
StatusFileSize
new password_policy-7.x-2.x-quote_strings_consistently-2865115-16.patch6.8 KB
new interdiff-2865115-14-16.txt994 bytes

#15 Patch for 7.x-1.x RTBC.

#14Placeholders++
But I don't like that use of variables $condition $arguments we define them to use only once in leftJoin(), I would suggest to use inline statement.

l0ke’s picture

l0ke
Primary language English
Location Ukraine
commented
StatusFileSize
new password_policy-7.x-2.x-quote_strings_consistently-2865115-17.patch6.8 KB
new interdiff-2865115-16-17.txt468 bytes 
+++ b/plugins/constraint/symbol_count.inc
@@ -12,7 +12,7 @@ $plugin = array(
-    'symbol_count_symbols' => "!@#$%^&*()_+=-|}{\"?:><,./;'\[]",
+    'symbol_count_symbols' => '!@#$%^&*()_+=-|}{"?:><,./;\'\[]',

Just noticed symbol_count_symbols string in symbol_count.inc is corrupted should be <code>'!@#$%^&*()_+=-|}{"?:><,./;\'\\[]'
Difference is in this part:
wrong \'\[]
correct \'\\[]

I guess it affects #2865506: Symbol count constraint does not count backtick and tilde as well

  • AohRveTPV committed 2d17b69 on 7.x-2.x authored by l0ke 
    Issue #2865115 by l0ke, AohRveTPV: Quote strings in consistent manner

  • AohRveTPV committed 9725815 on 7.x-1.x authored by l0ke 
    Issue #2865115 by l0ke, AohRveTPV: Quote strings in consistent manner
aohrvetpv’s picture

aohrvetpv commented
Status: Needs review » Fixed

Committed #16. We can investigate #17 in #2865506: Symbol count constraint does not count backtick and tilde.

Status: Fixed » Closed (fixed)

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