MQE-590: Fix PHP Mess Detector Errors

KevinBKozan · KevinBKozan · commit c8e48d87b8b6 · 2017-12-04T15:21:33.000-06:00
- First batch, mostly @SupressWarnings due to PHPMD hating singletons, and classes that we already have stories for refactoring. - TestGenerator was edited somewhat to actually fix the issues, no huge refactoring though.
diff --git a/dev/tests/static/Magento/CodeMessDetector/ruleset.xml b/dev/tests/static/Magento/CodeMessDetector/ruleset.xml
@@ -26,7 +26,11 @@
     </rule>
 
     <!-- Unused code rules -->
-    <rule ref="rulesets/unusedcode.xml" />
+    <rule ref="rulesets/unusedcode.xml" >
+        <properties>
+            <property name="allow-unused-foreach-variables" value="true"/>
+        </properties>
+    </rule>
 
     <!-- Code design rules -->
     <rule ref="rulesets/design.xml/ExitExpression" />
diff --git a/src/Magento/FunctionalTestingFramework/Config/Converter.php b/src/Magento/FunctionalTestingFramework/Config/Converter.php
@@ -82,6 +82,7 @@ public function convert($source)
      *
      * @param \DOMNodeList|array $elements
      * @return array
+     * @SuppressWarnings(PHPMD.CyclomaticComplexity)
      */
     protected function convertXml($elements)
     {
diff --git a/src/Magento/FunctionalTestingFramework/Config/Dom.php b/src/Magento/FunctionalTestingFramework/Config/Dom.php
@@ -114,6 +114,7 @@ public function merge($xml)
      * @param string $parentPath path to parent node
      * @return void
      * @SuppressWarnings(PHPMD.CyclomaticComplexity)
+     * @SuppressWarnings(PHPMD.NPathComplexity)
      */
     protected function mergeNode(\DOMElement $node, $parentPath)
     {
diff --git a/src/Magento/FunctionalTestingFramework/DataGenerator/Handlers/DataObjectHandler.php b/src/Magento/FunctionalTestingFramework/DataGenerator/Handlers/DataObjectHandler.php
@@ -109,6 +109,7 @@ public function getAllObjects()
      * Method to initialize parsing of data.xml and read into objects.
      *
      * @return void
+     * @SuppressWarnings(PHPMD.UnusedPrivateMethod)
      */
     private function initDataObjects()
     {
diff --git a/src/Magento/FunctionalTestingFramework/DataGenerator/Handlers/OperationDefinitionObjectHandler.php b/src/Magento/FunctionalTestingFramework/DataGenerator/Handlers/OperationDefinitionObjectHandler.php
@@ -122,9 +122,13 @@ public function getOperationDefinition($operation, $dataType)
     /**
      * This method reads all dataDefinitions from metadata xml into memory.
      * @return void
+     * @SuppressWarnings(PHPMD.CyclomaticComplexity)
+     * @SuppressWarnings(PHPMD.NPathComplexity)
+     * @SuppressWarnings(PHPMD.UnusedPrivateMethod)
      */
     private function initDataDefinitions()
     {
+        //TODO: Reduce CyclomaticComplexity/NPathComplexity of method, remove warning suppression.
         $objectManager = ObjectManagerFactory::getObjectManager();
         $metadataParser = $objectManager->create(OperationDefinitionParser::class);
         foreach ($metadataParser->readOperationMetadata()
diff --git a/src/Magento/FunctionalTestingFramework/DataGenerator/Objects/OperationElement.php b/src/Magento/FunctionalTestingFramework/DataGenerator/Objects/OperationElement.php
@@ -107,7 +107,7 @@ public function getType()
      *
      * @return bool
      */
-    public function getRequired()
+    public function isRequired()
     {
         return $this->required;
     }
diff --git a/src/Magento/FunctionalTestingFramework/DataGenerator/Persist/OperationDataArrayResolver.php b/src/Magento/FunctionalTestingFramework/DataGenerator/Persist/OperationDataArrayResolver.php
@@ -73,7 +73,7 @@ public function resolveOperationDataArray($entityObject, $operationMetadata, $op
         foreach ($operationMetadata as $operationElement) {
             if ($operationElement->getType() == OperationElementExtractor::OPERATION_OBJECT_OBJ_NAME) {
                 $entityObj = $this->resolveOperationObjectAndEntityData($entityObject, $operationElement->getValue());
-                if (null === $entityObj && $operationElement->getRequired()) {
+                if (null === $entityObj && $operationElement->isRequired()) {
                     throw new \Exception(sprintf(
                         self::EXCEPTION_REQUIRED_DATA,
                         $operationElement->getType(),
@@ -123,7 +123,7 @@ public function resolveOperationDataArray($entityObject, $operationMetadata, $op
                         $elementData
                     );
 
-                } elseif ($operationElement->getRequired()) {
+                } elseif ($operationElement->isRequired()) {
                     throw new \Exception(sprintf(
                         self::EXCEPTION_REQUIRED_DATA,
                         $operationElement->getType(),
@@ -135,7 +135,7 @@ public function resolveOperationDataArray($entityObject, $operationMetadata, $op
                 $entityNamesOfType = $entityObject->getLinkedEntitiesOfType($operationElementType);
 
                 // If an element is required by metadata, but was not provided in the entity, throw an exception
-                if ($operationElement->getRequired() && $entityNamesOfType == null) {
+                if ($operationElement->isRequired() && $entityNamesOfType == null) {
                     throw new \Exception(sprintf(
                         self::EXCEPTION_REQUIRED_DATA,
                         $operationElement->getType(),
diff --git a/src/Magento/FunctionalTestingFramework/Helper/AdminUrlList.php b/src/Magento/FunctionalTestingFramework/Helper/AdminUrlList.php
@@ -8,6 +8,7 @@
 
 /**
  * Class AdminUrlList
+ * @SuppressWarnings(PHPMD)
  */
 // @codingStandardsIgnoreFile
 class AdminUrlList
diff --git a/src/Magento/FunctionalTestingFramework/Helper/EntityRESTApiHelper.php b/src/Magento/FunctionalTestingFramework/Helper/EntityRESTApiHelper.php
@@ -39,7 +39,7 @@ class EntityRESTApiHelper
     public function __construct($host, $port)
     {
         $this->guzzle_client = new Client([
-            'base_uri' => "http://${host}:${port}",
+            'base_uri' => "http://{$host}:{$port}",
             'timeout' => 5.0,
         ]);
     }
diff --git a/src/Magento/FunctionalTestingFramework/Helper/MagentoFakerData.php b/src/Magento/FunctionalTestingFramework/Helper/MagentoFakerData.php
@@ -27,7 +27,7 @@ public function getCustomerData(array $additional = [])
             'lastname' => $faker->lastName,
             'suffix' => \Faker\Provider\en_US\Person::suffix(),
             'email' => $faker->email,
-            'dateOfBirth' => $faker->date($format = 'm/d/Y', $max = 'now'),
+            'dateOfBirth' => $faker->date('m/d/Y', 'now'),
             'gender' => rand(0, 1),
             'group_id' => 1,
             'store_id' => 1,
@@ -61,38 +61,36 @@ public function getCategoryData()
             'includeInMenu' => $faker->boolean(),
             'categoryName' => $faker->md5,
             'categoryImage' => '',
-            'description' => $faker->sentence($nbWords = 10, $variableNbWords = true),
+            'description' => $faker->sentence(10, true),
             'addCMSBlock' => '',
 
             'urlKey' => $faker->uuid,
             'metaTitle' => $faker->word,
-            'metaKeywords' => $faker->sentence($nbWords = 5, $variableNbWords = true),
-            'metaDescription' => $faker->sentence($nbWords = 10, $variableNbWords = true),
+            'metaKeywords' => $faker->sentence(5, true),
+            'metaDescription' => $faker->sentence(10, true),
         ];
     }
 
     /**
      * Get simple product data.
      *
-     * @param integer $categoryId
-     * @param array $productData
      * @return array
      */
-    public function getProductData($categoryId = 0, $productData = [])
+    public function getProductData()
     {
         $faker = \Faker\Factory::create();
         return [
             'enableProduct' => $faker->boolean(),
             'attributeSet' => '',
-            'productName' => $faker->text($maxNbChars = 20),
-            'sku' => \Faker\Provider\DateTime::unixTime($max = 'now'),
-            'price' => $faker->randomFloat($nbMaxDecimals = 2, $min = 0, $max = 999),
-            'quantity' => $faker->numberBetween($min = 1, $max = 999),
+            'productName' => $faker->text(20),
+            'sku' => \Faker\Provider\DateTime::unixTime('now'),
+            'price' => $faker->randomFloat(2, 0, 999),
+            'quantity' => $faker->numberBetween(1, 999),
 
             'urlKey' => $faker->uuid,
             'metaTitle' => $faker->word,
-            'metaKeywords' => $faker->sentence($nbWords = 5, $variableNbWords = true),
-            'metaDescription' => $faker->sentence($nbWords = 10, $variableNbWords = true)
+            'metaKeywords' => $faker->sentence(5, true),
+            'metaDescription' => $faker->sentence(10, true)
         ];
     }
 
@@ -106,14 +104,14 @@ public function getContentPage()
         $faker = \Faker\Factory::create();
 
         $pageContent = [
-            'pageTitle' => $faker->sentence($nbWords = 3, $variableNbWords = true),
-            'contentHeading' => $faker->sentence($nbWords = 3, $variableNbWords = true),
-            'contentBody' => $faker->sentence($nbWords = 10, $variableNbWords = true),
+            'pageTitle' => $faker->sentence(3, true),
+            'contentHeading' => $faker->sentence(3, true),
+            'contentBody' => $faker->sentence(10, true),
             'urlKey' => $faker->uuid,
             'metaTitle' => $faker->word,
-            'metaKeywords' => $faker->sentence($nbWords = 5, $variableNbWords = true),
-            'metaDescription' => $faker->sentence($nbWords = 10, $variableNbWords = true),
-            'from' => $faker->date($format = 'm/d/Y', $max = 'now'),
+            'metaKeywords' => $faker->sentence(5, true),
+            'metaDescription' => $faker->sentence(10, true),
+            'from' => $faker->date($format = 'm/d/Y', 'now'),
             'to' => $faker->date($format = 'm/d/Y')
         ];
         $pageContent['layoutUpdateXml'] = "<note><to>Tove</to><from>Jani</from><heading>Reminder</heading>";
diff --git a/src/Magento/FunctionalTestingFramework/Module/MagentoRestDriver.php b/src/Magento/FunctionalTestingFramework/Module/MagentoRestDriver.php
@@ -433,7 +433,7 @@ public function getCategoryApiData($categoryData = [])
                     ['attribute_code' => 'custom_design', 'value' => ''],
                     ['attribute_code' => 'page_layout', 'value' => ''],
                     ['attribute_code' => 'custom_design_to', 'value' => $faker->date($format = 'm/d/Y')],
-                    ['attribute_code' => 'custom_design_from', 'value' => $faker->date($format = 'm/d/Y', $max = 'now')]
+                    ['attribute_code' => 'custom_design_from', 'value' => $faker->date($format = 'm/d/Y', 'now')]
                 ]
             ],
             $categoryData
diff --git a/src/Magento/FunctionalTestingFramework/Page/Handlers/PageObjectHandler.php b/src/Magento/FunctionalTestingFramework/Page/Handlers/PageObjectHandler.php
@@ -88,6 +88,7 @@ public function getAllObjects()
      * Executes parser code to read in page xml data.
      *
      * @return void
+     * @SuppressWarnings(PHPMD.UnusedPrivateMethod)
      */
     private function initPageObjects()
     {
diff --git a/src/Magento/FunctionalTestingFramework/Page/Handlers/SectionObjectHandler.php b/src/Magento/FunctionalTestingFramework/Page/Handlers/SectionObjectHandler.php
@@ -92,6 +92,7 @@ public function getAllObjects()
      * Parse section objects if it's not previously done.
      *
      * @return void
+     * @SuppressWarnings(PHPMD.UnusedPrivateMethod)
      */
     private function initSectionObjects()
     {
diff --git a/src/Magento/FunctionalTestingFramework/Step/Backend/AdminStep.php b/src/Magento/FunctionalTestingFramework/Step/Backend/AdminStep.php
@@ -8,6 +8,10 @@
 
 require_once __DIR__ . '/../../Helper/AdminUrlList.php';
 
+/**
+ * Class AdminStep
+ * @SuppressWarnings(PHPMD)
+ */
 // @codingStandardsIgnoreFile
 class AdminStep extends \Magento\FunctionalTestingFramework\AcceptanceTester
 {
diff --git a/src/Magento/FunctionalTestingFramework/Suite/Handlers/SuiteObjectHandler.php b/src/Magento/FunctionalTestingFramework/Suite/Handlers/SuiteObjectHandler.php
@@ -87,6 +87,7 @@ public function getAllObjects()
      * Method to parse all suite data xml into objects.
      *
      * @return void
+     * @SuppressWarnings(PHPMD.UnusedPrivateMethod)
      */
     private function initSuiteData()
     {
diff --git a/src/Magento/FunctionalTestingFramework/Test/Handlers/ActionGroupObjectHandler.php b/src/Magento/FunctionalTestingFramework/Test/Handlers/ActionGroupObjectHandler.php
@@ -87,6 +87,7 @@ public function getAllObjects()
      * Method which populates field array with objects from parsed action_group.xml
      *
      * @return void
+     * @SuppressWarnings(PHPMD.UnusedPrivateMethod)
      */
     private function initActionGroups()
     {
diff --git a/src/Magento/FunctionalTestingFramework/Test/Handlers/CestObjectHandler.php b/src/Magento/FunctionalTestingFramework/Test/Handlers/CestObjectHandler.php
@@ -123,6 +123,7 @@ public function getCestsByGroup($groupName)
      * This method reads all Cest.xml files into objects and stores them in an array for future access.
      *
      * @return void
+     * @SuppressWarnings(PHPMD.UnusedPrivateMethod)
      */
     private function initCestData()
     {
diff --git a/src/Magento/FunctionalTestingFramework/Util/TestGenerator.php b/src/Magento/FunctionalTestingFramework/Util/TestGenerator.php
@@ -14,6 +14,10 @@
 use Magento\FunctionalTestingFramework\Test\Objects\CestObject;
 use Magento\FunctionalTestingFramework\Util\Filesystem\DirSetupUtil;
 
+/**
+ * Class TestGenerator
+ * @SuppressWarnings(PHPMD.ExcessiveClassComplexity)
+ */
 class TestGenerator
 {
 
@@ -187,8 +191,7 @@ private function assembleAllCestPhp($testManifest)
         $cestPhpArray = [];
 
         foreach ($cestObjects as $cest) {
-            $name = $cest->getName();
-            $name = $string = str_replace(' ', '', $name);
+            $name = str_replace(' ', '', $cest->getName());
             $php = $this->assembleCestPhp($cest);
             $cestPhpArray[] = [$name, $php];
 
@@ -239,9 +242,12 @@ private function generateUseStatementsPhp()
      * @param array $annotationsObject
      * @param string $scope
      * @return string
+     * @SuppressWarnings(PHPMD.CyclomaticComplexity)
+     * @SuppressWarnings(PHPMD.NPathComplexity)
      */
     private function generateAnnotationsPhp($annotationsObject, $scope)
     {
+        //TODO: Refactor to deal with PHPMD.CyclomaticComplexity
         if ($scope == self::TEST_SCOPE) {
             $indent = "\t";
         } else {
@@ -333,12 +339,13 @@ private function generateAnnotationsPhp($annotationsObject, $scope)
      * statement to handle each unique action. At the bottom of the case statement there is a generic function that can
      * construct the PHP string for nearly half of all Codeception actions.
      * @param array $stepsObject
-     * @param array $stepsData
      * @param array|bool $hookObject
      * @return string
+     * @SuppressWarnings(PHPMD)
      */
-    private function generateStepsPhp($stepsObject, $stepsData, $hookObject = false)
+    private function generateStepsPhp($stepsObject, $hookObject = false)
     {
+        //TODO: Refactor Method according to PHPMD warnings, remove @SuppressWarnings accordingly.
         $testSteps = "";
 
         foreach ($stepsObject as $steps) {
@@ -1221,22 +1228,14 @@ private function generateHooksPhp($hookObjects)
             try {
                 $steps = $this->generateStepsPhp(
                     $hookObject->getActions(),
-                    $hookObject->getCustomData(),
                     $createData
                 );
             } catch (TestReferenceException $e) {
                 throw new TestReferenceException($e->getMessage() . " in Element \"" . $type . "\"");
             }
 
-            if ($type == "after") {
-                $hooks .= sprintf("\tpublic function _after(%s)\n", $dependencies);
-                $hooks .= "\t{\n";
-                $hooks .= $steps;
-                $hooks .= "\t}\n\n";
-            }
-
-            if ($type == "before") {
-                $hooks .= sprintf("\tpublic function _before(%s)\n", $dependencies);
+            if ($type == "after" or $type == "before") {
+                $hooks .= sprintf("\tpublic function _{$type}(%s)\n", $dependencies);
                 $hooks .= "\t{\n";
                 $hooks .= $steps;
                 $hooks .= "\t}\n\n";
@@ -1265,7 +1264,7 @@ private function generateTestsPhp($testsObject)
             $testAnnotations = $this->generateAnnotationsPhp($test->getAnnotations(), "Test");
             $dependencies = 'AcceptanceTester $I';
             try {
-                $steps = $this->generateStepsPhp($test->getOrderedActions(), $test->getCustomData());
+                $steps = $this->generateStepsPhp($test->getOrderedActions());
             } catch (TestReferenceException $e) {
                 throw new TestReferenceException($e->getMessage() . " in Test \"" . $test->getName() . "\"");
             }
@@ -1473,9 +1472,11 @@ private function validateParameterArray($paramArray)
      * @param string $value
      * @param string $type
      * @return string
+     * @SuppressWarnings(PHPMD.CyclomaticComplexity)
      */
     private function resolveValueByType($value, $type)
     {
+        //TODO: Refactor to deal with PHPMD.CyclomaticComplexity, and remove @SuppressWarnings
         if (null === $value) {
             return null;
         }

Original file line number	Diff line number	Diff line change
`@@ -82,6 +82,7 @@ public function convert($source)`
`82`	`82`	`*`
`83`	`83`	`* @param \DOMNodeList\|array $elements`
`84`	`84`	`* @return array`
	`85`	`+ * @SuppressWarnings(PHPMD.CyclomaticComplexity)`
`85`	`86`	`*/`
`86`	`87`	`protected function convertXml($elements)`
`87`	`88`	`{`
Original file line number	Diff line number	Diff line change
`@@ -114,6 +114,7 @@ public function merge($xml)`
`114`	`114`	`* @param string $parentPath path to parent node`
`115`	`115`	`* @return void`
`116`	`116`	`* @SuppressWarnings(PHPMD.CyclomaticComplexity)`
	`117`	`+ * @SuppressWarnings(PHPMD.NPathComplexity)`
`117`	`118`	`*/`
`118`	`119`	`protected function mergeNode(\DOMElement $node, $parentPath)`
`119`	`120`	`{`
Original file line number	Diff line number	Diff line change
`@@ -109,6 +109,7 @@ public function getAllObjects()`
`109`	`109`	`* Method to initialize parsing of data.xml and read into objects.`
`110`	`110`	`*`
`111`	`111`	`* @return void`
	`112`	`+ * @SuppressWarnings(PHPMD.UnusedPrivateMethod)`
`112`	`113`	`*/`
`113`	`114`	`private function initDataObjects()`
`114`	`115`	`{`
Original file line number	Diff line number	Diff line change
`@@ -107,7 +107,7 @@ public function getType()`
`107`	`107`	`*`
`108`	`108`	`* @return bool`
`109`	`109`	`*/`
`110`		`- public function getRequired()`
	`110`	`+ public function isRequired()`
`111`	`111`	`{`
`112`	`112`	`return $this->required;`
`113`	`113`	`}`
Original file line number	Diff line number	Diff line change
`@@ -39,7 +39,7 @@ class EntityRESTApiHelper`
`39`	`39`	`public function __construct($host, $port)`
`40`	`40`	`{`
`41`	`41`	`$this->guzzle_client = new Client([`
`42`		`- 'base_uri' => "http://${host}:${port}",`
	`42`	`+ 'base_uri' => "http://{$host}:{$port}",`
`43`	`43`	`'timeout' => 5.0,`
`44`	`44`	`]);`
`45`	`45`	`}`