Ensure translatable strings, and string domains are not dynamic #136

shadyvb · 2013-12-12T05:00:21Z

Fixes #50.

jrfnl · 2013-12-12T10:20:43Z

@shadyvb May I ask why you would disallow dynamic string domains ? I realize that the .pot file can be generated in the WP repo if you use absolute string domains, but there are lots of other tools available to generate .pot files which work without issue with dynamic string domains and the actual translation of strings works fine with dynamic string domains.

Also from a programming point of view it's actually good practice as it makes the code more flexible and easier to maintain (no duplicate information).

May be a warning would be more appropriate ? Preferably a warning which can be turned off ?
At the end of the day, there is no reason to force developers to use the .pot generation via the WP repo or is there ?

jrfnl · 2013-12-12T10:22:53Z

Tests/WP/I18nUnitTest.php

+            7 => 1,
+            9 => 1,
+            );
+


Should line 5 be included too ?

No, because __( "hell\$varo", 'domain' ); does not contain a variable. It is escaped.

The comment was incorrectly saying it was bad, however. It is actually good.

GaryJones · 2014-10-11T10:23:12Z

WordPress/Tests/WP/I18nUnitTest.inc

+
+__( $var, 'domain' ); // Bad, shouldn't use variables
+
+__( 'string', SOMETHING ); // Bad, shouldn't use CONSTANTS


Few more unit tests:

__( 'string' . $var, 'domain' ); // Bad, shouldn't use variable for string __( $var . 'string', 'domain' ); // Bad, shouldn't use variable for string __( SOMETHING, 'domain' ); // Bad, shouldn't use constant for string __( 'string' . SOMETHING, 'domain' ); // Bad, shouldn't use constant for string __( SOMETHING . 'string', 'domain' ); // Bad, shouldn't use variable for string __( 'string', $domain ); // Bad, shouldn't use variable for domain __( 'string', 'my' . $domain ); // Bad, shouldn't use variable for domain __( 'string', $domain . 'domain' ); // Bad, shouldn't use variable for domain __( 'string', 'domain' ); // Good _x( 'string', 'context', 'domain' ); // Good _x( 'string', $var, 'domain' ); // Bad, shouldn't use variable for context _x( 'string', 'context' . $var, 'domain' ); // Bad, shouldn't use variable for context _x( 'string', $var . 'context', 'domain' ); // Bad, shouldn't use variable for context _x( 'string', SOMETHING, 'domain' ); // Bad, shouldn't use constant for context _x( 'string', SOMETHING . 'context', 'domain' ); // Bad, shouldn't use constant for context _x( 'string', 'context' . SOMETHING, 'domain' ); // Bad, shouldn't use constant for context

westonruter · 2015-05-13T04:07:14Z

May I ask why you would disallow dynamic string domains ?

@jrfnl: See https://markjaquith.wordpress.com/2011/10/06/translating-wordpress-plugins-and-themes-dont-get-clever/

jrfnl · 2015-05-13T10:33:31Z

@westonruter See my original comment in which I already respond to that article. Mark presumes in his article that people don't know how to generate .pot files outside of using the tooling in the WP repo admin.
I think an error is out of place, a warning may be more appropriate.

[Edit] Oh and just be sure that I'm making myself clear - I'm not talking about the string to be translated, only about the text-domain.

GaryJones · 2015-05-13T11:17:27Z

At this moment in time, even the WP i18n tools don't actually limit the strings collected by the text domain i.e. the final argument (string, variable, constant, something else) is not actually read. This is why it also works for core i18n strings which obviously don't have that arg. The precaution here is trying to make future developments which may consider the text domain, easier.

jrfnl · 2015-05-13T11:27:35Z

Thanks for that addition @GaryJones.

Sounds to me like all the more reason to differentiate between the messages about the translatable string and messages about the domain, to make messages about the domain a warning and to make sure they have a separate error code, so people can turn them off without turning messages about the translatable strings off.

westonruter · 2015-05-13T21:46:22Z

WordPress/Sniffs/WP/I18nSniff.php

