Merge pull request #1856 from ockham/add/i18n-html-tag-sniff

* I18n: Add sniff to detect string wrapped in HTML

Fixes the non-controversial part of #1419 by adding a sniff that detects translated strings wrapped in HTML tags, and a corresponding unit test. Quoting #1419 (comment):

> Examples where the markup _should_ be removed from inside of the string would be:
>
> ```
> <?php __( '<h1>Settings Page</h1>', 'text-domain ); ?>
> should be
> <h1><?php __( 'Settings Page', 'text-domain' ); ?></h1>
> because the markup only wraps the string.
> ```

Note that I had to add code to `I18nUnitTest.php` to reset the text domain as set via `setConfigData()` for one test file, as it would otherwise persist for every test file after that one.

WordPress/Sniffs/WP/I18nSniff.php

@@ -630,11 +630,43 @@ protected function check_text( $context ) {
 		 *
 		 * Strip placeholders and surrounding quotes.
 		 */
-		$non_placeholder_content = trim( $this->strip_quotes( $content ) );
-		$non_placeholder_content = preg_replace( self::SPRINTF_PLACEHOLDER_REGEX, '', $non_placeholder_content );
+		$content_without_surrounding_quotes = trim( $this->strip_quotes( $content ) );
+		$non_placeholder_content = preg_replace( self::SPRINTF_PLACEHOLDER_REGEX, '', $content_without_surrounding_quotes );
 
 		if ( '' === $non_placeholder_content ) {
 			$this->phpcsFile->addError( 'Strings should have translatable content', $stack_ptr, 'NoEmptyStrings' );
+			return;
+		}
+
+		/*
+		 * NoHtmlWrappedStrings
+		 *
+		 * Strip surrounding quotes.
+		 */
+		$reader = new \XMLReader();
+		$reader->XML( $content_without_surrounding_quotes, 'UTF-8', LIBXML_NOERROR | LIBXML_ERR_NONE | LIBXML_NOWARNING );
+
+		// Is the first node an HTML element?
+		if ( ! $reader->read() || \XMLReader::ELEMENT !== $reader->nodeType ) {
+			return;
+		}
+
+		// If the opening HTML element includes placeholders in its attributes, we don't warn.
+		// E.g. '<option id="%1$s" value="%2$s">Translatable option name</option>'.
+		for ( $i = 0; $attr = $reader->getAttributeNo( $i ); $i++ ) {
+			if ( preg_match( self::SPRINTF_PLACEHOLDER_REGEX, $attr ) ) {
+				return;
+			}
+		}
+
+		// We don't flag strings wrapped in `<a href="...">...</a>`, as the link target might actually need localization.
+		if ( 'a' === $reader->name && $reader->getAttribute( 'href' ) ) {
+			return;
+		}
+
+		// Does the entire string only consist of this HTML node?
+		if ( $reader->readOuterXml() === $content_without_surrounding_quotes ) {
+			$this->phpcsFile->addWarning( 'Strings should not be wrapped in HTML', $stack_ptr, 'NoHtmlWrappedStrings' );
 		}
 	}
 

WordPress/Tests/WP/I18nUnitTest.1.inc

@@ -183,5 +183,37 @@ _n_noop($singular); // Bad x 3.
 // This test is needed to verify that the missing plural argument above does not cause an internal error, stopping the run.
 _n_noop( 'I have %d cat.', 'I have %d cats.' ); // Bad.
 
+// HTML wrapped strings.
+__( '<address>123 Fake Street</address>', 'my-slug' ); // Bad, string shouldn't be wrapped in HTML.
+__( 'I live at <address>123 Fake Street</address>', 'my-slug' ); // Okay, no consensus on HTML tags within strings.
+__( '<address>123 Fake Street</address> is my home', 'my-slug' ); // Okay, no consensus on HTML tags within strings.
+__( '<span>Text</span> More text <span>Text</span>', 'my-slug' ); // Good, we're not wrapping
+__( '<div class="my-class">Translatable content</div>', 'my-slug' ); // Bad
+__( '<option id="%1$s" value="%2$s">Translatable option name</option>', 'my-slug' ); // Wrapping is okay, since there are placeholders
+__( '<b><i>Foo</i></b>', 'my-slug' ); // Bad
+__( '<foo>Foo</foo>', 'my-slug' ); // Good
+
+// Strings wrapped in `<a href="...">...</a>` aren't flagged, since the link target might require localization.
+__( '<a href="http://wordpress.org">WordPress</a>', 'my-slug' ); // Good
+__( '<a id="anchor">translatable text</a>', 'my-slug' ) // Bad, since no href
+__( '<a>translatable text</a>', 'my-slug' ) // Bad, since no href
+__( '<a href="%s">translatable text</a>', 'my-slug' ) // Good
+
+// Strings containing malformed XML, to make sure we're able to parse them. No warnings should be thrown.
+__( '<div>text to translate', 'my-slug' );
+__( 'text to <div>translate', 'my-slug' );
+__( 'text < to translate', 'my-slug' );
+__( 'text to > translate', 'my-slug' );
+__( '<div data-type=">foo">my address</div>', 'my-slug' );
+__( '<div>text to translate<div>', 'my-slug' );
+__( '<>text to translate</>', 'my-slug' );
+__( '<>text to translate<>', 'my-slug' );
+__( '<div><div>', 'my-slug' );
+__( '<<>>text to translate<<>', 'my-slug' );
+__( '<<>>123</<>', 'my-slug' );
+__( '<b><i>Foo</b></i>', 'my-slug' );
+__( '<b><i>Foo</b></i></b>', 'my-slug' );
+__( '<foo>Foo</bar>', 'my-slug' );
+
 // phpcs:set WordPress.WP.I18n text_domain[]
 // phpcs:set WordPress.WP.I18n check_translator_comments true

