search

WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.52k stars 4.21k forks source link

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

Open ockham opened 4 years ago

ockham commented 4 years ago

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 commented 4 years ago

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

aduth commented 4 years ago

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:

epiqueras commented 4 years ago

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

ockham commented 4 years ago

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 :sweat_smile:

aduth commented 4 years ago

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 commented 4 years ago

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

epiqueras commented 4 years ago

When?

ntwb commented 4 years ago

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 commented 4 years ago

Found an issue in WPCS asking for a sniff like this: https://github.com/WordPress/WordPress-Coding-Standards/issues/1419

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

aduth commented 4 years ago

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

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

Based on:

... 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:

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

epiqueras commented 4 years ago

Yes, that's the rule.

ockham commented 4 years ago

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 commented 4 years ago

PR: https://github.com/WordPress/WordPress-Coding-Standards/pull/1856

ockham commented 4 years ago

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 commented 4 years ago

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.