I18n (PHP): Add lint to avoid including HTML tags in a translated string

[ockham]

Is your feature request related to a problem? Please describe.
We discovered this issue through an i18n linter set up on WP.com, whereas it wasn't detected within the Gutenberg repo.

Describe the solution you'd like
I think it'd make sense to include similar linting capabilities to Gutenberg, which we'll ideally run from a commit hook, and CI (either Travis or GH Actions) to avoid cases like that in the future.

I'm not sure if core (or WP.org, more generally) already has a linter like that in place (and if we could share it with the Gutenberg repo)?

Question: Do we have a place for PHP linting rules like this in the Gutenberg repo?

Describe alternatives you've considered
N/A

/cc @akirk @epiqueras @chriskmnds

[epiqueras]

Yes, can you make a PR that adds it to the PHPCS config?

[aduth]

Question: Do we have a place for PHP linting rules like this in the Gutenberg repo?

If the issue is about markup in translated strings, would it be safe to assume that it affects strings both in PHP and in JavaScript?

If so, I think it would be a combination of:

• PHP: Forbidding by PHPCS (as @epiqueras mentioned)
  • Not sure what specifically exists for this, but I observe we have some i18n-related PHPCS rules. It would be worth exploring whether there is an option available here already and/or if this is something where we can make an upstream enhancement to these rules. Maybe @ntwb is familiar with this?
• JavaScript: Forbid by ESLint. If it's specific enough that we don't want to include it as part of the WordPress shared ESLint configuration, it could be included in the top-level .eslintrc.js file. There is even some precedent on how to form these rules specifically impacting translation function usage.

[epiqueras]

Yes we should add this for JS as well. I think it’s generic enough to be enabled for all of WordPress.

[ockham]

Question: Do we have a place for PHP linting rules like this in the Gutenberg repo?

If the issue is about markup in translated strings, would it be safe to assume that it affects strings both in PHP and in JavaScript?

I was mostly thinking of PHP, since these days, I think I kinda equate JS === React, where I don't think anyone would include HTML in a string, but yeah, fair point -- there's non-React JS where that might be the case, so a JS lint rule might make sense as well.

• PHP: Forbidding by PHPCS (as @epiqueras mentioned)
  • Not sure what specifically exists for this, but I observe we have some i18n-related PHPCS rules. It would be worth exploring whether there is an option available here already and/or if this is something where we can make an upstream enhancement to these rules. Maybe @ntwb is familiar with this?
• JavaScript: Forbid by ESLint. If it's specific enough that we don't want to include it as part of the WordPress shared ESLint configuration, it could be included in the top-level .eslintrc.js file. There is even some precedent on how to form these rules specifically impacting translation function usage.

Thanks for the pointers!

I'll try to work on this as soon as I find some time 😅

[aduth]

I think I kinda equate JS === React, where I don't think anyone would include HTML in a string

With #17376, we can probably expect it to be more common. I might hope that the overhead of using createInterpolateElement might be enough of a deterrent to developers to avoid the problem in most cases (if I understand the problem as specifically when a string includes a top-level element wrapper?).

[swissspidy]

Note that in PHP, while HTML in i18n strings is typically avoided, sometimes it is necessary.

[epiqueras]

When?

[ntwb]

The main WPCS repo doesn't include many i18n sniffs

The last I recall I think there were some i18n sniffs in the Automattic or VIP configs, would have to check this

[ockham]

Found an issue in WPCS asking for a sniff like this: WordPress/WordPress-Coding-Standards#1419

Edit: It's apparently not without controversy, but people seem to agree that 'wrapping' HTML tags (__( '<strong>Like this</strong>' )) should be avoided (WordPress/WordPress-Coding-Standards#1419 (comment)).

[aduth]

Could someone summarize what the "issue" is, exactly?

Is it that all HTML markup in translated strings is bad?

Based on:

• @swissspidy's comment at I18n (PHP): Add lint to avoid including HTML tags in a translated string #19911 (comment), @GaryJones comment at Sniff for HTML in translatable strings WordPress-Coding-Standards#1419 (comment)
• The i18n guidelines Best Practices

  • Do not put unnecessary HTML markup [...]

• The very existence of createInterpolateElement (New Feature: Introduce __experimentalCreateInterpolateElement #17376) and historical context at i18n: Implement a solution for interpolation of React elements in strings #9846 serve the purpose to allow markup interpolation
• The "i18n for WordPress" page

  • Including HTML in translatable strings depends on the context. Include HTML if the string is not separated from any text surrounding it

• GNU gettext reference from @GaryJones's earlier comment

  • HTML markup, however, is common enough that it’s probably ok to use in translatable strings.

... I don't think this is what we're wanting out of a rule is to forbid it altogether? Or is it more about forbidding them by default, and force the conscious decision to opt-out of (inline disable) the rule?

Otherwise, I think @ockham identifies the issue as being the specific use of wrapping tags, which is consistent with the "i18n for WordPress" page mentioned above:

Include HTML if the string is not separated from any text surrounding it

So maybe the rule can be:

Forbid translation strings where the string is wrapped in a single pair of HTML opening and closing tags:

• Ok: __( 'Hello world' )
• Ok: __( 'Hello <strong>world</strong>' )
• Bad: __( '<strong>Hello world</strong>' )

This seems to cover what was originally discussed in https://github.com/WordPress/gutenberg/pull/19576/files#r366897189.

[epiqueras]

Yes, that's the rule.

[ockham]

So maybe the rule can be:

Forbid translation strings where the string is wrapped in a single pair of HTML opening and closing tags:

• Ok: __( 'Hello world' )
• Ok: __( 'Hello <strong>world</strong>' )
• Bad: __( '<strong>Hello world</strong>' )

This seems to cover what was originally discussed in https://github.com/WordPress/gutenberg/pull/19576/files#r366897189.

Yeah, was leaning towards that. This seems to be the non-controversial part that would cover the issue that was orignally flagged by the WP.com linter, so it'd make sense to get a rule like that in. (Eventually, it might be extended to cover other cases if people agree on them.)

[ockham]

PR: WordPress/WordPress-Coding-Standards#1856

[ockham]

PR: WordPress/WordPress-Coding-Standards#1856

This has been merged, so I think the next version of WPCS should contain this rule.

Leave this issue open until then? Or until we also have something for the JS counterpart?

[aduth]

Leave this issue open until then? Or until we also have something for the JS counterpart?

We could optionally split it to two issues, but I think as written this should remain open 'til it's addressed in both PHP and JavaScript linting.