From 753724ce599b4960debd8ab11215719d1a26e5f1 Mon Sep 17 00:00:00 2001 From: Thomas Klein Date: Mon, 9 Apr 2018 21:42:50 +0200 Subject: [PATCH 1/3] Code cleanup, add more visibility --- .../Model/Condition/AbstractCondition.php | 98 +++++++------------ 1 file changed, 34 insertions(+), 64 deletions(-) diff --git a/app/code/Magento/Rule/Model/Condition/AbstractCondition.php b/app/code/Magento/Rule/Model/Condition/AbstractCondition.php index 01e715f06a27f..e41620c953412 100644 --- a/app/code/Magento/Rule/Model/Condition/AbstractCondition.php +++ b/app/code/Magento/Rule/Model/Condition/AbstractCondition.php @@ -24,7 +24,6 @@ abstract class AbstractCondition extends \Magento\Framework\DataObject implement { /** * Defines which operators will be available for this condition - * * @var string */ protected $_inputType = null; @@ -84,17 +83,11 @@ public function __construct(Context $context, array $data = []) $options = $this->getAttributeOptions(); if ($options) { - foreach (array_keys($options) as $attr) { - $this->setAttribute($attr); - break; - } + $this->setAttribute(key($options)); } $options = $this->getOperatorOptions(); if ($options) { - foreach (array_keys($options) as $operator) { - $this->setOperator($operator); - break; - } + $this->setOperator(key($options)); } } @@ -160,14 +153,13 @@ public function getForm() */ public function asArray(array $arrAttributes = []) { - $out = [ + return [ 'type' => $this->getType(), 'attribute' => $this->getAttribute(), 'operator' => $this->getOperator(), 'value' => $this->getValue(), 'is_value_processed' => $this->getIsValueParsed(), ]; - return $out; } /** @@ -205,7 +197,7 @@ public function getMappedSqlField() */ public function asXml() { - $xml = "" . + return "" . $this->getType() . "" . "" . @@ -217,7 +209,6 @@ public function asXml() "" . $this->getValue() . ""; - return $xml; } /** @@ -244,8 +235,7 @@ public function loadXml($xml) if (is_string($xml)) { $xml = simplexml_load_string($xml); } - $arr = (array)$xml; - $this->loadArray($arr); + $this->loadArray((array)$xml); return $this; } @@ -304,10 +294,7 @@ public function loadOperatorOptions() */ public function getInputType() { - if (null === $this->_inputType) { - return 'string'; - } - return $this->_inputType; + return null === $this->_inputType ? 'string' : $this->_inputType; } /** @@ -348,12 +335,11 @@ public function loadValueOptions() */ public function getValueSelectOptions() { - $valueOption = $opt = []; + $opt = []; if ($this->hasValueOption()) { - $valueOption = (array)$this->getValueOption(); - } - foreach ($valueOption as $key => $value) { - $opt[] = ['value' => $key, 'label' => $value]; + foreach ((array)$this->getValueOption() as $key => $value) { + $opt[] = ['value' => $key, 'label' => $value]; + } } return $opt; } @@ -470,13 +456,12 @@ public function getNewChildName() */ public function asHtml() { - $html = $this->getTypeElementHtml() . + return $this->getTypeElementHtml() . $this->getAttributeElementHtml() . $this->getOperatorElementHtml() . $this->getValueElementHtml() . $this->getRemoveLinkHtml() . $this->getChooserContainerHtml(); - return $html; } /** @@ -484,8 +469,7 @@ public function asHtml() */ public function asHtmlRecursive() { - $html = $this->asHtml(); - return $html; + return $this->asHtml(); } /** @@ -520,9 +504,9 @@ public function getTypeElementHtml() public function getAttributeElement() { if (null === $this->getAttribute()) { - foreach (array_keys($this->getAttributeOption()) as $option) { - $this->setAttribute($option); - break; + $options = $this->getAttributeOptions(); + if ($options) { + $this->setAttribute(key($options)); } } return $this->getForm()->addField( @@ -558,10 +542,8 @@ public function getOperatorElement() { $options = $this->getOperatorSelectOptions(); if ($this->getOperator() === null) { - foreach ($options as $option) { - $this->setOperator($option['value']); - break; - } + $option = current($options); + $this->setOperator($option['value']); } $elementId = sprintf('%s__%s__operator', $this->getPrefix(), $this->getId()); @@ -654,8 +636,7 @@ public function getValueElementHtml() public function getAddLinkHtml() { $src = $this->_assetRepo->getUrl('images/rule_component_add.gif'); - $html = ''; - return $html; + return ''; } /** @@ -676,11 +657,7 @@ public function getRemoveLinkHtml() public function getChooserContainerHtml() { $url = $this->getValueElementChooserUrl(); - $html = ''; - if ($url) { - $html = '
'; - } - return $html; + return $url ? '
' : ''; } /** @@ -690,8 +667,7 @@ public function getChooserContainerHtml() */ public function asString($format = '') { - $str = $this->getAttributeName() . ' ' . $this->getOperatorName() . ' ' . $this->getValueName(); - return $str; + return $this->getAttributeName() . ' ' . $this->getOperatorName() . ' ' . $this->getValueName(); } /** @@ -700,8 +676,7 @@ public function asString($format = '') */ public function asStringRecursive($level = 0) { - $str = str_pad('', $level * 3, ' ', STR_PAD_LEFT) . $this->asString(); - return $str; + return str_pad('', $level * 3, ' ', STR_PAD_LEFT) . $this->asString(); } /** @@ -740,12 +715,10 @@ public function validateAttribute($validatedValue) case '==': case '!=': if (is_array($value)) { - if (is_array($validatedValue)) { - $result = array_intersect($value, $validatedValue); - $result = !empty($result); - } else { + if (!is_array($validatedValue)) { return false; } + $result = !empty(array_intersect($value, $validatedValue)); } else { if (is_array($validatedValue)) { $result = count($validatedValue) == 1 && array_shift($validatedValue) == $value; @@ -759,18 +732,16 @@ public function validateAttribute($validatedValue) case '>': if (!is_scalar($validatedValue)) { return false; - } else { - $result = $validatedValue <= $value; } + $result = $validatedValue <= $value; break; case '>=': case '<': if (!is_scalar($validatedValue)) { return false; - } else { - $result = $validatedValue >= $value; } + $result = $validatedValue >= $value; break; case '{}': @@ -783,12 +754,11 @@ public function validateAttribute($validatedValue) } } } elseif (is_array($value)) { - if (is_array($validatedValue)) { - $result = array_intersect($value, $validatedValue); - $result = !empty($result); - } else { + if (!is_array($validatedValue)) { return false; } + $result = array_intersect($value, $validatedValue); + $result = !empty($result); } else { if (is_array($validatedValue)) { $result = in_array($value, $validatedValue); @@ -833,13 +803,13 @@ protected function _compareValues($validatedValue, $value, $strict = true) { if ($strict && is_numeric($validatedValue) && is_numeric($value)) { return $validatedValue == $value; - } else { - $validatePattern = preg_quote($validatedValue, '~'); - if ($strict) { - $validatePattern = '^' . $validatePattern . '$'; - } - return (bool)preg_match('~' . $validatePattern . '~iu', $value); } + + $validatePattern = preg_quote($validatedValue, '~'); + if ($strict) { + $validatePattern = '^' . $validatePattern . '$'; + } + return (bool)preg_match('~' . $validatePattern . '~iu', $value); } /** From 8517aa7429eb189cfcb6970d9faa4627d8b26d72 Mon Sep 17 00:00:00 2001 From: Stanislav Idolov Date: Sat, 28 Apr 2018 11:35:01 +0300 Subject: [PATCH 2/3] Fixed typo and logic mismatch --- .../Magento/Rule/Model/Condition/AbstractCondition.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/code/Magento/Rule/Model/Condition/AbstractCondition.php b/app/code/Magento/Rule/Model/Condition/AbstractCondition.php index e41620c953412..192b0e40d9983 100644 --- a/app/code/Magento/Rule/Model/Condition/AbstractCondition.php +++ b/app/code/Magento/Rule/Model/Condition/AbstractCondition.php @@ -83,11 +83,11 @@ public function __construct(Context $context, array $data = []) $options = $this->getAttributeOptions(); if ($options) { - $this->setAttribute(key($options)); + $this->setAttribute(key(reset($options))); } $options = $this->getOperatorOptions(); if ($options) { - $this->setOperator(key($options)); + $this->setOperator(key(reset($options))); } } @@ -504,9 +504,9 @@ public function getTypeElementHtml() public function getAttributeElement() { if (null === $this->getAttribute()) { - $options = $this->getAttributeOptions(); + $options = $this->getAttributeOption(); if ($options) { - $this->setAttribute(key($options)); + $this->setAttribute(key(reset($options))); } } return $this->getForm()->addField( @@ -542,7 +542,7 @@ public function getOperatorElement() { $options = $this->getOperatorSelectOptions(); if ($this->getOperator() === null) { - $option = current($options); + $option = current(reset($options)); $this->setOperator($option['value']); } From 6a4d654745ea592f361566b9aeb7f176c831a1af Mon Sep 17 00:00:00 2001 From: Stanislav Idolov Date: Sat, 28 Apr 2018 15:57:11 +0300 Subject: [PATCH 3/3] Fixed reset method usage --- .../Rule/Model/Condition/AbstractCondition.php | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/app/code/Magento/Rule/Model/Condition/AbstractCondition.php b/app/code/Magento/Rule/Model/Condition/AbstractCondition.php index 192b0e40d9983..e3bec2d9959b1 100644 --- a/app/code/Magento/Rule/Model/Condition/AbstractCondition.php +++ b/app/code/Magento/Rule/Model/Condition/AbstractCondition.php @@ -83,11 +83,13 @@ public function __construct(Context $context, array $data = []) $options = $this->getAttributeOptions(); if ($options) { - $this->setAttribute(key(reset($options))); + reset($options); + $this->setAttribute(key($options)); } $options = $this->getOperatorOptions(); if ($options) { - $this->setOperator(key(reset($options))); + reset($options); + $this->setOperator(key($options)); } } @@ -506,7 +508,8 @@ public function getAttributeElement() if (null === $this->getAttribute()) { $options = $this->getAttributeOption(); if ($options) { - $this->setAttribute(key(reset($options))); + reset($options); + $this->setAttribute(key($options)); } } return $this->getForm()->addField( @@ -542,7 +545,7 @@ public function getOperatorElement() { $options = $this->getOperatorSelectOptions(); if ($this->getOperator() === null) { - $option = current(reset($options)); + $option = reset($options); $this->setOperator($option['value']); }