MM-4941: [EQP][Sniffs Consolidation] Create new GitHub repo and move MEQP2 sniffs #1

lenaorobei · 2018-12-19T16:50:41Z

Decoupled MEQP2 sniffs from MEQP coding standard
Skipped false-positive and dynamic sniffs for now

…MEQP2 sniffs - Decoupled MEQP2 sniffs from MEQP coding standard - Skipped false-positive and dynamic sniffs for now

paliarush · 2018-12-20T19:24:23Z

README.md

+# Magento Coding Standard
+
+This is just the beginning of the coding standards consolidation process.


Better not add this, because we will forget to remove it eventually.

paliarush · 2018-12-20T19:25:46Z

README.md

+
+1. Make it easier to run static checks for core project contributors and extensions developers.
+2. Store all related to Magento 2 sniffs in one place.


Suggested change

2. Store all related to Magento 2 sniffs in one place.

2. Store all Magento 2 sniffs in one place.

Is this just Magento 2 or both?

Magento 2 only. Fixed message.

paliarush · 2018-12-20T20:09:15Z

composer.json

+        "squizlabs/php_codesniffer": "~3.3.0"
+    },
+    "require-dev": {
+        "phpunit/phpunit": "^4.0 || ^5.0 || ^6.0 || ^7.0"


How are we going to ensure compatibility with all major versions?

This is the only way to support "php": ">=5.5.0" compatibility which we need for Magento 2.0.x.

What I mean is, are we going to test our sniffs with all phpunit versions?

I used the same approach as PHP CodeSniffer https://github.com/squizlabs/PHP_CodeSniffer/blob/3.3.0/composer.json#L34

paliarush · 2018-12-20T20:51:58Z

Magento/Tests/Classes/ObjectInstantiationUnitTest.inc

@@ -0,0 +1,21 @@
+<?php


Why do we use custom .inc extension? Where does this convention come from for such tests?

This is out-of-box CodeSniffer approach for unit testing.

paliarush · 2018-12-20T20:55:27Z

Magento/Tests/CodeAnalysis/EmptyBlockUnitTest.inc

+
+function emptyBlockT () { return true; }
+
+interface EmptyBlockInterface { /*Empty interface block*/ }


Will it fail for an empty interface which extends another one? It should not.
Same for the class.

Please consider covering such scenarios.

I removed check for empty class and interface for now.

paliarush · 2018-12-20T23:55:07Z

Magento/Sniffs/Performance/EmptyCheckSniff.php

+    protected $map = [
+        'count' => [
+            // phpcs:ignore Generic.Files.LineLength.TooLong
+            'message' => 'count(...) function should not be used to check if array is empty. Use empty(...) language construct instead',


Need to run all these sniffs against Magento 2 repository to see how many false positive we get. Maybe will have to remove some of them like this one.

@lenaorobei could you please analyze this?

Here is the result of running against app/code

A TOTAL OF 0 ERRORS AND 253 WARNINGS WERE FOUND IN 205 FILES

At first glance - 90% of findings are positive.

paliarush · 2018-12-21T00:02:41Z

Magento/Sniffs/PHP/ReturnValueCheckSniff.php

+use PHP_CodeSniffer\Files\File;
+
+/**
+ * Detects misusing of IS_IDENTICAL operators.


What are we trying to detect with this sniff, briefly looked at Magento/Tests/PHP/ReturnValueCheckUnitTest.inc but cannot get the purpose.

Strict comparison should be user to check the return value for strpos, stripos,
array_search, because they can return boolean false, and may also return a non-boolean value which evaluates to false.

http://php.net/manual/en/function.strpos.php

paliarush · 2018-12-21T00:09:02Z

Magento/Tests/Strings/RegExUnitTest.inc

+    'get_input'
+);
+
+$html = preg_replace('(<h([1-6])>(.*?)</h\1>)sex', '"<h$1>" . strtoupper("$2") . "</h$1>"', $html);


Suggested change

$html = preg_replace('(<h([1-6])>(.*?)</h\1>)sex', '"<h$1>" . strtoupper("$2") . "</h$1>"', $html);

$html = preg_replace('(<h([1-6])>(.*?)</h\1>)xes', '"<h$1>" . strtoupper("$2") . "</h$1>"', $html);

paliarush · 2018-12-21T00:09:50Z

Magento/Tests/Strings/RegExUnitTest.inc

@@ -0,0 +1,26 @@
+<?php
+
+$input = "Bet you want a BMW.";


Suggested change

$input = "Bet you want a BMW.";

$input = "Test Input.";

paliarush · 2018-12-21T00:15:55Z

Magento/Tests/Templates/ThisInTemplateUnitTest.inc

+echo $block->escapeHtml($block->getGroupCode());
+echo $this->escapeHtml($this->getGroupCode());
+$this->foo();
+$this->helper();


I believe this is not allowed either. Please provide source of information which states that this is allowed.

Fixed sniff.

…MEQP2 sniffs - Code review fixes

…MEQP2 sniffs - Removed severity from PHP code and added to the ruleset

Merging in parent develop

…coding-standard-198 [Imported] Discourage local-FS-only methods

Fatal error: Uncaught TypeError: vsprintf(): Argument magento#2 ($values) must be of type array, int given in /srv/www/vendor/squizlabs/php_codesniffer/src/Files/File.php:1056 Stack trace: #0 /srv/www/vendor/squizlabs/php_codesniffer/src/Files/File.php(1056): vsprintf('Direct throw of...', 531) magento#1 /srv/www/vendor/squizlabs/php_codesniffer/src/Files/File.php(706): PHP_CodeSniffer\Files\File->addMessage(false, 'Direct throw of...', 67, 4, 'FoundDirectThro...', 531, 8, false) magento#2 /srv/www/vendor/magento/magento-coding-standard/Magento2/Sniffs/Exceptions/DirectThrowSniff.php(48): PHP_CodeSniffer\Files\File->addWarning('Direct throw of...', 526, 'FoundDirectThro...', 531) magento#3 /srv/www/vendor/squizlabs/php_codesniffer/src/Files/File.php(498): Magento2\Sniffs\Exceptions\DirectThrowSniff->process(Object(PHP_CodeSniffer\Files\LocalFile), 526) magento#4 /srv/www/vendor/squizlabs/php_codesniffer/src/Files/LocalFile.php(92): PHP_CodeSniffer\Files\File->process() #5 /srv/www/vendor/squizlabs/php_codesniffer/src/Runner.php(630): PHP_CodeSniffer\Files\LocalFile->process() magento#6 /srv/www/vendor/squizlabs/php_codesniffer/src/Runner.php(434): PHP_CodeSniffer\Runner->processFile(Object(PHP_CodeSniffer\Files\LocalFile)) magento#7 /srv/www/vendor/squizlabs/php_codesniffer/src/Runner.php(114): PHP_CodeSniffer\Runner->run() magento#8 /srv/www/vendor/squizlabs/php_codesniffer/bin/phpcs(18): PHP_CodeSniffer\Runner->runPHPCS() magento#9 {main} thrown in /srv/www/vendor/squizlabs/php_codesniffer/src/Files/File.php on line 1056

PHP Fatal error: Uncaught TypeError: strpos(): Argument magento#2 ($needle) must be of type string, null given in /srv/www/vendor/magento/magento-coding-standard/Magento2/Sniffs/Annotation/MethodArgumentsSniff.php:629 Stack trace: #0 /srv/www/vendor/magento/magento-coding-standard/Magento2/Sniffs/Annotation/MethodArgumentsSniff.php(629): strpos('\\VendorName\\Mod...', NULL) magento#1 /srv/www/vendor/magento/magento-coding-standard/Magento2/Sniffs/Annotation/MethodArgumentsSniff.php(515): Magento2\Sniffs\Annotation\MethodArgumentsSniff->validateFormattingConsistency(Array, Array, Object(PHP_CodeSniffer\Files\LocalFile), Array) magento#2 /srv/www/vendor/magento/magento-coding-standard/Magento2/Sniffs/Annotation/MethodArgumentsSniff.php(600): Magento2\Sniffs\Annotation\MethodArgumentsSniff->validateMethodParameterAnnotations(385, Array, Array, Object(PHP_CodeSniffer\Files\LocalFile), Array, 356, 380) magento#3 /srv/www/vendor/squizlabs/php_codesniffer/src/Files/File.php(498): Magento2\Sniffs\Annotation\MethodArgumentsSniff->process(Object(PHP_CodeSniffer\Files\LocalFile), 385) magento#4 /srv/www/vendor/squizlabs/php_codesniffer/src/Files/LocalFile.php(92): PHP_CodeSniffer\Files\File->process() #5 /srv/www/vendor/squizlabs/php_codesniffer/src/Runner.php(630): PHP_CodeSniffer\Files\LocalFile->process() magento#6 /srv/www/vendor/squizlabs/php_codesniffer/src/Runner.php(434): PHP_CodeSniffer\Runner->processFile(Object(PHP_CodeSniffer\Files\LocalFile)) magento#7 /srv/www/vendor/squizlabs/php_codesniffer/src/Runner.php(114): PHP_CodeSniffer\Runner->run() magento#8 /srv/www/vendor/squizlabs/php_codesniffer/bin/phpcs(18): PHP_CodeSniffer\Runner->runPHPCS() magento#9 {main} thrown in /srv/www/vendor/magento/magento-coding-standard/Magento2/Sniffs/Annotation/MethodArgumentsSniff.php on line 629

MM-4941: [EQP][Sniffs Consolidation] Create new GitHub repo and move …

1c22ba7

…MEQP2 sniffs - Decoupled MEQP2 sniffs from MEQP coding standard - Skipped false-positive and dynamic sniffs for now

lenaorobei requested a review from paliarush December 19, 2018 16:50

paliarush reviewed Dec 21, 2018

View reviewed changes

lenaorobei added 2 commits December 21, 2018 10:23

MM-4941: [EQP][Sniffs Consolidation] Create new GitHub repo and move …

8b81307

…MEQP2 sniffs - Code review fixes

MM-4941: [EQP][Sniffs Consolidation] Create new GitHub repo and move …

bb77a39

…MEQP2 sniffs - Removed severity from PHP code and added to the ruleset

paliarush approved these changes Dec 21, 2018

View reviewed changes

paliarush merged commit 7630545 into master Dec 21, 2018

lenaorobei deleted the MM-4941-move-MEQP2-sniffs branch December 23, 2018 01:26

lenaorobei pushed a commit that referenced this pull request Apr 8, 2019

Merge pull request #1 from magento/develop

0dcfef0

Merging in parent develop

danmooney2 mentioned this pull request May 22, 2019

Exception thrown in single catch block passes #93

Closed

mmansoor-magento pushed a commit that referenced this pull request Dec 2, 2020

Merge pull request #1 from magento-commerce/imported-magento-magento-…

b74f41f

…coding-standard-198 [Imported] Discourage local-FS-only methods

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MM-4941: [EQP][Sniffs Consolidation] Create new GitHub repo and move MEQP2 sniffs #1

MM-4941: [EQP][Sniffs Consolidation] Create new GitHub repo and move MEQP2 sniffs #1

lenaorobei commented Dec 19, 2018

paliarush Dec 20, 2018

lenaorobei Dec 21, 2018

paliarush Dec 20, 2018

lenaorobei Dec 21, 2018

paliarush Dec 20, 2018

lenaorobei Dec 21, 2018

paliarush Dec 21, 2018

lenaorobei Dec 21, 2018

paliarush Dec 20, 2018

lenaorobei Dec 21, 2018

paliarush Dec 20, 2018

lenaorobei Dec 21, 2018

paliarush Dec 20, 2018

lenaorobei Dec 21, 2018

paliarush Dec 21, 2018

lenaorobei Dec 21, 2018

paliarush Dec 21, 2018

lenaorobei Dec 21, 2018

paliarush Dec 21, 2018

lenaorobei Dec 21, 2018

paliarush Dec 21, 2018

lenaorobei Dec 21, 2018

		# Magento Coding Standard

		This is just the beginning of the coding standards consolidation process.


		1. Make it easier to run static checks for core project contributors and extensions developers.
		2. Store all related to Magento 2 sniffs in one place.

	2. Store all related to Magento 2 sniffs in one place.
	2. Store all Magento 2 sniffs in one place.


		function emptyBlockT () { return true; }

		interface EmptyBlockInterface { /Empty interface block/ }

	$html = preg_replace('(<h([1-6])>(.*?)</h\1>)sex', '"<h$1>" . strtoupper("$2") . "</h$1>"', $html);
	$html = preg_replace('(<h([1-6])>(.*?)</h\1>)xes', '"<h$1>" . strtoupper("$2") . "</h$1>"', $html);

MM-4941: [EQP][Sniffs Consolidation] Create new GitHub repo and move MEQP2 sniffs #1

MM-4941: [EQP][Sniffs Consolidation] Create new GitHub repo and move MEQP2 sniffs #1

Conversation

lenaorobei commented Dec 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment