From a43ae6d8c0a1a0b6b7cb294bc92d5c4e4f1b81c1 Mon Sep 17 00:00:00 2001 From: Bernie Reiter Date: Mon, 3 Feb 2020 15:33:17 +0600 Subject: [PATCH 01/24] 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 https://github.com/WordPress/WordPress-Coding-Standards/issues/1419#issuecomment-403797241: > Examples where the markup _should_ be removed from inside of the string would be: > > ``` > Settings Page', 'text-domain ); ?> > should be >

> 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 | 31 +++++++++++++++++++-- WordPress/Tests/WP/I18nUnitTest.1.inc | 8 ++++++ WordPress/Tests/WP/I18nUnitTest.1.inc.fixed | 8 ++++++ WordPress/Tests/WP/I18nUnitTest.php | 5 ++++ 4 files changed, 50 insertions(+), 2 deletions(-) diff --git a/WordPress/Sniffs/WP/I18nSniff.php b/WordPress/Sniffs/WP/I18nSniff.php index d481955368..61e30a6aaf 100644 --- a/WordPress/Sniffs/WP/I18nSniff.php +++ b/WordPress/Sniffs/WP/I18nSniff.php @@ -630,11 +630,38 @@ 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. ''. + for ( $i = 0; $attr = $reader->getAttributeNo( $i ); $i++ ) { + if ( preg_match( self::SPRINTF_PLACEHOLDER_REGEX, $attr ) ) { + return; + } + } + + // Is this all there is? + if ( $reader->readOuterXml() === $content_without_surrounding_quotes ) { + $this->phpcsFile->addWarning( 'Strings should not be wrapped in HTML', $stack_ptr, 'NoHtmlWrappedStrings' ); } } diff --git a/WordPress/Tests/WP/I18nUnitTest.1.inc b/WordPress/Tests/WP/I18nUnitTest.1.inc index ac9dde8eec..bd220a7f5a 100644 --- a/WordPress/Tests/WP/I18nUnitTest.1.inc +++ b/WordPress/Tests/WP/I18nUnitTest.1.inc @@ -183,5 +183,13 @@ _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. +__( '
123 Fake Street
', 'my-slug' ); // Bad, string shouldn't be wrapped in HTML. +__( 'I live at
123 Fake Street
', 'my-slug' ); // Okay, no consensus on HTML tags within strings. +__( '
123 Fake Street
is my home', 'my-slug' ); // Okay, no consensus on HTML tags within strings. +__( 'Text More text Text', 'my-slug' ); // Good, we're not wrapping +__( '
Translatable content
', 'my-slug' ); // Bad +__( '', 'my-slug' ); // Wrapping is okay, since there are placeholders + // phpcs:set WordPress.WP.I18n text_domain[] // phpcs:set WordPress.WP.I18n check_translator_comments true diff --git a/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed b/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed index 6e2cf60622..ba016a8ca8 100644 --- a/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed +++ b/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed @@ -183,5 +183,13 @@ _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. +__( '
123 Fake Street
', 'my-slug' ); // Bad, string shouldn't be wrapped in HTML. +__( 'I live at
123 Fake Street
', 'my-slug' ); // Okay, no consensus on HTML tags within strings. +__( '
123 Fake Street
is my home', 'my-slug' ); // Okay, no consensus on HTML tags within strings. +__( 'Text More text Text', 'my-slug' ); // Good, we're not wrapping +__( '
Translatable content
', 'my-slug' ); // Bad +__( '', 'my-slug' ); // Wrapping is okay, since there are placeholders + // phpcs:set WordPress.WP.I18n text_domain[] // phpcs:set WordPress.WP.I18n check_translator_comments true diff --git a/WordPress/Tests/WP/I18nUnitTest.php b/WordPress/Tests/WP/I18nUnitTest.php index 8a462ef915..374e9f94fc 100644 --- a/WordPress/Tests/WP/I18nUnitTest.php +++ b/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,8 @@ public function getWarningList( $testFile = '' ) { 154 => 1, 158 => 1, 159 => 1, + 187 => 1, + 191 => 1, ); case 'I18nUnitTest.2.inc': From 90f34dbf7b888f3ae468c656fc877e5ad7e7300d Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 1 Feb 2020 14:29:18 +0100 Subject: [PATCH 02/24] Travis: various tweaks * Run against the highest stable PHP version for the sniff/rulesets build (+ the `dev-master` `quicktest` build). * Only run `composer validate` on the `sniff` stage. The `composer.json` file will be validated on the install for the all builds anyway. This just adds the `strict` checking. * Add an extra `test` build against PHPCS 4.x for which development has just started. This build is allowed to fail for now. * Remove the DealerDirect plugin and PHPCompatibility for the build against `nightly` and the PHPCS 4.x build. Neither are needed anyway as a `--no-dev` install is done for the `test` stages and the `installed_paths` is set directly. The DealerDirect plugin currently won't install on PHP 8.x, so the build would fail on "failure to install" before doing anything. And both PHPCompatibility as well as the DealerDirect plugin won't install/will block `--no-dev` installs with PHPCS 4.x. This way, the linting and unit tests should still run on `nightly` and with PHPCS 4.x. --- .travis.yml | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/.travis.yml b/.travis.yml index bbd5417579..5343be7249 100644 --- a/.travis.yml +++ b/.travis.yml @@ -45,7 +45,7 @@ jobs: include: #### SNIFF STAGE #### - stage: sniff - php: 7.3 + php: 7.4 env: PHPCS_BRANCH="dev-master" addons: apt: @@ -70,12 +70,16 @@ jobs: - diff -B --tabsize=4 ./WordPress-Extra/ruleset.xml <(xmllint --format "./WordPress-Extra/ruleset.xml") - diff -B --tabsize=4 ./phpcs.xml.dist.sample <(xmllint --format "./phpcs.xml.dist.sample") + # Validate the composer.json file. + # @link https://getcomposer.org/doc/03-cli.md#validate + - composer validate --no-check-all --strict + #### RULESET STAGE #### # Make sure the rulesets don't throw unexpected errors or warnings. # This check needs to be run against a high PHP version to prevent triggering the syntax error check. # It also needs to be run against all PHPCS versions WPCS is tested against. - stage: rulesets - php: 7.3 + php: 7.4 env: PHPCS_BRANCH="dev-master" script: - $(pwd)/vendor/bin/phpcs -ps ./bin/class-ruleset-test.php --standard=WordPress-Core @@ -91,7 +95,7 @@ jobs: - travis_retry $(pwd)/vendor/bin/phpcbf -pq ./WordPress/Tests/ --standard=WordPress --extensions=inc --exclude=Generic.PHP.Syntax --report=summary - stage: rulesets - php: 7.3 + php: 7.4 env: PHPCS_BRANCH="3.3.1" script: - $(pwd)/vendor/bin/phpcs -ps ./bin/class-ruleset-test.php --standard=WordPress-Core @@ -103,7 +107,7 @@ jobs: # This is a much quicker test which only runs the unit tests and linting against the low/high # supported PHP/PHPCS combinations. - stage: quicktest - php: 7.3 + php: 7.4 env: PHPCS_BRANCH="dev-master" LINT=1 - php: 7.3 env: PHPCS_BRANCH="3.3.1" @@ -112,9 +116,16 @@ jobs: - php: 5.4 env: PHPCS_BRANCH="3.3.1" + #### TEST STAGE #### + # Add extra build to test against PHPCS 4. + - stage: test + php: 7.4 + env: PHPCS_BRANCH="4.0.x-dev" + allow_failures: # Allow failures for unstable builds. - php: "nightly" + - env: PHPCS_BRANCH="4.0.x-dev" before_install: # Speed up build time by disabling Xdebug. @@ -131,6 +142,13 @@ before_install: - export XMLLINT_INDENT=" " - export PHPUNIT_DIR=/tmp/phpunit + - | + if [[ $TRAVIS_PHP_VERSION == "nightly" || $PHPCS_BRANCH == "4.0.x-dev" ]]; then + # Even though we're not doing a dev install, dev requirements are still checked. + # Neither the PHPCS Composer plugin nor PHPCompatibility allows yet for PHPCS 4.x. + # The Composer plugin also doesn't allow for installation on PHP 8.x/nightly. + composer remove --dev dealerdirect/phpcodesniffer-composer-installer phpcompatibility/php-compatibility --no-update --no-scripts + fi - composer require squizlabs/php_codesniffer:${PHPCS_BRANCH} --update-no-dev --no-suggest --no-scripts - | if [[ "$TRAVIS_BUILD_STAGE_NAME" == "Sniff" ]]; then @@ -148,10 +166,6 @@ script: # Lint the PHP files against parse errors. - if [[ "$LINT" == "1" ]]; then if find . -path ./vendor -prune -o -path ./bin -prune -o -name "*.php" -exec php -l {} \; | grep "^[Parse error|Fatal error]"; then exit 1; fi; fi - # Validate the composer.json file. - # @link https://getcomposer.org/doc/03-cli.md#validate - - if [[ "$LINT" == "1" ]]; then composer validate --no-check-all --strict; fi - # Run the unit tests. - | if [[ ${TRAVIS_PHP_VERSION:0:3} > "7.1" ]]; then From 63ae7213d31f37c9d52e4516141fb7c060832c68 Mon Sep 17 00:00:00 2001 From: Bernie Reiter Date: Fri, 7 Feb 2020 12:32:16 +0600 Subject: [PATCH 03/24] Add strings containing malformed XML --- WordPress/Tests/WP/I18nUnitTest.1.inc | 6 ++++++ WordPress/Tests/WP/I18nUnitTest.1.inc.fixed | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/WordPress/Tests/WP/I18nUnitTest.1.inc b/WordPress/Tests/WP/I18nUnitTest.1.inc index bd220a7f5a..c362ea800a 100644 --- a/WordPress/Tests/WP/I18nUnitTest.1.inc +++ b/WordPress/Tests/WP/I18nUnitTest.1.inc @@ -191,5 +191,11 @@ __( 'Text More text Text', 'my-slug' ); // Good, we're __( '
Translatable content
', 'my-slug' ); // Bad __( '', 'my-slug' ); // Wrapping is okay, since there are placeholders +// Strings containing malformed XML, to make sure we're able to parse them. No warnings should be thrown. +__( '
text to translate', 'my-slug' ); +__( 'text to
translate', 'my-slug' ); +__( 'text < to translate', 'my-slug' ); +__( 'text to > translate', 'my-slug' ); + // phpcs:set WordPress.WP.I18n text_domain[] // phpcs:set WordPress.WP.I18n check_translator_comments true diff --git a/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed b/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed index ba016a8ca8..4696fb9bb9 100644 --- a/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed +++ b/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed @@ -191,5 +191,11 @@ __( 'Text More text Text', 'my-slug' ); // Good, we're __( '
Translatable content
', 'my-slug' ); // Bad __( '', 'my-slug' ); // Wrapping is okay, since there are placeholders +// Strings containing malformed XML, to make sure we're able to parse them. No warnings should be thrown. +__( '
text to translate', 'my-slug' ); +__( 'text to
translate', 'my-slug' ); +__( 'text < to translate', 'my-slug' ); +__( 'text to > translate', 'my-slug' ); + // phpcs:set WordPress.WP.I18n text_domain[] // phpcs:set WordPress.WP.I18n check_translator_comments true From 20b87f2a28dcf485e6e41cc6b67c6c1abb539b53 Mon Sep 17 00:00:00 2001 From: Bernie Reiter Date: Fri, 7 Feb 2020 12:35:11 +0600 Subject: [PATCH 04/24] More weird XML --- WordPress/Tests/WP/I18nUnitTest.1.inc | 1 + WordPress/Tests/WP/I18nUnitTest.1.inc.fixed | 1 + 2 files changed, 2 insertions(+) diff --git a/WordPress/Tests/WP/I18nUnitTest.1.inc b/WordPress/Tests/WP/I18nUnitTest.1.inc index c362ea800a..bc5052bee7 100644 --- a/WordPress/Tests/WP/I18nUnitTest.1.inc +++ b/WordPress/Tests/WP/I18nUnitTest.1.inc @@ -196,6 +196,7 @@ __( '
text to translate', 'my-slug' ); __( 'text to
translate', 'my-slug' ); __( 'text < to translate', 'my-slug' ); __( 'text to > translate', 'my-slug' ); +__( '
my address
', 'my-slug' ); // phpcs:set WordPress.WP.I18n text_domain[] // phpcs:set WordPress.WP.I18n check_translator_comments true diff --git a/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed b/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed index 4696fb9bb9..73210ca0c1 100644 --- a/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed +++ b/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed @@ -196,6 +196,7 @@ __( '
text to translate', 'my-slug' ); __( 'text to
translate', 'my-slug' ); __( 'text < to translate', 'my-slug' ); __( 'text to > translate', 'my-slug' ); +__( '
my address
', 'my-slug' ); // phpcs:set WordPress.WP.I18n text_domain[] // phpcs:set WordPress.WP.I18n check_translator_comments true From 51f9f6eaca89e19df85bf65c93805f229c515da4 Mon Sep 17 00:00:00 2001 From: Bernie Reiter Date: Fri, 7 Feb 2020 13:09:57 +0600 Subject: [PATCH 05/24] More weirdness --- WordPress/Tests/WP/I18nUnitTest.1.inc | 6 ++++++ WordPress/Tests/WP/I18nUnitTest.1.inc.fixed | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/WordPress/Tests/WP/I18nUnitTest.1.inc b/WordPress/Tests/WP/I18nUnitTest.1.inc index bc5052bee7..d0fc1d98ac 100644 --- a/WordPress/Tests/WP/I18nUnitTest.1.inc +++ b/WordPress/Tests/WP/I18nUnitTest.1.inc @@ -197,6 +197,12 @@ __( 'text to
translate', 'my-slug' ); __( 'text < to translate', 'my-slug' ); __( 'text to > translate', 'my-slug' ); __( '
my address
', 'my-slug' ); +__( '
text to translate
', 'my-slug' ); +__( '<>text to translate', 'my-slug' ); +__( '<>text to translate<>', 'my-slug' ); +__( '
', 'my-slug' ); +__( '<<>>text to translate<<>', 'my-slug' ); +__( '<<>>123', 'my-slug' ); // phpcs:set WordPress.WP.I18n text_domain[] // phpcs:set WordPress.WP.I18n check_translator_comments true diff --git a/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed b/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed index 73210ca0c1..bc75786434 100644 --- a/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed +++ b/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed @@ -197,6 +197,12 @@ __( 'text to
translate', 'my-slug' ); __( 'text < to translate', 'my-slug' ); __( 'text to > translate', 'my-slug' ); __( '
my address
', 'my-slug' ); +__( '
text to translate
', 'my-slug' ); +__( '<>text to translate', 'my-slug' ); +__( '<>text to translate<>', 'my-slug' ); +__( '
', 'my-slug' ); +__( '<<>>text to translate<<>', 'my-slug' ); +__( '<<>>123', 'my-slug' ); // phpcs:set WordPress.WP.I18n text_domain[] // phpcs:set WordPress.WP.I18n check_translator_comments true From 1294413a2c59e5c170a0e2666c29bd45415fc2a5 Mon Sep 17 00:00:00 2001 From: Bernie Reiter Date: Fri, 7 Feb 2020 13:33:29 +0600 Subject: [PATCH 06/24] Add NoAHrefWrappedStrings rule --- WordPress/Sniffs/WP/I18nSniff.php | 12 ++++++++++-- WordPress/Tests/WP/I18nUnitTest.1.inc | 7 +++++++ WordPress/Tests/WP/I18nUnitTest.1.inc.fixed | 7 +++++++ WordPress/Tests/WP/I18nUnitTest.php | 1 + 4 files changed, 25 insertions(+), 2 deletions(-) diff --git a/WordPress/Sniffs/WP/I18nSniff.php b/WordPress/Sniffs/WP/I18nSniff.php index 61e30a6aaf..9688ddd847 100644 --- a/WordPress/Sniffs/WP/I18nSniff.php +++ b/WordPress/Sniffs/WP/I18nSniff.php @@ -639,7 +639,7 @@ protected function check_text( $context ) { } /* - * NoHtmlWrappedStrings. + * NoHtmlWrappedStrings and NoAHrefWrappedStrings. * * Strip surrounding quotes. */ @@ -659,7 +659,15 @@ protected function check_text( $context ) { } } - // Is this all there is? + // Dedicated rule for cases where strings are wrapped in '...'. + // The link target might actually need localization. + // Using a separate rule here allows codebases to disable it independently of the NoHtmlWrappedStrings rule. + if ( $reader->name === 'a' && $reader->getAttribute( 'href' ) ) { + $this->phpcsFile->addWarning( 'Strings should not be wrapped in ...', $stack_ptr, 'NoAHrefWrappedStrings' ); + 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' ); } diff --git a/WordPress/Tests/WP/I18nUnitTest.1.inc b/WordPress/Tests/WP/I18nUnitTest.1.inc index d0fc1d98ac..ac9ab89d1f 100644 --- a/WordPress/Tests/WP/I18nUnitTest.1.inc +++ b/WordPress/Tests/WP/I18nUnitTest.1.inc @@ -204,5 +204,12 @@ __( '
', 'my-slug' ); __( '<<>>text to translate<<>', 'my-slug' ); __( '<<>>123', 'my-slug' ); +// Strings wrapped in '...'. These get a different warning, as the link target might actually be localized. +// phpcs:disable WordPress.WP.I18n.NoHtmlWrappedStrings +__( 'WordPress', 'my-slug' ); // Bad, strings shouldn't be wrapped in ... +__( 'translatable text', 'my-slug' ) // Good, since we have disabled the NoHtmlWrappedStrings rule here +__( 'translatable text', 'my-slug' ) // Good, since we have disabled the NoHtmlWrappedStrings rule here +// phpcs:enable WordPress.WP.I18n.NoHtmlWrappedStrings + // phpcs:set WordPress.WP.I18n text_domain[] // phpcs:set WordPress.WP.I18n check_translator_comments true diff --git a/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed b/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed index bc75786434..6319d35492 100644 --- a/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed +++ b/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed @@ -204,5 +204,12 @@ __( '
', 'my-slug' ); __( '<<>>text to translate<<>', 'my-slug' ); __( '<<>>123', 'my-slug' ); +// Strings wrapped in '...'. These get a different warning, as the link target might actually be localized. +// phpcs:disable WordPress.WP.I18n.NoHtmlWrappedStrings +__( 'WordPress', 'my-slug' ); // Bad, strings shouldn't be wrapped in ... +__( 'translatable text', 'my-slug' ) // Good, since we have disabled the NoHtmlWrappedStrings rule here +__( 'translatable text', 'my-slug' ) // Good, since we have disabled the NoHtmlWrappedStrings rule here +// phpcs:enable WordPress.WP.I18n.NoHtmlWrappedStrings + // phpcs:set WordPress.WP.I18n text_domain[] // phpcs:set WordPress.WP.I18n check_translator_comments true diff --git a/WordPress/Tests/WP/I18nUnitTest.php b/WordPress/Tests/WP/I18nUnitTest.php index 374e9f94fc..629601dd72 100644 --- a/WordPress/Tests/WP/I18nUnitTest.php +++ b/WordPress/Tests/WP/I18nUnitTest.php @@ -166,6 +166,7 @@ public function getWarningList( $testFile = '' ) { 159 => 1, 187 => 1, 191 => 1, + 209 => 1, ); case 'I18nUnitTest.2.inc': From cf3fcb300247c30409ab7a59fc44216ef18f4c0b Mon Sep 17 00:00:00 2001 From: Bernie Reiter Date: Fri, 7 Feb 2020 13:43:07 +0600 Subject: [PATCH 07/24] Yoda --- WordPress/Sniffs/WP/I18nSniff.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WordPress/Sniffs/WP/I18nSniff.php b/WordPress/Sniffs/WP/I18nSniff.php index 9688ddd847..9b7e7457cd 100644 --- a/WordPress/Sniffs/WP/I18nSniff.php +++ b/WordPress/Sniffs/WP/I18nSniff.php @@ -662,7 +662,7 @@ protected function check_text( $context ) { // Dedicated rule for cases where strings are wrapped in '...'. // The link target might actually need localization. // Using a separate rule here allows codebases to disable it independently of the NoHtmlWrappedStrings rule. - if ( $reader->name === 'a' && $reader->getAttribute( 'href' ) ) { + if ( 'a' === $reader->name && $reader->getAttribute( 'href' ) ) { $this->phpcsFile->addWarning( 'Strings should not be wrapped in ...', $stack_ptr, 'NoAHrefWrappedStrings' ); return; } From 70f903213df74baa5cf9542ef06c089776ec324a Mon Sep 17 00:00:00 2001 From: Bernie Reiter Date: Fri, 7 Feb 2020 14:51:32 +0600 Subject: [PATCH 08/24] Add another test for the a href rule, fix some comments --- WordPress/Tests/WP/I18nUnitTest.1.inc | 7 ++++--- WordPress/Tests/WP/I18nUnitTest.1.inc.fixed | 7 ++++--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/WordPress/Tests/WP/I18nUnitTest.1.inc b/WordPress/Tests/WP/I18nUnitTest.1.inc index ac9ab89d1f..97a3db76fd 100644 --- a/WordPress/Tests/WP/I18nUnitTest.1.inc +++ b/WordPress/Tests/WP/I18nUnitTest.1.inc @@ -206,9 +206,10 @@ __( '<<>>123', 'my-slug' ); // Strings wrapped in '...'. These get a different warning, as the link target might actually be localized. // phpcs:disable WordPress.WP.I18n.NoHtmlWrappedStrings -__( 'WordPress', 'my-slug' ); // Bad, strings shouldn't be wrapped in ... -__( 'translatable text', 'my-slug' ) // Good, since we have disabled the NoHtmlWrappedStrings rule here -__( 'translatable text', 'my-slug' ) // Good, since we have disabled the NoHtmlWrappedStrings rule here +__( 'WordPress', 'my-slug' ); // Bad, strings shouldn't be wrapped in .... +__( 'translatable text', 'my-slug' ) // Good, since we have disabled the NoHtmlWrappedStrings rule here. +__( 'translatable text', 'my-slug' ) // Good, since we have disabled the NoHtmlWrappedStrings rule here. +__( 'translatable text', 'my-slug' ) // Good, contains a placeholder in the href attribute. // phpcs:enable WordPress.WP.I18n.NoHtmlWrappedStrings // phpcs:set WordPress.WP.I18n text_domain[] diff --git a/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed b/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed index 6319d35492..4a539a5dff 100644 --- a/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed +++ b/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed @@ -206,9 +206,10 @@ __( '<<>>123', 'my-slug' ); // Strings wrapped in '...'. These get a different warning, as the link target might actually be localized. // phpcs:disable WordPress.WP.I18n.NoHtmlWrappedStrings -__( 'WordPress', 'my-slug' ); // Bad, strings shouldn't be wrapped in ... -__( 'translatable text', 'my-slug' ) // Good, since we have disabled the NoHtmlWrappedStrings rule here -__( 'translatable text', 'my-slug' ) // Good, since we have disabled the NoHtmlWrappedStrings rule here +__( 'WordPress', 'my-slug' ); // Bad, strings shouldn't be wrapped in .... +__( 'translatable text', 'my-slug' ) // Good, since we have disabled the NoHtmlWrappedStrings rule here. +__( 'translatable text', 'my-slug' ) // Good, since we have disabled the NoHtmlWrappedStrings rule here. +__( 'translatable text', 'my-slug' ) // Good, contains a placeholder in the href attribute. // phpcs:enable WordPress.WP.I18n.NoHtmlWrappedStrings // phpcs:set WordPress.WP.I18n text_domain[] From 94a0e6b7cb0548037b6c3ad1ee39466a67b849c9 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 7 Feb 2020 15:31:10 +0100 Subject: [PATCH 09/24] AlternativeFunctions: use `whitelist` instead of special case The `AbstractFunctionRestrictions` allow for a `whitelist` of functions which shouldn't be matched when a wildcard is used. We may as well use it as it will bow out earlier than special casing the function in the `switch`. --- WordPress/Sniffs/WP/AlternativeFunctionsSniff.php | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/WordPress/Sniffs/WP/AlternativeFunctionsSniff.php b/WordPress/Sniffs/WP/AlternativeFunctionsSniff.php index 43ad575cd3..fc6e8fbbb3 100644 --- a/WordPress/Sniffs/WP/AlternativeFunctionsSniff.php +++ b/WordPress/Sniffs/WP/AlternativeFunctionsSniff.php @@ -89,6 +89,9 @@ public function getGroups() { 'functions' => array( 'curl_*', ), + 'whitelist' => array( + 'curl_version' => true, + ), ), 'parse_url' => array( @@ -273,10 +276,6 @@ public function process_matched_token( $stackPtr, $group_name, $matched_content unset( $first_param ); break; - - case 'curl_version': - // Curl version doesn't actually create a connection. - return; } if ( ! isset( $this->groups[ $group_name ]['since'] ) ) { From fa94eed5628855da211b9f626ef535e017022806 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 7 Feb 2020 15:36:00 +0100 Subject: [PATCH 10/24] AbstractFunctionRestrictions: improve matching of function calls The code as it was could inadvertently match a CONSTANT with the same name as a function. This has been fixed by adding a check for an open parenthesis after the function name. Adding that check, however, would break the check on `use function` import statements. So some additional code has been added to make sure those will still be matched too. Includes tightening up the regex pattern. Includes unit tests via the `WP.AlternativeFunctions` sniff. --- .../AbstractFunctionRestrictionsSniff.php | 59 ++++++++++++------- .../Tests/WP/AlternativeFunctionsUnitTest.inc | 7 +++ .../Tests/WP/AlternativeFunctionsUnitTest.php | 1 + 3 files changed, 46 insertions(+), 21 deletions(-) diff --git a/WordPress/AbstractFunctionRestrictionsSniff.php b/WordPress/AbstractFunctionRestrictionsSniff.php index e208c90ac1..53ea9a255e 100644 --- a/WordPress/AbstractFunctionRestrictionsSniff.php +++ b/WordPress/AbstractFunctionRestrictionsSniff.php @@ -57,7 +57,7 @@ abstract class AbstractFunctionRestrictionsSniff extends Sniff { * * @var string */ - protected $regex_pattern = '`\b(?:%s)\b`i'; + protected $regex_pattern = '`^(?:%s)$`i'; /** * Cache for the group information. @@ -212,35 +212,52 @@ public function process_token( $stackPtr ) { */ public function is_targetted_token( $stackPtr ) { - // Exclude function definitions, class methods, and namespaced calls. - if ( \T_STRING === $this->tokens[ $stackPtr ]['code'] ) { - if ( $this->is_class_object_call( $stackPtr ) === true ) { - return false; - } + if ( \T_STRING !== $this->tokens[ $stackPtr ]['code'] ) { + return false; + } - if ( $this->is_token_namespaced( $stackPtr ) === true ) { - return false; - } + // Exclude function definitions, class methods, and namespaced calls. + if ( $this->is_class_object_call( $stackPtr ) === true ) { + return false; + } - $prev = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $stackPtr - 1 ), null, true ); + if ( $this->is_token_namespaced( $stackPtr ) === true ) { + return false; + } - if ( false !== $prev ) { - // Skip sniffing if calling a same-named method, or on function definitions. - $skipped = array( - \T_FUNCTION => \T_FUNCTION, - \T_CLASS => \T_CLASS, - \T_AS => \T_AS, // Use declaration alias. - ); + $prev = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $stackPtr - 1 ), null, true ); + if ( false !== $prev ) { + // Skip sniffing on function, class definitions or for function aliases in use statements. + $skipped = array( + \T_FUNCTION => \T_FUNCTION, + \T_CLASS => \T_CLASS, + \T_AS => \T_AS, // Use declaration alias. + ); - if ( isset( $skipped[ $this->tokens[ $prev ]['code'] ] ) ) { - return false; - } + if ( isset( $skipped[ $this->tokens[ $prev ]['code'] ] ) ) { + return false; } + } + + // Check if this could even be a function call. + $next = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true ); + if ( false === $next ) { + return false; + } + // Check for `use function ... (as|;)`. + if ( ( \T_STRING === $this->tokens[ $prev ]['code'] && 'function' === $this->tokens[ $prev ]['content'] ) + && ( \T_AS === $this->tokens[ $next ]['code'] || \T_SEMICOLON === $this->tokens[ $next ]['code'] ) + ) { return true; } - return false; + // If it's not a `use` statement, there should be parenthesis. + if ( \T_OPEN_PARENTHESIS !== $this->tokens[ $next ]['code'] ) { + return false; + } + + return true; } /** diff --git a/WordPress/Tests/WP/AlternativeFunctionsUnitTest.inc b/WordPress/Tests/WP/AlternativeFunctionsUnitTest.inc index 7bfaf9b14d..d819f25a11 100644 --- a/WordPress/Tests/WP/AlternativeFunctionsUnitTest.inc +++ b/WordPress/Tests/WP/AlternativeFunctionsUnitTest.inc @@ -71,3 +71,10 @@ curl_version(); // OK. // Safeguard that additional logic uses case-insensitive function name check. Strip_Tags( $something ); // Warning. + +if ( ! $curl['features'] && CURL_VERSION_SSL ) {} // OK. +my_parse_url_function(); // OK. +function curl_version_ssl() {} // OK. +use function curl_version; // OK. +use function something as curl_version; // OK. +use function curl_init as curl_version; // Bad. diff --git a/WordPress/Tests/WP/AlternativeFunctionsUnitTest.php b/WordPress/Tests/WP/AlternativeFunctionsUnitTest.php index f25dec8cd3..ca95672572 100644 --- a/WordPress/Tests/WP/AlternativeFunctionsUnitTest.php +++ b/WordPress/Tests/WP/AlternativeFunctionsUnitTest.php @@ -64,6 +64,7 @@ public function getWarningList() { 67 => 1, 68 => 1, 73 => 1, + 80 => 1, ); } From 1c843e251a1607d90f7ecfd64c8757b5a050674c Mon Sep 17 00:00:00 2001 From: jrfnl Date: Tue, 13 Aug 2019 15:11:23 +0200 Subject: [PATCH 11/24] Travis/QA: always check that all sniffs are feature complete The new `phpcsstandards/phpcsdevtools` package includes a script which can check whether sniffs are feature complete, i.e. whether all sniffs have unit tests and documentation. By adding this check to the Travis script, we prevent untested and/or undocumented sniffs from entering the repo. For now, the documentation check is silenced. P.S.: the `PHPCSDevTools` package contains a few more goodies, have a look at the [readme](https://github.com/PHPCSStandards/PHPCSDevTools) for more information. --- .travis.yml | 5 +++++ composer.json | 9 ++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 5343be7249..7c45e6d2db 100644 --- a/.travis.yml +++ b/.travis.yml @@ -74,6 +74,11 @@ jobs: # @link https://getcomposer.org/doc/03-cli.md#validate - composer validate --no-check-all --strict + # Check that the sniffs available are feature complete. + # For now, just check that all sniffs have unit tests. + # At a later stage the documentation check can be activated. + - composer check-complete + #### RULESET STAGE #### # Make sure the rulesets don't throw unexpected errors or warnings. # This check needs to be run against a high PHP version to prevent triggering the syntax error check. diff --git a/composer.json b/composer.json index 13d4afbc3d..7e3c558fb3 100644 --- a/composer.json +++ b/composer.json @@ -21,7 +21,8 @@ "require-dev": { "dealerdirect/phpcodesniffer-composer-installer": "^0.5 || ^0.6", "phpcompatibility/php-compatibility": "^9.0", - "phpunit/phpunit": "^4.0 || ^5.0 || ^6.0 || ^7.0" + "phpunit/phpunit": "^4.0 || ^5.0 || ^6.0 || ^7.0", + "phpcsstandards/phpcsdevtools": "^1.0" }, "suggest": { "dealerdirect/phpcodesniffer-composer-installer": "^0.6 || This Composer plugin will sort out the PHPCS 'installed_paths' automatically." @@ -39,6 +40,12 @@ ], "run-tests": [ "@php ./vendor/phpunit/phpunit/phpunit --filter WordPress --bootstrap=\"./vendor/squizlabs/php_codesniffer/tests/bootstrap.php\" ./vendor/squizlabs/php_codesniffer/tests/AllTests.php" + ], + "check-complete": [ + "@php ./vendor/phpcsstandards/phpcsdevtools/bin/phpcs-check-feature-completeness -q ./WordPress" + ], + "check-complete-strict": [ + "@php ./vendor/phpcsstandards/phpcsdevtools/bin/phpcs-check-feature-completeness ./WordPress" ] }, "support": { From c56f18a91a6cc779f7e8fe35cd8b9d5e87505238 Mon Sep 17 00:00:00 2001 From: Bernie Reiter Date: Tue, 18 Feb 2020 00:21:24 +0600 Subject: [PATCH 12/24] Add more tests --- WordPress/Tests/WP/I18nUnitTest.1.inc | 5 +++++ WordPress/Tests/WP/I18nUnitTest.1.inc.fixed | 5 +++++ WordPress/Tests/WP/I18nUnitTest.php | 4 +++- 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/WordPress/Tests/WP/I18nUnitTest.1.inc b/WordPress/Tests/WP/I18nUnitTest.1.inc index 97a3db76fd..cad46a9ae2 100644 --- a/WordPress/Tests/WP/I18nUnitTest.1.inc +++ b/WordPress/Tests/WP/I18nUnitTest.1.inc @@ -190,6 +190,8 @@ __( '
123 Fake Street
is my home', 'my-slug' ); // Okay, no co __( 'Text More text Text', 'my-slug' ); // Good, we're not wrapping __( '
Translatable content
', 'my-slug' ); // Bad __( '', 'my-slug' ); // Wrapping is okay, since there are placeholders +__( 'Foo', 'my-slug' ); // Bad +__( 'Foo', 'my-slug' ); // Good // Strings containing malformed XML, to make sure we're able to parse them. No warnings should be thrown. __( '
text to translate', 'my-slug' ); @@ -203,6 +205,9 @@ __( '<>text to translate<>', 'my-slug' ); __( '
', 'my-slug' ); __( '<<>>text to translate<<>', 'my-slug' ); __( '<<>>123', 'my-slug' ); +__( 'Foo', 'my-slug' ); +__( 'Foo', 'my-slug' ); +__( 'Foo', 'my-slug' ); // Strings wrapped in '...'. These get a different warning, as the link target might actually be localized. // phpcs:disable WordPress.WP.I18n.NoHtmlWrappedStrings diff --git a/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed b/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed index 4a539a5dff..d7d93b8274 100644 --- a/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed +++ b/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed @@ -190,6 +190,8 @@ __( '
123 Fake Street
is my home', 'my-slug' ); // Okay, no co __( 'Text More text Text', 'my-slug' ); // Good, we're not wrapping __( '
Translatable content
', 'my-slug' ); // Bad __( '', 'my-slug' ); // Wrapping is okay, since there are placeholders +__( 'Foo', 'my-slug' ); // Bad +__( 'Foo', 'my-slug' ); // Good // Strings containing malformed XML, to make sure we're able to parse them. No warnings should be thrown. __( '
text to translate', 'my-slug' ); @@ -203,6 +205,9 @@ __( '<>text to translate<>', 'my-slug' ); __( '
', 'my-slug' ); __( '<<>>text to translate<<>', 'my-slug' ); __( '<<>>123', 'my-slug' ); +__( 'Foo', 'my-slug' ); +__( 'Foo', 'my-slug' ); +__( 'Foo', 'my-slug' ); // Strings wrapped in '...'. These get a different warning, as the link target might actually be localized. // phpcs:disable WordPress.WP.I18n.NoHtmlWrappedStrings diff --git a/WordPress/Tests/WP/I18nUnitTest.php b/WordPress/Tests/WP/I18nUnitTest.php index 629601dd72..503c46f3b7 100644 --- a/WordPress/Tests/WP/I18nUnitTest.php +++ b/WordPress/Tests/WP/I18nUnitTest.php @@ -166,7 +166,9 @@ public function getWarningList( $testFile = '' ) { 159 => 1, 187 => 1, 191 => 1, - 209 => 1, + 193 => 1, + 194 => 1, + 214 => 1, ); case 'I18nUnitTest.2.inc': From 005c198ede3da5fc92b352fec62bdcaae47e5a37 Mon Sep 17 00:00:00 2001 From: Bernie Reiter Date: Wed, 19 Feb 2020 14:11:09 +0600 Subject: [PATCH 13/24] Remove NoAHrefWrappedStrings warning --- WordPress/Sniffs/WP/I18nSniff.php | 7 ++----- WordPress/Tests/WP/I18nUnitTest.1.inc | 10 ++++------ WordPress/Tests/WP/I18nUnitTest.1.inc.fixed | 10 ++++------ WordPress/Tests/WP/I18nUnitTest.php | 1 + 4 files changed, 11 insertions(+), 17 deletions(-) diff --git a/WordPress/Sniffs/WP/I18nSniff.php b/WordPress/Sniffs/WP/I18nSniff.php index 9b7e7457cd..27bf95470d 100644 --- a/WordPress/Sniffs/WP/I18nSniff.php +++ b/WordPress/Sniffs/WP/I18nSniff.php @@ -639,7 +639,7 @@ protected function check_text( $context ) { } /* - * NoHtmlWrappedStrings and NoAHrefWrappedStrings. + * NoHtmlWrappedStrings * * Strip surrounding quotes. */ @@ -659,11 +659,8 @@ protected function check_text( $context ) { } } - // Dedicated rule for cases where strings are wrapped in '...'. - // The link target might actually need localization. - // Using a separate rule here allows codebases to disable it independently of the NoHtmlWrappedStrings rule. + // We don't flag strings wrapped in `...`, as the link target might actually need localization. if ( 'a' === $reader->name && $reader->getAttribute( 'href' ) ) { - $this->phpcsFile->addWarning( 'Strings should not be wrapped in ...', $stack_ptr, 'NoAHrefWrappedStrings' ); return; } diff --git a/WordPress/Tests/WP/I18nUnitTest.1.inc b/WordPress/Tests/WP/I18nUnitTest.1.inc index cad46a9ae2..60d7345f12 100644 --- a/WordPress/Tests/WP/I18nUnitTest.1.inc +++ b/WordPress/Tests/WP/I18nUnitTest.1.inc @@ -209,13 +209,11 @@ __( 'Foo', 'my-slug' ); __( 'Foo', 'my-slug' ); __( 'Foo', 'my-slug' ); -// Strings wrapped in '...'. These get a different warning, as the link target might actually be localized. -// phpcs:disable WordPress.WP.I18n.NoHtmlWrappedStrings -__( 'WordPress', 'my-slug' ); // Bad, strings shouldn't be wrapped in .... -__( 'translatable text', 'my-slug' ) // Good, since we have disabled the NoHtmlWrappedStrings rule here. -__( 'translatable text', 'my-slug' ) // Good, since we have disabled the NoHtmlWrappedStrings rule here. +// Strings wrapped in `...` aren't flagged, since the link target might require localization. +__( 'WordPress', 'my-slug' ); // Good +__( 'translatable text', 'my-slug' ) // Bad, since no href. +__( 'translatable text', 'my-slug' ) // Bad, since no href. __( 'translatable text', 'my-slug' ) // Good, contains a placeholder in the href attribute. -// phpcs:enable WordPress.WP.I18n.NoHtmlWrappedStrings // phpcs:set WordPress.WP.I18n text_domain[] // phpcs:set WordPress.WP.I18n check_translator_comments true diff --git a/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed b/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed index d7d93b8274..e7525ec7ad 100644 --- a/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed +++ b/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed @@ -209,13 +209,11 @@ __( 'Foo', 'my-slug' ); __( 'Foo', 'my-slug' ); __( 'Foo', 'my-slug' ); -// Strings wrapped in '...'. These get a different warning, as the link target might actually be localized. -// phpcs:disable WordPress.WP.I18n.NoHtmlWrappedStrings -__( 'WordPress', 'my-slug' ); // Bad, strings shouldn't be wrapped in .... -__( 'translatable text', 'my-slug' ) // Good, since we have disabled the NoHtmlWrappedStrings rule here. -__( 'translatable text', 'my-slug' ) // Good, since we have disabled the NoHtmlWrappedStrings rule here. +// Strings wrapped in `...` aren't flagged, since the link target might require localization. +__( 'WordPress', 'my-slug' ); // Good +__( 'translatable text', 'my-slug' ) // Bad, since no href. +__( 'translatable text', 'my-slug' ) // Bad, since no href. __( 'translatable text', 'my-slug' ) // Good, contains a placeholder in the href attribute. -// phpcs:enable WordPress.WP.I18n.NoHtmlWrappedStrings // phpcs:set WordPress.WP.I18n text_domain[] // phpcs:set WordPress.WP.I18n check_translator_comments true diff --git a/WordPress/Tests/WP/I18nUnitTest.php b/WordPress/Tests/WP/I18nUnitTest.php index 503c46f3b7..0d466b1d44 100644 --- a/WordPress/Tests/WP/I18nUnitTest.php +++ b/WordPress/Tests/WP/I18nUnitTest.php @@ -169,6 +169,7 @@ public function getWarningList( $testFile = '' ) { 193 => 1, 194 => 1, 214 => 1, + 215 => 1, ); case 'I18nUnitTest.2.inc': From 6663aefb9bcfc70bf1bae8e43ebd0d5888124386 Mon Sep 17 00:00:00 2001 From: Bernie Reiter Date: Wed, 19 Feb 2020 14:12:05 +0600 Subject: [PATCH 14/24] Re-order tests --- WordPress/Tests/WP/I18nUnitTest.1.inc | 12 ++++++------ WordPress/Tests/WP/I18nUnitTest.1.inc.fixed | 12 ++++++------ WordPress/Tests/WP/I18nUnitTest.php | 4 ++-- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/WordPress/Tests/WP/I18nUnitTest.1.inc b/WordPress/Tests/WP/I18nUnitTest.1.inc index 60d7345f12..212bf6c135 100644 --- a/WordPress/Tests/WP/I18nUnitTest.1.inc +++ b/WordPress/Tests/WP/I18nUnitTest.1.inc @@ -193,6 +193,12 @@ __( '', 'my-slug __( 'Foo', 'my-slug' ); // Bad __( 'Foo', 'my-slug' ); // Good +// Strings wrapped in `...` aren't flagged, since the link target might require localization. +__( 'WordPress', 'my-slug' ); // Good +__( 'translatable text', 'my-slug' ) // Bad, since no href. +__( 'translatable text', 'my-slug' ) // Bad, since no href. +__( 'translatable text', 'my-slug' ) // Good, contains a placeholder in the href attribute. + // Strings containing malformed XML, to make sure we're able to parse them. No warnings should be thrown. __( '
text to translate', 'my-slug' ); __( 'text to
translate', 'my-slug' ); @@ -209,11 +215,5 @@ __( 'Foo', 'my-slug' ); __( 'Foo', 'my-slug' ); __( 'Foo', 'my-slug' ); -// Strings wrapped in `...` aren't flagged, since the link target might require localization. -__( 'WordPress', 'my-slug' ); // Good -__( 'translatable text', 'my-slug' ) // Bad, since no href. -__( 'translatable text', 'my-slug' ) // Bad, since no href. -__( 'translatable text', 'my-slug' ) // Good, contains a placeholder in the href attribute. - // phpcs:set WordPress.WP.I18n text_domain[] // phpcs:set WordPress.WP.I18n check_translator_comments true diff --git a/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed b/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed index e7525ec7ad..a91377762d 100644 --- a/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed +++ b/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed @@ -193,6 +193,12 @@ __( '', 'my-slug __( 'Foo', 'my-slug' ); // Bad __( 'Foo', 'my-slug' ); // Good +// Strings wrapped in `...` aren't flagged, since the link target might require localization. +__( 'WordPress', 'my-slug' ); // Good +__( 'translatable text', 'my-slug' ) // Bad, since no href. +__( 'translatable text', 'my-slug' ) // Bad, since no href. +__( 'translatable text', 'my-slug' ) // Good, contains a placeholder in the href attribute. + // Strings containing malformed XML, to make sure we're able to parse them. No warnings should be thrown. __( '
text to translate', 'my-slug' ); __( 'text to
translate', 'my-slug' ); @@ -209,11 +215,5 @@ __( 'Foo', 'my-slug' ); __( 'Foo', 'my-slug' ); __( 'Foo', 'my-slug' ); -// Strings wrapped in `...` aren't flagged, since the link target might require localization. -__( 'WordPress', 'my-slug' ); // Good -__( 'translatable text', 'my-slug' ) // Bad, since no href. -__( 'translatable text', 'my-slug' ) // Bad, since no href. -__( 'translatable text', 'my-slug' ) // Good, contains a placeholder in the href attribute. - // phpcs:set WordPress.WP.I18n text_domain[] // phpcs:set WordPress.WP.I18n check_translator_comments true diff --git a/WordPress/Tests/WP/I18nUnitTest.php b/WordPress/Tests/WP/I18nUnitTest.php index 0d466b1d44..0d81d06e30 100644 --- a/WordPress/Tests/WP/I18nUnitTest.php +++ b/WordPress/Tests/WP/I18nUnitTest.php @@ -168,8 +168,8 @@ public function getWarningList( $testFile = '' ) { 191 => 1, 193 => 1, 194 => 1, - 214 => 1, - 215 => 1, + 198 => 1, + 199 => 1, ); case 'I18nUnitTest.2.inc': From 45abbf687e1867f9c561073081f7e211ce42481c Mon Sep 17 00:00:00 2001 From: Bernie Reiter Date: Wed, 19 Feb 2020 14:13:02 +0600 Subject: [PATCH 15/24] Update comments --- WordPress/Tests/WP/I18nUnitTest.1.inc | 6 +++--- WordPress/Tests/WP/I18nUnitTest.1.inc.fixed | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/WordPress/Tests/WP/I18nUnitTest.1.inc b/WordPress/Tests/WP/I18nUnitTest.1.inc index 212bf6c135..9dee14b160 100644 --- a/WordPress/Tests/WP/I18nUnitTest.1.inc +++ b/WordPress/Tests/WP/I18nUnitTest.1.inc @@ -195,9 +195,9 @@ __( 'Foo', 'my-slug' ); // Good // Strings wrapped in `...` aren't flagged, since the link target might require localization. __( 'WordPress', 'my-slug' ); // Good -__( 'translatable text', 'my-slug' ) // Bad, since no href. -__( 'translatable text', 'my-slug' ) // Bad, since no href. -__( 'translatable text', 'my-slug' ) // Good, contains a placeholder in the href attribute. +__( 'translatable text', 'my-slug' ) // Bad, since no href +__( 'translatable text', 'my-slug' ) // Bad, since no href +__( 'translatable text', 'my-slug' ) // Good // Strings containing malformed XML, to make sure we're able to parse them. No warnings should be thrown. __( '
text to translate', 'my-slug' ); diff --git a/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed b/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed index a91377762d..f143370ec0 100644 --- a/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed +++ b/WordPress/Tests/WP/I18nUnitTest.1.inc.fixed @@ -195,9 +195,9 @@ __( 'Foo', 'my-slug' ); // Good // Strings wrapped in `...` aren't flagged, since the link target might require localization. __( 'WordPress', 'my-slug' ); // Good -__( 'translatable text', 'my-slug' ) // Bad, since no href. -__( 'translatable text', 'my-slug' ) // Bad, since no href. -__( 'translatable text', 'my-slug' ) // Good, contains a placeholder in the href attribute. +__( 'translatable text', 'my-slug' ) // Bad, since no href +__( 'translatable text', 'my-slug' ) // Bad, since no href +__( 'translatable text', 'my-slug' ) // Good // Strings containing malformed XML, to make sure we're able to parse them. No warnings should be thrown. __( '
text to translate', 'my-slug' ); From 728b58b05b18fec1d4bde98c9fa3bd8779d349aa Mon Sep 17 00:00:00 2001 From: Ryan McCue Date: Fri, 28 Feb 2020 16:34:37 +0000 Subject: [PATCH 16/24] Correct grammar for wp_reset_query warning --- WordPress/Sniffs/WP/DiscouragedFunctionsSniff.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WordPress/Sniffs/WP/DiscouragedFunctionsSniff.php b/WordPress/Sniffs/WP/DiscouragedFunctionsSniff.php index 89d8e09c47..d51504a3d3 100644 --- a/WordPress/Sniffs/WP/DiscouragedFunctionsSniff.php +++ b/WordPress/Sniffs/WP/DiscouragedFunctionsSniff.php @@ -46,7 +46,7 @@ public function getGroups() { 'wp_reset_query' => array( 'type' => 'warning', - 'message' => '%s() is discouraged. Use the wp_reset_postdata() instead.', + 'message' => '%s() is discouraged. Use wp_reset_postdata() instead.', 'functions' => array( 'wp_reset_query', ), From 00f9a03169606400b1d615dbd171a5d92ce15a35 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Tue, 31 Mar 2020 19:43:38 +0200 Subject: [PATCH 17/24] Travis: fix the build The Travis docs say that `$TRAVIS_BUILD_STAGE_NAME` is in "proper case" form: > TRAVIS_BUILD_STAGE_NAME: The build stage in capitalized form, e.g. Test or Deploy. If a build does not use build stages, this variable is empty (""). However, it looks like they made an (undocumented) change (probably a bug in their script handling) which means that the `$TRAVIS_BUILD_STAGE_NAME` name is now in the case as given, which in this case is _lowercase_. This means that some of the comparisons are failing and the wrong things are executed for certain builds. As I expect this to be a bug in Travis, I'm not changing the case for the comparisons at this time. Instead I'm fixing this by inline fixing the case of the variable for the comparisons. Refs: * https://docs.travis-ci.com/user/environment-variables#default-environment-variables (near the bottom of the list) --- .travis.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 7c45e6d2db..4369ad0926 100644 --- a/.travis.yml +++ b/.travis.yml @@ -141,7 +141,7 @@ before_install: # On stable PHPCS versions, allow for PHP deprecation notices. # Unit tests don't need to fail on those for stable releases where those issues won't get fixed anymore. - | - if [[ "$TRAVIS_BUILD_STAGE_NAME" != "Sniff" && $PHPCS_BRANCH != "dev-master" ]]; then + if [[ "${TRAVIS_BUILD_STAGE_NAME^}" != "Sniff" && $PHPCS_BRANCH != "dev-master" ]]; then echo 'error_reporting = E_ALL & ~E_DEPRECATED' >> ~/.phpenv/versions/$(phpenv version-name)/etc/conf.d/travis.ini fi @@ -156,7 +156,7 @@ before_install: fi - composer require squizlabs/php_codesniffer:${PHPCS_BRANCH} --update-no-dev --no-suggest --no-scripts - | - if [[ "$TRAVIS_BUILD_STAGE_NAME" == "Sniff" ]]; then + if [[ "${TRAVIS_BUILD_STAGE_NAME^}" == "Sniff" ]]; then composer install --dev --no-suggest # The `dev` required DealerDirect Composer plugin takes care of the installed_paths. else From 48eb3266cb8930ba3780ebf1e8749700af966c1d Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 1 Apr 2020 17:08:45 +0200 Subject: [PATCH 18/24] PreparedSQL: quick fix for namespace separator Reported in 1875: using a namespace separator with function calls in the `WordPress.DB.PreparedSQL` triggers a false positive on the namespace separator as "not being escaped". While this really should be handled differently with proper FQN function name determination for function calls etc, for now, let's just ignore the namespace separator as that is not something which would need to be "prepared". Fixes 1875 --- WordPress/Sniffs/DB/PreparedSQLSniff.php | 1 + WordPress/Tests/DB/PreparedSQLUnitTest.inc | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/WordPress/Sniffs/DB/PreparedSQLSniff.php b/WordPress/Sniffs/DB/PreparedSQLSniff.php index a3ed993542..1fe2ad96da 100644 --- a/WordPress/Sniffs/DB/PreparedSQLSniff.php +++ b/WordPress/Sniffs/DB/PreparedSQLSniff.php @@ -69,6 +69,7 @@ class PreparedSQLSniff extends Sniff { \T_INT_CAST => true, \T_DOUBLE_CAST => true, \T_BOOL_CAST => true, + \T_NS_SEPARATOR => true, ); /** diff --git a/WordPress/Tests/DB/PreparedSQLUnitTest.inc b/WordPress/Tests/DB/PreparedSQLUnitTest.inc index 57fc2c5918..52ef3a0e31 100644 --- a/WordPress/Tests/DB/PreparedSQLUnitTest.inc +++ b/WordPress/Tests/DB/PreparedSQLUnitTest.inc @@ -39,7 +39,7 @@ $all_post_meta = $wpdb->get_results( $wpdb->prepare( sprintf( WHERE `meta_key` = "sort_order" AND `post_id` IN (%s)', $wpdb->postmeta, - implode( ',', array_fill( 0, count( $post_ids ), '%d' ) ) + \implode( ',', \array_fill( 0, \count( $post_ids ), '%d' ) ) ), $post_ids ) ); // Ok. $wpdb->query( " From 18703c727ca0956e69f716f589890432c3a8faed Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 9 Apr 2020 07:45:39 +0200 Subject: [PATCH 19/24] Update default minimum_supported_version to WP 5.1 The minimum version should be three versions behind the latest WP release, so what with 5.4 being out, it should now be 5.1. Includes (not) updating the list of deprecated functions. Input for this based on @JDGrimes's WP deprecated code scanner. --- WordPress/Sniff.php | 2 +- WordPress/Sniffs/WP/DeprecatedFunctionsSniff.php | 6 ++++++ WordPress/Tests/WP/DeprecatedFunctionsUnitTest.inc | 2 ++ WordPress/Tests/WP/DeprecatedFunctionsUnitTest.php | 7 +++++-- 4 files changed, 14 insertions(+), 3 deletions(-) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index 8c3528b2fa..55ca6e7419 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -82,7 +82,7 @@ abstract class Sniff implements PHPCS_Sniff { * * @var string WordPress version. */ - public $minimum_supported_version = '5.0'; + public $minimum_supported_version = '5.1'; /** * Custom list of classes which test classes can extend. diff --git a/WordPress/Sniffs/WP/DeprecatedFunctionsSniff.php b/WordPress/Sniffs/WP/DeprecatedFunctionsSniff.php index 7793917096..df7a4b5525 100644 --- a/WordPress/Sniffs/WP/DeprecatedFunctionsSniff.php +++ b/WordPress/Sniffs/WP/DeprecatedFunctionsSniff.php @@ -1359,6 +1359,12 @@ class DeprecatedFunctionsSniff extends AbstractFunctionRestrictionsSniff { 'alt' => 'wp_update_user()', 'version' => '5.3.0', ), + + // WP 5.4.0. + 'wp_get_user_request_data' => array( + 'alt' => 'wp_get_user_request()', + 'version' => '5.4.0', + ), ); /** diff --git a/WordPress/Tests/WP/DeprecatedFunctionsUnitTest.inc b/WordPress/Tests/WP/DeprecatedFunctionsUnitTest.inc index b7095678b5..3990ad6c4d 100644 --- a/WordPress/Tests/WP/DeprecatedFunctionsUnitTest.inc +++ b/WordPress/Tests/WP/DeprecatedFunctionsUnitTest.inc @@ -345,3 +345,5 @@ install_blog(); _wp_json_prepare_data(); _wp_privacy_requests_screen_options(); update_user_status(); +/* ============ WP 5.4 ============ */ +wp_get_user_request_data(); diff --git a/WordPress/Tests/WP/DeprecatedFunctionsUnitTest.php b/WordPress/Tests/WP/DeprecatedFunctionsUnitTest.php index 6f20c09dca..979807e3b0 100644 --- a/WordPress/Tests/WP/DeprecatedFunctionsUnitTest.php +++ b/WordPress/Tests/WP/DeprecatedFunctionsUnitTest.php @@ -76,10 +76,13 @@ public function getErrorList() { */ public function getWarningList() { - $warnings = array_fill( 342, 6, 1 ); + $warnings = array_fill( 342, 8, 1 ); // Unset the lines related to version comments. - unset( $warnings[344] ); + unset( + $warnings[344], + $warnings[348] + ); return $warnings; } From 054eadd315aa1fc80b1e8b9b22744d77531d503b Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 10 Apr 2020 00:08:00 +0200 Subject: [PATCH 20/24] I18n: rename variable Rename a very long variable name and fix a CS warning. --- WordPress/Sniffs/WP/I18nSniff.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/WordPress/Sniffs/WP/I18nSniff.php b/WordPress/Sniffs/WP/I18nSniff.php index 27bf95470d..b628100d55 100644 --- a/WordPress/Sniffs/WP/I18nSniff.php +++ b/WordPress/Sniffs/WP/I18nSniff.php @@ -630,8 +630,8 @@ protected function check_text( $context ) { * * Strip placeholders and surrounding quotes. */ - $content_without_surrounding_quotes = trim( $this->strip_quotes( $content ) ); - $non_placeholder_content = preg_replace( self::SPRINTF_PLACEHOLDER_REGEX, '', $content_without_surrounding_quotes ); + $content_without_quotes = trim( $this->strip_quotes( $content ) ); + $non_placeholder_content = preg_replace( self::SPRINTF_PLACEHOLDER_REGEX, '', $content_without_quotes ); if ( '' === $non_placeholder_content ) { $this->phpcsFile->addError( 'Strings should have translatable content', $stack_ptr, 'NoEmptyStrings' ); @@ -644,7 +644,7 @@ protected function check_text( $context ) { * Strip surrounding quotes. */ $reader = new \XMLReader(); - $reader->XML( $content_without_surrounding_quotes, 'UTF-8', LIBXML_NOERROR | LIBXML_ERR_NONE | LIBXML_NOWARNING ); + $reader->XML( $content_without_quotes, 'UTF-8', LIBXML_NOERROR | LIBXML_ERR_NONE | LIBXML_NOWARNING ); // Is the first node an HTML element? if ( ! $reader->read() || \XMLReader::ELEMENT !== $reader->nodeType ) { @@ -665,7 +665,7 @@ protected function check_text( $context ) { } // Does the entire string only consist of this HTML node? - if ( $reader->readOuterXml() === $content_without_surrounding_quotes ) { + if ( $reader->readOuterXml() === $content_without_quotes ) { $this->phpcsFile->addWarning( 'Strings should not be wrapped in HTML', $stack_ptr, 'NoHtmlWrappedStrings' ); } } From ef87d607867474bff1ed5782544ee567ccfb7268 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 10 Apr 2020 00:16:28 +0200 Subject: [PATCH 21/24] I18n: use explicit comparison --- WordPress/Sniffs/WP/I18nSniff.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WordPress/Sniffs/WP/I18nSniff.php b/WordPress/Sniffs/WP/I18nSniff.php index b628100d55..2b68730f20 100644 --- a/WordPress/Sniffs/WP/I18nSniff.php +++ b/WordPress/Sniffs/WP/I18nSniff.php @@ -654,7 +654,7 @@ protected function check_text( $context ) { // If the opening HTML element includes placeholders in its attributes, we don't warn. // E.g. ''. for ( $i = 0; $attr = $reader->getAttributeNo( $i ); $i++ ) { - if ( preg_match( self::SPRINTF_PLACEHOLDER_REGEX, $attr ) ) { + if ( preg_match( self::SPRINTF_PLACEHOLDER_REGEX, $attr ) === 1 ) { return; } } From 04fc205838bbafaeee7fad327f119d7c5538a544 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 10 Apr 2020 00:22:41 +0200 Subject: [PATCH 22/24] I18n: assignment in condition is only allowed in a while loop The code as was, was throwing two warnings. Let's not set a bad example. --- WordPress/Sniffs/WP/I18nSniff.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/WordPress/Sniffs/WP/I18nSniff.php b/WordPress/Sniffs/WP/I18nSniff.php index 2b68730f20..d229cb5e9a 100644 --- a/WordPress/Sniffs/WP/I18nSniff.php +++ b/WordPress/Sniffs/WP/I18nSniff.php @@ -653,10 +653,13 @@ protected function check_text( $context ) { // If the opening HTML element includes placeholders in its attributes, we don't warn. // E.g. ''. - for ( $i = 0; $attr = $reader->getAttributeNo( $i ); $i++ ) { + $i = 0; + while ( $attr = $reader->getAttributeNo( $i ) ) { if ( preg_match( self::SPRINTF_PLACEHOLDER_REGEX, $attr ) === 1 ) { return; } + + ++$i; } // We don't flag strings wrapped in `...`, as the link target might actually need localization. From 45d08f258832b429dcf9c08b366951a9d0ff757a Mon Sep 17 00:00:00 2001 From: marcortola <15958009+marcortola@users.noreply.github.com> Date: Sat, 25 Apr 2020 18:08:30 +0200 Subject: [PATCH 23/24] Minor type hint fix --- WordPress/Sniffs/Security/EscapeOutputSniff.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WordPress/Sniffs/Security/EscapeOutputSniff.php b/WordPress/Sniffs/Security/EscapeOutputSniff.php index aef649468f..48a4766bad 100644 --- a/WordPress/Sniffs/Security/EscapeOutputSniff.php +++ b/WordPress/Sniffs/Security/EscapeOutputSniff.php @@ -456,7 +456,7 @@ public function process_token( $stackPtr ) { "All output should be run through an escaping function (see the Security sections in the WordPress Developer Handbooks), found '%s'.", $ptr, 'OutputNotEscaped', - $content + array( $content ) ); } From d5e3cd94a47b9e39262c45e558eece24b7b1255f Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 8 May 2020 22:31:44 +0200 Subject: [PATCH 24/24] Changelog for WPCS 2.3.0 * Release date set at this **Thursday May 14th**. * Includes all currently merged changes. --- CHANGELOG.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 072f0ac71d..f8c8c98784 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,24 @@ This projects adheres to [Semantic Versioning](https://semver.org/) and [Keep a _No documentation available about unreleased changes as of yet._ +## [2.3.0] - 2020-05-14 + +### Added +- The `WordPress.WP.I18n` sniff contains a new check for translatable text strings which are wrapped in HTML tags, like `

Translate me

`. Those tags should be moved out of the translatable string. + Note: Translatable strings wrapped in `` tags where the URL is intended to be localized will not trigger this check. + +### Changed +- The default value for `minimum_supported_wp_version`, as used by a [number of sniffs detecting usage of deprecated WP features](https://github.com/WordPress/WordPress-Coding-Standards/wiki/Customizable-sniff-properties#minimum-wp-version-to-check-for-usage-of-deprecated-functions-classes-and-function-parameters), has been updated to `5.1`. +- The `WordPress.WP.DeprecatedFunctions` sniff will now detect functions deprecated in WP 5.4. +- Improved grammar of an error message in the `WordPress.WP.DiscouragedFunctions` sniff. +- CI: The codebase is now - preliminary - being tested against the PHPCS 4.x development branch. + +### Fixed +- All function call detection sniffs: fixed a bug where constants with the same name as one of the targeted functions could inadvertently be recognized as if they were a called function. +- `WordPress.DB.PreparedSQL`: fixed a bug where the sniff would trigger on the namespace separator character `\\`. +- `WordPress.Security.EscapeOutput`: fixed a bug with the variable replacement in one of the error messages. + + ## [2.2.1] - 2020-02-04 ### Added