Skip to content

Commit b87e50c

Browse files
KevinBKozanKevinBKozan
authored
MQE-590: Fix PHP Mess Detector Errors
- Refactored classes to deal with complexity where possible. PHPMD no longer fails. - Added SuppressWarning tags to classes that could not be done in scope of story, created MQE-610 to deal with TODOs inserted.
2 parents 1ce05f7 + e42fc90 commit b87e50c

28 files changed

+272
-156
lines changed

‎bin/static-checks

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,11 @@ echo "==============================="
1717
vendor/bin/phpcpd ./src
1818
echo ""
1919

20-
# Uncomment lines as part of MQE-590
21-
# echo "==============================="
22-
# echo " MESS DETECTOR"
23-
# echo "==============================="
24-
# vendor/bin/phpmd ./src text /dev/tests/static/Magento/CodeMessDetector/ruleset.xml --exclude _generated
25-
# echo ""
20+
echo "==============================="
21+
echo " MESS DETECTOR"
22+
echo "==============================="
23+
vendor/bin/phpmd ./src text /dev/tests/static/Magento/CodeMessDetector/ruleset.xml --exclude _generated
24+
echo ""
2625

2726
echo "==============================="
2827
echo " MAGENTO COPYRIGHT CHECK"

‎bin/static-checks.bat

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,8 @@ call vendor\bin\phpcs .\dev\tests\verification --standard=.\dev\tests\static\Mag
1010
@echo ===============================COPY PASTE DETECTOR REPORT===============================
1111
call vendor\bin\phpcpd .\src
1212

13-
:: Uncomment lines as part of MQE-590
14-
:: @echo "===============================PHP MESS DETECTOR REPORT===============================
15-
:: vendor\bin\phpmd .\src text \dev\tests\static\Magento\CodeMessDetector\ruleset.xml --exclude _generated
13+
@echo "===============================PHP MESS DETECTOR REPORT===============================
14+
vendor\bin\phpmd .\src text \dev\tests\static\Magento\CodeMessDetector\ruleset.xml --exclude _generated
1615

1716
@echo ===============================MAGENTO COPYRIGHT REPORT===============================
1817
echo msgbox "INFO:Copyright check currently not run as part of .bat implementation" > "%temp%\popup.vbs"

‎dev/tests/static/Magento/CodeMessDetector/ruleset.xml

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,14 @@
2626
</rule>
2727

2828
<!-- Unused code rules -->
29-
<rule ref="rulesets/unusedcode.xml" />
29+
<rule ref="rulesets/unusedcode.xml/UnusedPrivateField" />
30+
<rule ref="rulesets/unusedcode.xml/UnusedPrivateMethod" />
31+
<rule ref="rulesets/unusedcode.xml/UnusedFormalParameter" />
32+
<rule ref="rulesets/unusedcode.xml/UnusedLocalVariable" >
33+
<properties>
34+
<property name="allow-unused-foreach-variables" value="true"/>
35+
</properties>
36+
</rule>
3037

3138
<!-- Code design rules -->
3239
<rule ref="rulesets/design.xml/ExitExpression" />

