From 83ddd30fe644168d59be0f958574ea88b17c18a8 Mon Sep 17 00:00:00 2001 From: Dan Wallis Date: Tue, 11 Jan 2022 22:20:58 +0000 Subject: [PATCH 01/11] Enforce consistent indentation --- eslint/.eslintrc-magento | 1 + eslint/rules/utils.js | 24 ++++++++++++------------ 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/eslint/.eslintrc-magento b/eslint/.eslintrc-magento index 5d403da3..0ae1f657 100644 --- a/eslint/.eslintrc-magento +++ b/eslint/.eslintrc-magento @@ -18,6 +18,7 @@ "eol-last": 2, "eqeqeq": [2, "smart"], "guard-for-in": 2, + "indent": [2, 4], "keyword-spacing": [2, {}], "lines-around-comment": [ 2, diff --git a/eslint/rules/utils.js b/eslint/rules/utils.js index ae181210..398e2b86 100644 --- a/eslint/rules/utils.js +++ b/eslint/rules/utils.js @@ -75,18 +75,18 @@ function getExpressionId(node) { while (node) { switch (node.type) { - case 'CallExpression': - node = node.callee; - break; - - case 'MemberExpression': - node = node.object; - break; - - case 'Identifier': - return node; - default: - return null; + case 'CallExpression': + node = node.callee; + break; + + case 'MemberExpression': + node = node.object; + break; + + case 'Identifier': + return node; + default: + return null; } } } From 0f1c1f195aa12117e80b7e9026338ce4528ea19e Mon Sep 17 00:00:00 2001 From: Dan Wallis Date: Tue, 11 Jan 2022 22:27:50 +0000 Subject: [PATCH 02/11] Forbid overriding built-in objects --- eslint/.eslintrc-magento | 1 + 1 file changed, 1 insertion(+) diff --git a/eslint/.eslintrc-magento b/eslint/.eslintrc-magento index 5d403da3..f84ff61c 100644 --- a/eslint/.eslintrc-magento +++ b/eslint/.eslintrc-magento @@ -50,6 +50,7 @@ "no-fallthrough": 2, "no-floating-decimal": 2, "no-func-assign": 2, + "no-global-assign": 2, "no-implied-eval": 2, "no-inner-declarations": 2, "no-invalid-regexp": 2, From 241f932a0324776cee6f451a9dbd8b3ba1064152 Mon Sep 17 00:00:00 2001 From: Dan Wallis Date: Tue, 11 Jan 2022 22:37:45 +0000 Subject: [PATCH 03/11] Disallow useless constructs --- eslint/.eslintrc-magento | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/eslint/.eslintrc-magento b/eslint/.eslintrc-magento index 5d403da3..7283333c 100644 --- a/eslint/.eslintrc-magento +++ b/eslint/.eslintrc-magento @@ -81,6 +81,12 @@ } ], "no-use-before-define": 2, + "no-useless-call": 2, + "no-useless-computed-key": 2, + "no-useless-constructor": 2, + "no-useless-escape": 2, + "no-useless-rename": 2, + "no-useless-return": 2, "no-with": 2, "one-var": [2, "always"], "operator-assignment": [2, "always"], From 784a702a0923366be28ed62f1d2feb2efc27ae98 Mon Sep 17 00:00:00 2001 From: Kiel Pykett Date: Tue, 25 Jan 2022 10:18:06 +0000 Subject: [PATCH 04/11] Allow Template Literals --- eslint/.eslintrc-magento | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/eslint/.eslintrc-magento b/eslint/.eslintrc-magento index 5d403da3..a25fcf5c 100644 --- a/eslint/.eslintrc-magento +++ b/eslint/.eslintrc-magento @@ -84,7 +84,7 @@ "no-with": 2, "one-var": [2, "always"], "operator-assignment": [2, "always"], - "quotes": [2, "single"], + "quotes": [2, "single", {"allowTemplateLiterals": true}], "radix": 2, "semi": [2, "always"], "semi-spacing": 2, From 21db43236599f62bc161b1fdb35c7daa9559acac Mon Sep 17 00:00:00 2001 From: Aad Mathijssen Date: Fri, 4 Mar 2022 20:27:46 +0100 Subject: [PATCH 05/11] Add semicolon as statement separator in the special annotation check of the `Magento2.Security.XssTemplate` sniff --- Magento2/Sniffs/Security/XssTemplateSniff.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Magento2/Sniffs/Security/XssTemplateSniff.php b/Magento2/Sniffs/Security/XssTemplateSniff.php index 3b4385e8..c7067d20 100644 --- a/Magento2/Sniffs/Security/XssTemplateSniff.php +++ b/Magento2/Sniffs/Security/XssTemplateSniff.php @@ -146,11 +146,11 @@ public function process(File $phpcsFile, $stackPtr) private function findSpecialAnnotation($stackPtr) { if ($this->tokens[$stackPtr]['code'] === T_ECHO) { - $startOfStatement = $this->file->findPrevious(T_OPEN_TAG, $stackPtr); + $startOfStatement = $this->file->findPrevious([T_OPEN_TAG, T_SEMICOLON], $stackPtr); return $this->file->findPrevious(T_COMMENT, $stackPtr, $startOfStatement); } if ($this->tokens[$stackPtr]['code'] === T_OPEN_TAG_WITH_ECHO) { - $endOfStatement = $this->file->findNext(T_CLOSE_TAG, $stackPtr); + $endOfStatement = $this->file->findNext([T_CLOSE_TAG, T_SEMICOLON], $stackPtr); return $this->file->findNext(T_COMMENT, $stackPtr, $endOfStatement); } return false; From 5936239050ddb99f5aea5c9ab655efbc1e6c2c35 Mon Sep 17 00:00:00 2001 From: Dan Wallis Date: Thu, 26 Jan 2023 11:51:03 +0000 Subject: [PATCH 06/11] Automatically remove useless comments When issues are detected and classified as Magento2.Commenting.ClassAndInterfacePHPDocFormatting.ForbiddenTags or Magento2.Commenting.ClassAndInterfacePHPDocFormatting.InvalidDescription, these can be fixed by removing the offending comment. --- ...ClassAndInterfacePHPDocFormattingSniff.php | 42 ++++- ...AndInterfacePHPDocFormattingUnitTest.1.inc | 7 + ...erfacePHPDocFormattingUnitTest.1.inc.fixed | 166 ++++++++++++++++++ ...AndInterfacePHPDocFormattingUnitTest.2.inc | 17 ++ ...erfacePHPDocFormattingUnitTest.2.inc.fixed | 166 ++++++++++++++++++ ...ssAndInterfacePHPDocFormattingUnitTest.php | 3 +- 6 files changed, 398 insertions(+), 3 deletions(-) create mode 100644 Magento2/Tests/Commenting/ClassAndInterfacePHPDocFormattingUnitTest.1.inc.fixed create mode 100644 Magento2/Tests/Commenting/ClassAndInterfacePHPDocFormattingUnitTest.2.inc.fixed diff --git a/Magento2/Sniffs/Commenting/ClassAndInterfacePHPDocFormattingSniff.php b/Magento2/Sniffs/Commenting/ClassAndInterfacePHPDocFormattingSniff.php index 2d715c50..fec9f396 100644 --- a/Magento2/Sniffs/Commenting/ClassAndInterfacePHPDocFormattingSniff.php +++ b/Magento2/Sniffs/Commenting/ClassAndInterfacePHPDocFormattingSniff.php @@ -62,8 +62,10 @@ public function process(File $phpcsFile, $stackPtr) return; } + $commentCloserPtr = $tokens[$commentStartPtr]['comment_closer']; + if ($this->PHPDocFormattingValidator->providesMeaning($namePtr, $commentStartPtr, $tokens) !== true) { - $phpcsFile->addWarning( + $fix = $phpcsFile->addFixableWarning( sprintf( '%s description must contain meaningful information beyond what its name provides or be removed.', ucfirst($tokens[$stackPtr]['content']) @@ -71,6 +73,18 @@ public function process(File $phpcsFile, $stackPtr) $stackPtr, 'InvalidDescription' ); + + if ($fix) { + for ($i = $commentStartPtr; $i <= $commentCloserPtr; $i++) { + $phpcsFile->fixer->replaceToken($i, ''); + } + + if ($tokens[$commentStartPtr - 1]['code'] === T_WHITESPACE + && $tokens[$commentCloserPtr + 1]['code'] === T_WHITESPACE + ) { + $phpcsFile->fixer->replaceToken($commentCloserPtr + 1, ''); + } + } } if ($this->PHPDocFormattingValidator->hasDeprecatedWellFormatted($commentStartPtr, $tokens) !== true) { @@ -104,11 +118,35 @@ private function validateTags(File $phpcsFile, $commentStartPtr, $tokens) } if (in_array($tokens[$i]['content'], $this->forbiddenTags) === true) { - $phpcsFile->addWarning( + $fix = $phpcsFile->addFixableWarning( sprintf('Tag %s MUST NOT be used.', $tokens[$i]['content']), $i, 'ForbiddenTags' ); + + if ($fix) { + for ($j = $i - 1; $j > $commentStartPtr; $j--) { + if (!in_array($tokens[$j]['code'], [T_DOC_COMMENT_STAR, T_DOC_COMMENT_WHITESPACE], true)) { + break; + } + + if ($tokens[$j]['code'] === T_DOC_COMMENT_WHITESPACE && $tokens[$j]['content'] === "\n") { + break; + } + + $phpcsFile->fixer->replaceToken($j, ''); + } + + $phpcsFile->fixer->replaceToken($i, ''); + + for ($j = $i + 1; $j < $commentCloserPtr; $j++) { + $phpcsFile->fixer->replaceToken($j, ''); + + if ($tokens[$j]['code'] === T_DOC_COMMENT_WHITESPACE && $tokens[$j]['content'] === "\n") { + break; + } + } + } } } diff --git a/Magento2/Tests/Commenting/ClassAndInterfacePHPDocFormattingUnitTest.1.inc b/Magento2/Tests/Commenting/ClassAndInterfacePHPDocFormattingUnitTest.1.inc index afd4c934..a17e80c8 100644 --- a/Magento2/Tests/Commenting/ClassAndInterfacePHPDocFormattingUnitTest.1.inc +++ b/Magento2/Tests/Commenting/ClassAndInterfacePHPDocFormattingUnitTest.1.inc @@ -179,3 +179,10 @@ class AlsoDeprecatedButHandler } +/** + * @package this tag should not be used + */ +class OnlyUselessCommentContent +{ + +} diff --git a/Magento2/Tests/Commenting/ClassAndInterfacePHPDocFormattingUnitTest.1.inc.fixed b/Magento2/Tests/Commenting/ClassAndInterfacePHPDocFormattingUnitTest.1.inc.fixed new file mode 100644 index 00000000..6be0195c --- /dev/null +++ b/Magento2/Tests/Commenting/ClassAndInterfacePHPDocFormattingUnitTest.1.inc.fixed @@ -0,0 +1,166 @@ + 1, 109 => 1, 118 => 1, - 127 => 1 + 127 => 1, + 183 => 1, ]; } } From ed981cff04b257f9e8c265b63fc76cad4f386a86 Mon Sep 17 00:00:00 2001 From: Kiel Pykett Date: Sat, 15 Apr 2023 01:34:46 +0100 Subject: [PATCH 07/11] Make Unescaped Output Error With Severity 10 --- Magento2/ruleset.xml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Magento2/ruleset.xml b/Magento2/ruleset.xml index ee171871..81ff0d8d 100644 --- a/Magento2/ruleset.xml +++ b/Magento2/ruleset.xml @@ -94,6 +94,10 @@ 10 error + + 10 + error + 10 error From 7b6bd743937183d00390a9fdd9d3fa7af389ac30 Mon Sep 17 00:00:00 2001 From: Dan Wallis Date: Thu, 6 Jul 2023 14:48:29 +0100 Subject: [PATCH 08/11] Add sniff for deprecated use of $block->escape... --- .../Legacy/EscapeMethodsOnBlockClassSniff.php | 100 ++++++++++++++++++ .../EscapeMethodsOnBlockClassUnitTest.inc | 87 +++++++++++++++ ...scapeMethodsOnBlockClassUnitTest.inc.fixed | 87 +++++++++++++++ .../EscapeMethodsOnBlockClassUnitTest.php | 45 ++++++++ 4 files changed, 319 insertions(+) create mode 100644 Magento2/Sniffs/Legacy/EscapeMethodsOnBlockClassSniff.php create mode 100644 Magento2/Tests/Legacy/EscapeMethodsOnBlockClassUnitTest.inc create mode 100644 Magento2/Tests/Legacy/EscapeMethodsOnBlockClassUnitTest.inc.fixed create mode 100644 Magento2/Tests/Legacy/EscapeMethodsOnBlockClassUnitTest.php diff --git a/Magento2/Sniffs/Legacy/EscapeMethodsOnBlockClassSniff.php b/Magento2/Sniffs/Legacy/EscapeMethodsOnBlockClassSniff.php new file mode 100644 index 00000000..f44e3d0c --- /dev/null +++ b/Magento2/Sniffs/Legacy/EscapeMethodsOnBlockClassSniff.php @@ -0,0 +1,100 @@ + true, + 'escapeHtml' => true, + 'escapeHtmlAttr' => true, + 'escapeJs' => true, + 'escapeJsQuote' => true, + 'escapeQuote' => true, + 'escapeUrl' => true, + 'escapeXssInUrl' => true, + ]; + + public function register() + { + return [ + T_OBJECT_OPERATOR, + ]; + } + + public function process(File $phpcsFile, $stackPtr) + { + $tokens = $phpcsFile->getTokens(); + + if ($stackPtr <= 1 || !isset($tokens[$stackPtr + 2])) { + return; + } + + $objectPtr = $stackPtr - 1; + if ($tokens[$objectPtr]['code'] !== T_VARIABLE) { + $objectPtr = $phpcsFile->findPrevious(Tokens::$emptyTokens, $objectPtr, null, true); + + if (!$objectPtr) { + return; + } + } + + if ($tokens[$objectPtr]['code'] !== T_VARIABLE + || $tokens[$objectPtr]['content'] !== '$block' + ) { + return; + } + + $methodPtr = $stackPtr + 1; + if ($tokens[$methodPtr]['code'] !== T_STRING) { + $methodPtr = $phpcsFile->findNext(Tokens::$emptyTokens, $methodPtr, null, true); + + if (!$methodPtr) { + return; + } + } + + if ($tokens[$methodPtr]['code'] !== T_STRING + || !isset(self::ESCAPER_METHODS[$tokens[$methodPtr]['content']]) + ) { + return; + } + + $openParenPtr = $methodPtr + 1; + if ($tokens[$openParenPtr]['code'] !== T_OPEN_PARENTHESIS) { + $openParenPtr = $phpcsFile->findNext(Tokens::$emptyTokens, $openParenPtr, null, true); + + if (!$openParenPtr) { + return; + } + } + + if ($tokens[$openParenPtr]['code'] !== T_OPEN_PARENTHESIS) { + return; + } + + $fix = $phpcsFile->addFixableWarning( + 'Using %s on $block is deprecated. Please use equivalent method on $escaper', + $methodPtr, + 'Found', + [ + $tokens[$methodPtr]['content'], // method name + ] + ); + + if ($fix) { + $phpcsFile->fixer->replaceToken($objectPtr, '$escaper'); + } + } +} diff --git a/Magento2/Tests/Legacy/EscapeMethodsOnBlockClassUnitTest.inc b/Magento2/Tests/Legacy/EscapeMethodsOnBlockClassUnitTest.inc new file mode 100644 index 00000000..260f0c93 --- /dev/null +++ b/Magento2/Tests/Legacy/EscapeMethodsOnBlockClassUnitTest.inc @@ -0,0 +1,87 @@ + + +
+

This unescaped output is fine here; other sniffs will complain about it though.

+ + getSomeString(); ?> + getSomeString(); ?> + getSomeString(); ?> + getSomeString(); ?> +
+ +
+

These should be using equivalent methods on the `$escaper` class, not the `$block` class.

+ + Note that I couldn't find any use of this method in any templates within Magento. + escapeCss($block->getSomeString()); ?> + + escapeHtml(__($block->getSomeString())) ?> + escapeHtml(__($block->getSomeString())); ?> + escapeHtml(__($block->getSomeString()), ['strong', 'em', 'span']) ?> + +
+
+
+ + + + The only example of this method being used was in a block class, rather than a template. + getItems() as $item) { + $item['sku'] = $block->escapeJsQuote($item['sku']); + } + ?> + + The only example of this method being used was in a block class, rather than a template. + escapeQuote(__($block->getData('welcome'))); ?> + + link text + + Note that I couldn't find any use of this method in any templates within Magento. + escapeXssInUrl($block->getSomeString()); ?> +
+ +
+

These are edge cases for formatting differences

+ + escapeHtml(''); + $block ->escapeHtml(''); + $block-> escapeHtml(''); + $block + ->escapeHtml(''); + $block + + ->escapeHtml(''); + $block-> + escapeHtml(''); + $block-> // comment + escapeHtml(''); + $block /* comment */ + ->escapeHtml(''); + + $block /* comment */ -> /* comment */ escapeHtml(''); + ?> +
+ +
+

These close-matches shouldn't be flagged by this sniff.

+ + escapeHTML(__($block->getSomeString())) ?> + escapeHtmlString(__($block->getSomeString())) ?> + escapeHtmlAttribute($block->getSomeString()) ?> + escapeCSS($block->getSomeString()); ?> + escapeJS($block->getData('html_id')) ?> + escapeJavaScript($block->getData('html_id')) ?> + escapeQuotes(__($block->getData('welcome'))); ?> + escapeURL($block->getUrl('adminhtml/notification/index')) ?> +
diff --git a/Magento2/Tests/Legacy/EscapeMethodsOnBlockClassUnitTest.inc.fixed b/Magento2/Tests/Legacy/EscapeMethodsOnBlockClassUnitTest.inc.fixed new file mode 100644 index 00000000..80c4f22c --- /dev/null +++ b/Magento2/Tests/Legacy/EscapeMethodsOnBlockClassUnitTest.inc.fixed @@ -0,0 +1,87 @@ + + +
+

This unescaped output is fine here; other sniffs will complain about it though.

+ + getSomeString(); ?> + getSomeString(); ?> + getSomeString(); ?> + getSomeString(); ?> +
+ +
+

These should be using equivalent methods on the `$escaper` class, not the `$block` class.

+ + Note that I couldn't find any use of this method in any templates within Magento. + escapeCss($block->getSomeString()); ?> + + escapeHtml(__($block->getSomeString())) ?> + escapeHtml(__($block->getSomeString())); ?> + escapeHtml(__($block->getSomeString()), ['strong', 'em', 'span']) ?> + +
+
+
+ + + + The only example of this method being used was in a block class, rather than a template. + getItems() as $item) { + $item['sku'] = $escaper->escapeJsQuote($item['sku']); + } + ?> + + The only example of this method being used was in a block class, rather than a template. + escapeQuote(__($block->getData('welcome'))); ?> + + link text + + Note that I couldn't find any use of this method in any templates within Magento. + escapeXssInUrl($block->getSomeString()); ?> +
+ +
+

These are edge cases for formatting differences

+ + escapeHtml(''); + $escaper ->escapeHtml(''); + $escaper-> escapeHtml(''); + $escaper + ->escapeHtml(''); + $escaper + + ->escapeHtml(''); + $escaper-> + escapeHtml(''); + $escaper-> // comment + escapeHtml(''); + $escaper /* comment */ + ->escapeHtml(''); + + $escaper /* comment */ -> /* comment */ escapeHtml(''); + ?> +
+ +
+

These close-matches shouldn't be flagged by this sniff.

+ + escapeHTML(__($block->getSomeString())) ?> + escapeHtmlString(__($block->getSomeString())) ?> + escapeHtmlAttribute($block->getSomeString()) ?> + escapeCSS($block->getSomeString()); ?> + escapeJS($block->getData('html_id')) ?> + escapeJavaScript($block->getData('html_id')) ?> + escapeQuotes(__($block->getData('welcome'))); ?> + escapeURL($block->getUrl('adminhtml/notification/index')) ?> +
diff --git a/Magento2/Tests/Legacy/EscapeMethodsOnBlockClassUnitTest.php b/Magento2/Tests/Legacy/EscapeMethodsOnBlockClassUnitTest.php new file mode 100644 index 00000000..feead3d1 --- /dev/null +++ b/Magento2/Tests/Legacy/EscapeMethodsOnBlockClassUnitTest.php @@ -0,0 +1,45 @@ + 1, + 21 => 1, + 22 => 1, + 23 => 1, + 25 => 1, + 26 => 1, + 27 => 1, + 31 => 1, + 40 => 1, + 45 => 1, + 47 => 1, + 50 => 1, + 57 => 1, + 58 => 1, + 59 => 1, + 61 => 1, + 64 => 1, + 66 => 1, + 68 => 1, + 70 => 1, + 72 => 1, + ]; + } +} From b8acb6c61a735b9770470ea44cefbebf70139f69 Mon Sep 17 00:00:00 2001 From: Dan Wallis Date: Thu, 6 Jul 2023 15:25:49 +0100 Subject: [PATCH 09/11] Add docblock to appease coding standard Note that these are useless, as it's the default behaviour to inherit from the parent docblock. --- Magento2/Sniffs/Legacy/EscapeMethodsOnBlockClassSniff.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Magento2/Sniffs/Legacy/EscapeMethodsOnBlockClassSniff.php b/Magento2/Sniffs/Legacy/EscapeMethodsOnBlockClassSniff.php index f44e3d0c..4d6d8288 100644 --- a/Magento2/Sniffs/Legacy/EscapeMethodsOnBlockClassSniff.php +++ b/Magento2/Sniffs/Legacy/EscapeMethodsOnBlockClassSniff.php @@ -26,6 +26,9 @@ class EscapeMethodsOnBlockClassSniff implements Sniff 'escapeXssInUrl' => true, ]; + /** + * @inheritDoc + */ public function register() { return [ @@ -33,6 +36,9 @@ public function register() ]; } + /** + * @inheritDoc + */ public function process(File $phpcsFile, $stackPtr) { $tokens = $phpcsFile->getTokens(); From 5497f3f9f4b7c478ce6d1bef2f792ee16de33e4f Mon Sep 17 00:00:00 2001 From: Zach Stein Date: Wed, 26 Jul 2023 14:19:57 -0400 Subject: [PATCH 10/11] Update README.md Fixed a typo in readme.md --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 3cfd6c0a..8ca973e1 100644 --- a/README.md +++ b/README.md @@ -113,7 +113,7 @@ npm run eslint -- path/to/analyze ``` ### RECTOR PHP -From `magento-condign-standard` project, you can execute rector php as follows: +From `magento-coding-standard` project, you can execute rector php as follows: ```bash vendor/bin/rector process Magento2 Magento2Framework PHP_CodeSniffer --dry-run --autoload-file vendor/squizlabs/php_codesniffer/autoload.php ``` From 045be185b47acbc1fe077eb175c225de279d950f Mon Sep 17 00:00:00 2001 From: Dan Wallis Date: Tue, 11 Jan 2022 22:05:24 +0000 Subject: [PATCH 11/11] Enforce consistent spacing around keywords --- eslint/.eslintrc-magento | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/eslint/.eslintrc-magento b/eslint/.eslintrc-magento index 8ae28b85..e74a6f95 100644 --- a/eslint/.eslintrc-magento +++ b/eslint/.eslintrc-magento @@ -19,7 +19,7 @@ "eqeqeq": [2, "smart"], "guard-for-in": 2, "indent": [2, 4], - "keyword-spacing": [2, {}], + "keyword-spacing": [2, {"after": true, "before": true}], "lines-around-comment": [ 2, {