WordPress/Tests/WP/I18nUnitTest.1.inc.fixed

@@ -183,5 +183,37 @@ _n_noop($singular); // Bad x 3.
 // This test is needed to verify that the missing plural argument above does not cause an internal error, stopping the run.
 _n_noop( 'I have %d cat.', 'I have %d cats.' ); // Bad.
 
+// HTML wrapped strings.
+__( '<address>123 Fake Street</address>', 'my-slug' ); // Bad, string shouldn't be wrapped in HTML.
+__( 'I live at <address>123 Fake Street</address>', 'my-slug' ); // Okay, no consensus on HTML tags within strings.
+__( '<address>123 Fake Street</address> is my home', 'my-slug' ); // Okay, no consensus on HTML tags within strings.
+__( '<span>Text</span> More text <span>Text</span>', 'my-slug' ); // Good, we're not wrapping
+__( '<div class="my-class">Translatable content</div>', 'my-slug' ); // Bad
+__( '<option id="%1$s" value="%2$s">Translatable option name</option>', 'my-slug' ); // Wrapping is okay, since there are placeholders
+__( '<b><i>Foo</i></b>', 'my-slug' ); // Bad
+__( '<foo>Foo</foo>', 'my-slug' ); // Good
+
+// Strings wrapped in `<a href="...">...</a>` aren't flagged, since the link target might require localization.
+__( '<a href="http://wordpress.org">WordPress</a>', 'my-slug' ); // Good
+__( '<a id="anchor">translatable text</a>', 'my-slug' ) // Bad, since no href
+__( '<a>translatable text</a>', 'my-slug' ) // Bad, since no href
+__( '<a href="%s">translatable text</a>', 'my-slug' ) // Good
+
+// Strings containing malformed XML, to make sure we're able to parse them. No warnings should be thrown.
+__( '<div>text to translate', 'my-slug' );
+__( 'text to <div>translate', 'my-slug' );
+__( 'text < to translate', 'my-slug' );
+__( 'text to > translate', 'my-slug' );
+__( '<div data-type=">foo">my address</div>', 'my-slug' );
+__( '<div>text to translate<div>', 'my-slug' );
+__( '<>text to translate</>', 'my-slug' );
+__( '<>text to translate<>', 'my-slug' );
+__( '<div><div>', 'my-slug' );
+__( '<<>>text to translate<<>', 'my-slug' );
+__( '<<>>123</<>', 'my-slug' );
+__( '<b><i>Foo</b></i>', 'my-slug' );
+__( '<b><i>Foo</b></i></b>', 'my-slug' );
+__( '<foo>Foo</bar>', 'my-slug' );
+
 // phpcs:set WordPress.WP.I18n text_domain[]
 // phpcs:set WordPress.WP.I18n check_translator_comments true

WordPress/Tests/WP/I18nUnitTest.php

@@ -33,6 +33,9 @@ public function setCliValues( $testFile, $config ) {
 		// Test overruling the text domain from the command line for one test file.
 		if ( 'I18nUnitTest.3.inc' === $testFile ) {
 			$config->setConfigData( 'text_domain', 'something', true );
+		} else {
+			// Delete the text domain option so it doesn't persist for subsequent test files.
+			$config->setConfigData( 'text_domain', null, true );
 		}
 	}
 
@@ -161,6 +164,12 @@ public function getWarningList( $testFile = '' ) {
 					154 => 1,
 					158 => 1,
 					159 => 1,
+					187 => 1,
+					191 => 1,
+					193 => 1,
+					194 => 1,
+					198 => 1,
+					199 => 1,
 				);
 
 			case 'I18nUnitTest.2.inc':