+            if ( $tokens[$i]['code'] === T_DOUBLE_QUOTED_STRING ) {
+                $string = $tokens[$i]['content'];
+                if ( preg_match( '#\$#', $string ) > 0 ) {
+                    $phpcsFile->addError( 'Translatable strings should not contain variables, found ' . $tokens[$i]['content'], $i );


Per @jrfnl's note on this PR:

Sounds to me like all the more reason to differentiate between the messages about the translatable string and messages about the domain, to make messages about the domain a warning and to make sure they have a separate error code, so people can turn them off without turning messages about the translatable strings off.

This can be change to a warning, and the third argument can he supplied to the method to provide the warning with an identifier which can be used to target the warning in a custom ruleset.

As a side question, shouldn't all our errors and warnings have identifiers so folks can target them as they desire?

As a side question, shouldn't all our errors and warnings have identifiers so folks can target them as they desire?

@GaryJones yes, absolutely.

…ds/WordPress-Coding-Standards into issue-50

westonruter · 2016-03-14T22:33:26Z

I checked with @ocean90, and he says if it is hosted on WordPress.org, the string literal text domain must match the theme/plugin slug. So…

another check we can is to see if the string literal corresponds to a directory name in the file's directory hierarchy as a basic way to check for conformance to this.

ocean90 · 2016-03-14T22:35:21Z

WordPress/Sniffs/WP/I18nSniff.php

+
+	public $i18n_functions = array(
+		'translate',
+		'translate_with_gettext_context',


translate() and translate_with_gettext_context() are low-API functions, they shouldn't be used by plugins IMO.

ocean90 · 2016-03-14T22:42:52Z

Since #50 is about "proper usage of translation functions", should this include a warning when _() (single underscore) gets used?

See also https://github.com/WordPress/WordPress/blob/bd4c250078deb4ade9b6b1d3ec884c63d6595e2e/wp-includes/compat.php#L9-L14.

westonruter · 2016-03-14T23:05:03Z

Warn when using _(), a single underscore.
Implement proper checks for _n() and friends.
Add discrete error codes.

jrfnl · 2016-03-15T02:05:38Z

Just an idea: Looking at the added tests - what about an optional extra check to verify that a text-domain has been provided ? Would be off by default (core), but could be turned on either via the extra rules or via a custom phpcs.xml ruleset.
With a recommendation for plugins and themes to turn it on ?

westonruter · 2016-03-15T02:21:22Z

@jrfnl I like that. Actually, I think we can accomplish this via a MissingTextDomain error code which for Core can just be suppressed in the default ruleset.

westonruter · 2016-03-15T07:15:48Z

WordPress-Extra/ruleset.xml

@@ -33,6 +33,7 @@
 	<rule ref="WordPress.WP.PreparedSQL"/>
 	<rule ref="WordPress.Variables.GlobalVariables"/>
 	<rule ref="WordPress.PHP.StrictComparisons" />
+	<rule ref="WordPress.WP.I18n" />


Should this go into WordPress-Extra or WordPress-Core? I'm just concerned about use with actual core, and how a lot of warnings will start appearing since no text domain is supplied.

westonruter · 2016-03-15T07:20:31Z

Ready for review.

westonruter · 2016-03-15T07:23:23Z

WordPress/Sniffs/WP/I18nSniff.php

+	 * @param array $tokens
+	 * @return bool
+	 */
+	protected function check_literal_string_text_tokens( $phpcs_file, $arg_name = '$text', $stack_ptr, $tokens = array() ) {


There's an unfortunate amount of duplication between check_literal_string_text_tokens, check_literal_string_context_tokens, and check_string_domain_tokens. However, an immediate refactoring wasn't apparent which would allow custom messages as well as error codes for each argument type.

There's an unfortunate amount of duplication between check_literal_string_text_tokens, check_literal_string_context_tokens, and check_string_domain_tokens. However, an immediate refactoring wasn't apparent which would allow custom messages as well as error codes for each argument type.

Aren't the messages and error codes identical other than the arg names though? ("translatable text", "context", "text domain") I think it could be refactored to a single method that would just accept those strings and build the error messages and codes from them?

Also, some are errors and some are warnings.

It could be refactored to try to abstract these out more by passing in params to common methods if you think it is worthwhile.

@JDGrimes here you go: a47029e

[ci skip]

westonruter · 2016-03-17T22:50:09Z

@JDGrimes would you be able to reviewmerge?

JDGrimes · 2016-03-18T13:29:46Z

WordPress/Sniffs/WP/I18nSniff.php

+ */
+class WordPress_Sniffs_WP_I18nSniff extends WordPress_Sniff {
+
+	public $i18n_functions = array(


Wouldn't it be better to flip this array and have the function names as keys (this is standard in most other sniffs)? Then you can use isset() instead of in_array(). And perhaps more importantly, you could let the nature of each function be specified by the values, rather than hardcoding this below. So you could have:

array( '__' => 'simple', '_x' => 'context', '_n' => 'plural', '_nx' => 'plural_context', )

ect.

Because right now if someone ever wanted to sub-class this and add their own custom translation functions (bad idea, I know), they'd have to duplicate all of the code rather than just add them to this list, because how each function is handled has to be hard-coded below.

Anyway, I just prefer isset() to in_array(). 😄

Great points! See 9ebd183.

grappler · 2016-03-24T09:42:38Z

The text domain can be a variable even for the WordPress makepot or on WordPress.org. The only condition for wordpress.org is that the text domain must match the theme/plugin slug.

I have tested it. Anyone who has says differently has not tested it. What is best practice is another discussion.

ocean90 · 2016-03-24T09:54:01Z

@grappler Nobody is saying that it doesn't work… See also #136 (comment).

Not sure why you think that it's just a "best practice" but it's definitely a WordPress coding standard which is enforced by several handbook pages, for example https://developer.wordpress.org/plugins/internationalization/how-to-internationalize-your-plugin/.

grappler · 2016-03-24T10:04:27Z

It can’t read variables or constants. It only reads strings. So your text domain strings needs to stay hardcoded as actual quoted strings. 'my-plugin-name'.

Quoting from the post https://markjaquith.wordpress.com/2011/10/06/translating-wordpress-plugins-and-themes-dont-get-clever/

Yes, I can accept it is WordPress coding standard. 😄

Ensure translatable strings, and string domains are not dynamic, fixes …

242c05f

…#50

jrfnl reviewed Dec 12, 2013
View reviewed changes

Merge branch 'develop' into issue-50

073620e

GaryJones reviewed Oct 11, 2014
View reviewed changes

westonruter reviewed May 13, 2015
View reviewed changes

westonruter added 3 commits March 14, 2016 15:09

Merge branch 'develop' of https://github.com/WordPress-Coding-Standar…

ab1075b

…ds/WordPress-Coding-Standards into issue-50

Add unit tests from @GaryJones; use WP coding standards

0fc4f46

Add WordPress.WP.I18n to WordPres-Extra

d02511e

ocean90 reviewed Mar 14, 2016
View reviewed changes

westonruter added 2 commits March 14, 2016 16:02

Improve checks for interpolated vars

022444a

Add intentionally-failing tests for _n() functions

6f21e41

westonruter added 3 commits March 15, 2016 00:01

Implement translation function-specific arg checking

baa4b9e

Warn against using translate() and translate_with_gettext_context()

3f2c695

Forbid using single-underscore _()

eada3e7

westonruter reviewed Mar 15, 2016
View reviewed changes

westonruter added the Status: Review ready label Mar 15, 2016

westonruter reviewed Mar 15, 2016
View reviewed changes

Fix some coding style and spelling

f2312fa

[ci skip]

JDGrimes reviewed Mar 18, 2016
View reviewed changes

westonruter added 2 commits March 18, 2016 12:37

Let i18n function types be defined in array

9ebd183

Refactor methods to reduce duplication; add too-many-args check

a47029e

JDGrimes added this to the 0.10.0 milestone Mar 23, 2016

JDGrimes merged commit 8c92bdf into develop Mar 23, 2016

JDGrimes deleted the issue-50 branch March 23, 2016 13:09

westonruter mentioned this pull request Apr 8, 2016

Fix _n_noop() and _nx_noop() #554

Merged

GaryJones removed the Status: Review ready label Aug 21, 2018

jrfnl added the Focus: I18n label Mar 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure translatable strings, and string domains are not dynamic #136

Ensure translatable strings, and string domains are not dynamic #136

shadyvb commented Dec 12, 2013

jrfnl commented Dec 12, 2013

jrfnl Dec 12, 2013

westonruter Oct 11, 2014

westonruter Mar 14, 2016

GaryJones Oct 11, 2014

westonruter commented May 13, 2015

jrfnl commented May 13, 2015

GaryJones commented May 13, 2015

jrfnl commented May 13, 2015

westonruter May 13, 2015

GaryJones May 14, 2015

westonruter May 14, 2015

westonruter commented Mar 14, 2016

ocean90 Mar 14, 2016

ocean90 commented Mar 14, 2016

westonruter commented Mar 14, 2016

jrfnl commented Mar 15, 2016

westonruter commented Mar 15, 2016

westonruter Mar 15, 2016

westonruter commented Mar 15, 2016

westonruter Mar 15, 2016

JDGrimes Mar 18, 2016

westonruter Mar 18, 2016

westonruter Mar 23, 2016

westonruter commented Mar 17, 2016

JDGrimes Mar 18, 2016

westonruter Mar 18, 2016

grappler commented Mar 24, 2016

ocean90 commented Mar 24, 2016

grappler commented Mar 24, 2016

+=> 1,
+=> 1,
+                          );


		__( $var, 'domain' ); // Bad, shouldn't use variables

		__( 'string', SOMETHING ); // Bad, shouldn't use CONSTANTS

Ensure translatable strings, and string domains are not dynamic #136

Ensure translatable strings, and string domains are not dynamic #136

Conversation

shadyvb commented Dec 12, 2013

jrfnl commented Dec 12, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented May 13, 2015

jrfnl commented May 13, 2015

GaryJones commented May 13, 2015

jrfnl commented May 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Mar 14, 2016

Choose a reason for hiding this comment

ocean90 commented Mar 14, 2016

westonruter commented Mar 14, 2016

jrfnl commented Mar 15, 2016

westonruter commented Mar 15, 2016

Choose a reason for hiding this comment

westonruter commented Mar 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Mar 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grappler commented Mar 24, 2016

ocean90 commented Mar 24, 2016

grappler commented Mar 24, 2016