‎src/Magento/FunctionalTestingFramework/Config/Converter.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,7 @@ public function convert($source)
8282
*
8383
* @param \DOMNodeList|array $elements
8484
* @return array
85+
* @SuppressWarnings(PHPMD.CyclomaticComplexity)
8586
*/
8687
protected function convertXml($elements)
8788
{

‎src/Magento/FunctionalTestingFramework/Config/Dom.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,7 @@ public function merge($xml)
114114
* @param string $parentPath path to parent node
115115
* @return void
116116
* @SuppressWarnings(PHPMD.CyclomaticComplexity)
117+
* @SuppressWarnings(PHPMD.NPathComplexity)
117118
*/
118119
protected function mergeNode(\DOMElement $node, $parentPath)
119120
{

‎src/Magento/FunctionalTestingFramework/Config/FileResolver/Module.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ class Module implements FileResolverInterface
2525
/**
2626
* Module constructor.
2727
* @param ModuleResolver|null $moduleResolver
28+
* @SuppressWarnings(PHPMD.UnusedFormalParameter)
2829
*/
2930
public function __construct(ModuleResolver $moduleResolver = null)
3031
{

‎src/Magento/FunctionalTestingFramework/DataGenerator/Handlers/DataObjectHandler.php

Lines changed: 71 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,7 @@ public function getAllObjects()
109109
* Method to initialize parsing of data.xml and read into objects.
110110
*
111111
* @return void
112+
* @SuppressWarnings(PHPMD.UnusedPrivateMethod)
112113
*/
113114
private function initDataObjects()
114115
{
@@ -172,26 +173,12 @@ private function parseDataEntities()
172173
$uniquenessValues = [];
173174

174175
if (array_key_exists(self::DATA_VALUES, $entity)) {
175-
foreach ($entity[self::DATA_VALUES] as $dataElement) {
176-
$dataElementKey = strtolower($dataElement[self::DATA_ELEMENT_KEY]);
177-
$dataElementValue = $dataElement[self::DATA_ELEMENT_VALUE] ?? "";
178-
if (array_key_exists(self::DATA_ELEMENT_UNIQUENESS_ATTR, $dataElement)) {
179-
$uniquenessValues[$dataElementKey] = $dataElement[self::DATA_ELEMENT_UNIQUENESS_ATTR];
180-
}
181-
182-
$dataValues[$dataElementKey] = $dataElementValue;
183-
}
184-
unset($dataElement);
176+
$dataValues = $this->parseDataElements($entity);
177+
$uniquenessValues = $this->parseUniquenessValues($entity);
185178
}
186179

187180
if (array_key_exists(self::REQUIRED_ENTITY, $entity)) {
188-
foreach ($entity[self::REQUIRED_ENTITY] as $linkedEntity) {
189-
$linkedEntityName = $linkedEntity[self::REQUIRED_ENTITY_VALUE];
190-
$linkedEntityType = $linkedEntity[self::REQUIRED_ENTITY_TYPE];
191-
192-
$linkedEntities[$linkedEntityName] = $linkedEntityType;
193-
}
194-
unset($linkedEntity);
181+
$linkedEntities = $this->parseRequiredEntityElements($entity);
195182
}
196183

197184
if (array_key_exists(self::ARRAY_VALUES, $entity)) {
@@ -206,11 +193,7 @@ private function parseDataEntities()
206193
}
207194

208195
if (array_key_exists(self::VAR_VALUES, $entity)) {
209-
foreach ($entity[self::VAR_VALUES] as $varElement) {
210-
$varKey = $varElement[self::VAR_KEY];
211-
$varValue = $varElement[self::VAR_ENTITY] . self::VAR_ENTITY_FIELD_SEPARATOR . $varElement[self::VAR_FIELD];
212-
$vars[$varKey] = $varValue;
213-
}
196+
$vars = $this->parseVarElements($entity);
214197
}
215198

216199
$entityDataObject = new EntityDataObject(
@@ -223,9 +206,74 @@ private function parseDataEntities()
223206
);
224207

225208
$this->data[$entityDataObject->getName()] = $entityDataObject;
226-
227209
}
228210
unset($entityName);
229211
unset($entity);
230212
}
213+
214+
/**
215+
* Parses <data> elements in an entity, and returns them as an array of "lowerKey"=>value.
216+
* @param array $entityData
217+
* @return array
218+
*/
219+
private function parseDataElements($entityData)
220+
{
221+
$dataValues = [];
222+
foreach ($entityData[self::DATA_VALUES] as $dataElement) {
223+
$dataElementKey = strtolower($dataElement[self::DATA_ELEMENT_KEY]);
224+
$dataElementValue = $dataElement[self::DATA_ELEMENT_VALUE] ?? "";
225+
$dataValues[$dataElementKey] = $dataElementValue;
226+
}
227+
return $dataValues;
228+
}
229+
230+
/**
231+
* Parses through <data> elements in an entity to return an array of "DataKey" => "UniquenessAttribute"
232+
* @param array $entityData
233+
* @return array
234+
*/
235+
private function parseUniquenessValues($entityData)
236+
{
237+
$uniquenessValues = [];
238+
foreach ($entityData[self::DATA_VALUES] as $dataElement) {
239+
if (array_key_exists(self::DATA_ELEMENT_UNIQUENESS_ATTR, $dataElement)) {
240+
$dataElementKey = strtolower($dataElement[self::DATA_ELEMENT_KEY]);
241+
$uniquenessValues[$dataElementKey] = $dataElement[self::DATA_ELEMENT_UNIQUENESS_ATTR];
242+
}
243+
}
244+
return $uniquenessValues;
245+
}
246+
247+
/**
248+
* Parses <required-entity> elements given entity, and returns them as an array of "EntityValue"=>"EntityType"
249+
* @param array $entityData
250+
* @return array
251+
*/
252+
private function parseRequiredEntityElements($entityData)
253+
{
254+
$linkedEntities = [];
255+
foreach ($entityData[self::REQUIRED_ENTITY] as $linkedEntity) {
256+
$linkedEntityName = $linkedEntity[self::REQUIRED_ENTITY_VALUE];
257+
$linkedEntityType = $linkedEntity[self::REQUIRED_ENTITY_TYPE];
258+
259+
$linkedEntities[$linkedEntityName] = $linkedEntityType;
260+
}
261+
return $linkedEntities;
262+
}
263+
264+
/**
265+
* Parses <var> elements in given entity, and returns them as an array of "Key"=> entityType -> entityKey
266+
* @param array $entityData
267+
* @return array
268+
*/
269+
private function parseVarElements($entityData)
270+
{
271+
$vars = [];
272+
foreach ($entityData[self::VAR_VALUES] as $varElement) {
273+
$varKey = $varElement[self::VAR_KEY];
274+
$varValue = $varElement[self::VAR_ENTITY] . self::VAR_ENTITY_FIELD_SEPARATOR . $varElement[self::VAR_FIELD];
275+
$vars[$varKey] = $varValue;
276+
}
277+
return $vars;
278+
}
231279
}

‎src/Magento/FunctionalTestingFramework/DataGenerator/Handlers/OperationDefinitionObjectHandler.php

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,9 +122,14 @@ public function getOperationDefinition($operation, $dataType)
122122
/**
123123
* This method reads all dataDefinitions from metadata xml into memory.
124124
* @return void
125+
* @SuppressWarnings(PHPMD.CyclomaticComplexity)
126+
* @SuppressWarnings(PHPMD.NPathComplexity)
127+
* @SuppressWarnings(PHPMD.UnusedPrivateMethod)
128+
* @SuppressWarnings(PHPMD.ExcessiveMethodLength)
125129
*/
126130
private function initDataDefinitions()
127131
{
132+
//TODO: Reduce CyclomaticComplexity/NPathComplexity/Length of method, remove warning suppression.
128133
$objectManager = ObjectManagerFactory::getObjectManager();
129134
$metadataParser = $objectManager->create(OperationDefinitionParser::class);
130135
foreach ($metadataParser->readOperationMetadata()

‎src/Magento/FunctionalTestingFramework/DataGenerator/Objects/EntityDataObject.php

Lines changed: 65 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -147,58 +147,76 @@ public function getDataByName($dataName, $uniDataFormat)
147147
if (null === $uniData || $uniDataFormat == self::NO_UNIQUE_PROCESS) {
148148
return $this->data[$name];
149149
}
150-
151-
switch ($uniDataFormat) {
152-
case self::SUITE_UNIQUE_VALUE:
153-
if (!function_exists(self::SUITE_UNIQUE_FUNCTION)) {
154-
throw new TestFrameworkException(
155-
sprintf(
156-
'Unique data format value: %s can only be used when running cests.\n',
157-
$uniDataFormat
158-
)
159-
);
160-
} elseif ($uniData == 'prefix') {
161-
return msqs($this->getName()) . $this->data[$name];
162-
} else { // $uniData == 'suffix'
163-
return $this->data[$name] . msqs($this->getName());
164-
}
165-
break;
166-
case self::CEST_UNIQUE_VALUE:
167-
if (!function_exists(self::CEST_UNIQUE_FUNCTION)) {
168-
throw new TestFrameworkException(
169-
sprintf(
170-
'Unique data format value: %s can only be used when running cests.\n',
171-
$uniDataFormat
172-
)
173-
);
174-
} elseif ($uniData == 'prefix') {
175-
return msq($this->getName()) . $this->data[$name];
176-
} else { // $uniData == 'suffix'
177-
return $this->data[$name] . msq($this->getName());
178-
}
179-
break;
180-
case self::SUITE_UNIQUE_NOTATION:
181-
if ($uniData == 'prefix') {
182-
return self::SUITE_UNIQUE_FUNCTION . '("' . $this->getName() . '")' . $this->data[$name];
183-
} else { // $uniData == 'suffix'
184-
return $this->data[$name] . self::SUITE_UNIQUE_FUNCTION . '("' . $this->getName() . '")';
185-
}
186-
break;
187-
case self::CEST_UNIQUE_NOTATION:
188-
if ($uniData == 'prefix') {
189-
return self::CEST_UNIQUE_FUNCTION . '("' . $this->getName() . '")' . $this->data[$name];
190-
} else { // $uniData == 'suffix'
191-
return $this->data[$name] . self::CEST_UNIQUE_FUNCTION . '("' . $this->getName() . '")';
192-
}
193-
break;
194-
default:
195-
break;
196-
}
150+
return $this->formatUniqueData($name, $uniData, $uniDataFormat);
197151
}
152+
return null;
153+
}
198154

155+
/**
156+
* Formats and returns data based on given uniqueDataFormat and prefix/suffix.
157+
* @param string $name
158+
* @param string $uniqueData
159+
* @param string $uniqueDataFormat
160+
* @return null|string
161+
*/
162+
private function formatUniqueData($name, $uniqueData, $uniqueDataFormat)
163+
{
164+
switch ($uniqueDataFormat) {
165+
case self::SUITE_UNIQUE_VALUE:
166+
$this->checkUniquenessFunctionExists(self::SUITE_UNIQUE_FUNCTION, $uniqueDataFormat);
167+
if ($uniqueData == 'prefix') {
168+
return msqs($this->getName()) . $this->data[$name];
169+
} else { // $uniData == 'suffix'
170+
return $this->data[$name] . msqs($this->getName());
171+
}
172+
break;
173+
case self::CEST_UNIQUE_VALUE:
174+
$this->checkUniquenessFunctionExists(self::CEST_UNIQUE_FUNCTION, $uniqueDataFormat);
175+
if ($uniqueData == 'prefix') {
176+
return msq($this->getName()) . $this->data[$name];
177+
} else { // $uniqueData == 'suffix'
178+
return $this->data[$name] . msq($this->getName());
179+
}
180+
break;
181+
case self::SUITE_UNIQUE_NOTATION:
182+
if ($uniqueData == 'prefix') {
183+
return self::SUITE_UNIQUE_FUNCTION . '("' . $this->getName() . '")' . $this->data[$name];
184+
} else { // $uniqueData == 'suffix'
185+
return $this->data[$name] . self::SUITE_UNIQUE_FUNCTION . '("' . $this->getName() . '")';
186+
}
187+
break;
188+
case self::CEST_UNIQUE_NOTATION:
189+
if ($uniqueData == 'prefix') {
190+
return self::CEST_UNIQUE_FUNCTION . '("' . $this->getName() . '")' . $this->data[$name];
191+
} else { // $uniqueData == 'suffix'
192+
return $this->data[$name] . self::CEST_UNIQUE_FUNCTION . '("' . $this->getName() . '")';
193+
}
194+
break;
195+
default:
196+
break;
197+
}
199198
return null;
200199
}
201200

201+
/**
202+
* Performs a check that the given uniqueness function exists, throws an exception if it doesn't.
203+
* @param string $function
204+
* @param string $uniqueDataFormat
205+
* @return void
206+
* @throws TestFrameworkException
207+
*/
208+
private function checkUniquenessFunctionExists($function, $uniqueDataFormat)
209+
{
210+
if (!function_exists($function)) {
211+
throw new TestFrameworkException(
212+
sprintf(
213+
'Unique data format value: %s can only be used when running cests.\n',
214+
$uniqueDataFormat
215+
)
216+
);
217+
}
218+
}
219+
202220
/**
203221
* Function which returns a reference to another entity (e.g. a var with entity="category" field="id" returns as
204222
* category->id)

‎src/Magento/FunctionalTestingFramework/DataGenerator/Objects/OperationDefinitionObject.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,7 @@ class OperationDefinitionObject
118118
* @param string $contentType
119119
* @param string $successRegex
120120
* @param string $returnRegex
121+
* @SuppressWarnings(PHPMD.ExcessiveParameterList)
121122
*/
122123
public function __construct(
123124
$name,

0 commit comments

Comments
 (0)