Skip to content

Avoid checking source,target type multiple times #9270

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 6 commits into from
Closed

Avoid checking source,target type multiple times #9270

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
28992f3
Merge branch 'develop' of github.com:magento/magento2 into develop
sivajik34 Apr 10, 2017
7427d0a
Merge branch 'develop' of github.com:magento/magento2 into develop
sivajik34 Apr 11, 2017
bd38b62
Merge branch 'develop' of github.com:magento/magento2 into develop
sivajik34 Apr 12, 2017
26c8c3b
Merge branch 'develop' of github.com:magento/magento2 into develop
sivajik34 Apr 13, 2017
af50fca
Merge branch 'develop' of github.com:magento/magento2 into develop
sivajik34 Apr 17, 2017
81255f8
removed duplicate checking source,target type in loop
sivajik34 Apr 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
320 changes: 232 additions & 88 deletions lib/internal/Magento/Framework/DataObject/Copy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,28 +61,240 @@ public function __construct(
*/
public function copyFieldsetToTarget($fieldset, $aspect, $source, $target, $root = 'global')
{
if (!$this->_isFieldsetInputValid($source, $target)) {
$sourceIsArray = is_array($source);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method becomes much more complex than it should be. There is duplication of logic and the complexity of the method increases.

You can do following:

  1. wrap the information about isDataObject, isExtensible, etc in some simple class, i.e. and pass this object as input to the methods, where currently object is passed.
class DataObjectTypeWrapper {
    public function __construct($object) {}

    public function getObject() {}

    public function isDataObject() {}

    ...
}
  1. Move every "if" body to a separate method (and potentially separate class) which accepts the DataOjectTypeWrapper. There are a lot of duplication of logic right now which an be avoided by re-suing those methods.

Copy link
Contributor Author

@sivajik34 sivajik34 Apr 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vrann
Regarding 2nd point:I'm not getting to avoid duplication.
we need create separate class or method for inner if loop,or outer if loop?
maybe I'm not getting your implementation thoughts on this.

<?php

/**
 * Copyright © Magento, Inc. All rights reserved.
 * See COPYING.txt for license details.
 */
/**
 * Utility class for checking type of  object
 */

namespace Magento\Framework\DataObject;

class DataObjectTypeWrapper {

    private $dataObject;

    public function __construct($object) {
        $this->dataObject = $object;
    }

    public function getObject() {
        return $this->dataObject;
    }

    public function isArray() {
        return is_array($this->dataObject);
    }

    public function isDataObject() {
        return ($this->dataObject instanceof \Magento\Framework\DataObject);
    }

    public function isExtensible() {
        return ($this->dataObject instanceof \Magento\Framework\Api\ExtensibleDataInterface);
    }

    public function isAbstractSimple() {
        return ($this->dataObject instanceof \Magento\Framework\Api\AbstractSimpleObject);
    }

}

$sourceIsDataObject = $source instanceof \Magento\Framework\DataObject;
$sourceIsExtensible = $source instanceof \Magento\Framework\Api\ExtensibleDataInterface;
$sourceIsAbstract = $source instanceof \Magento\Framework\Api\AbstractSimpleObject;
$targetIsArray = is_array($target);
$targetIsDataObject = $target instanceof \Magento\Framework\DataObject;
$targetIsExtensible = $target instanceof \Magento\Framework\Api\ExtensibleDataInterface;
$targetIsAbstract = $target instanceof \Magento\Framework\Api\AbstractSimpleObject;
if (!(($sourceIsArray || $sourceIsDataObject ||
$sourceIsExtensible ||
$sourceIsAbstract) && (
$targetIsArray || $targetIsDataObject ||
$targetIsExtensible ||
$targetIsAbstract))) {
return null;
}
$fields = $this->fieldsetConfig->getFieldset($fieldset, $root);
if ($fields === null) {
return $target;
}
$targetIsArray = is_array($target);


foreach ($fields as $code => $node) {
if (empty($node[$aspect])) {
continue;
if ($sourceIsArray) {
if ($targetIsArray) {
foreach ($fields as $code => $node) {
if (empty($node[$aspect])) {
continue;
}
$value = isset($source[$code]) ? $source[$code] : null;
$targetCode = (string) $node[$aspect];
$targetCode = $targetCode == '*' ? $code : $targetCode;

$target[$targetCode] = $value;
}
} else if ($targetIsDataObject) {
foreach ($fields as $code => $node) {
if (empty($node[$aspect])) {
continue;
}
$value = isset($source[$code]) ? $source[$code] : null;
$targetCode = (string) $node[$aspect];
$targetCode = $targetCode == '*' ? $code : $targetCode;

$target->setDataUsingMethod($targetCode, $value);
}
} else if ($targetIsExtensible) {
foreach ($fields as $code => $node) {
if (empty($node[$aspect])) {
continue;
}
$value = isset($source[$code]) ? $source[$code] : null;
$targetCode = (string) $node[$aspect];
$targetCode = $targetCode == '*' ? $code : $targetCode;

$this->setAttributeValueFromExtensibleDataObject($target, $targetCode, $value);
}
} else if ($targetIsAbstract) {
foreach ($fields as $code => $node) {
if (empty($node[$aspect])) {
continue;
}
$value = isset($source[$code]) ? $source[$code] : null;
$targetCode = (string) $node[$aspect];
$targetCode = $targetCode == '*' ? $code : $targetCode;

$target->setData($targetCode, $value);
}
} else {
throw new \InvalidArgumentException(
'Target should be array, Magento Object, ExtensibleDataInterface, or AbstractSimpleObject'
);
}
} else if ($sourceIsDataObject) {
if ($targetIsArray) {
foreach ($fields as $code => $node) {
if (empty($node[$aspect])) {
continue;
}
$value = $source->getDataUsingMethod($code);
$targetCode = (string) $node[$aspect];
$targetCode = $targetCode == '*' ? $code : $targetCode;

$value = $this->_getFieldsetFieldValue($source, $code);
$target[$targetCode] = $value;
}
} else if ($targetIsDataObject) {
foreach ($fields as $code => $node) {
if (empty($node[$aspect])) {
continue;
}
$value = $source->getDataUsingMethod($code);
$targetCode = (string) $node[$aspect];
$targetCode = $targetCode == '*' ? $code : $targetCode;

$targetCode = (string)$node[$aspect];
$targetCode = $targetCode == '*' ? $code : $targetCode;
$target->setDataUsingMethod($targetCode, $value);
}
} else if ($targetIsExtensible) {
foreach ($fields as $code => $node) {
if (empty($node[$aspect])) {
continue;
}
$value = $source->getDataUsingMethod($code);
$targetCode = (string) $node[$aspect];
$targetCode = $targetCode == '*' ? $code : $targetCode;

$this->setAttributeValueFromExtensibleDataObject($target, $targetCode, $value);
}
} else if ($targetIsAbstract) {
foreach ($fields as $code => $node) {
if (empty($node[$aspect])) {
continue;
}
$value = $source->getDataUsingMethod($code);
$targetCode = (string) $node[$aspect];
$targetCode = $targetCode == '*' ? $code : $targetCode;

$target->setData($targetCode, $value);
}
} else {
throw new \InvalidArgumentException(
'Target should be array, Magento Object, ExtensibleDataInterface, or AbstractSimpleObject'
);
}
} else if ($sourceIsExtensible) {
if ($targetIsArray) {
foreach ($fields as $code => $node) {
if (empty($node[$aspect])) {
continue;
}
$value = $this->getAttributeValueFromExtensibleDataObject($source, $code);
$targetCode = (string) $node[$aspect];
$targetCode = $targetCode == '*' ? $code : $targetCode;

$target[$targetCode] = $value;
}
} else if ($targetIsDataObject) {
foreach ($fields as $code => $node) {
if (empty($node[$aspect])) {
continue;
}
$value = $this->getAttributeValueFromExtensibleDataObject($source, $code);
$targetCode = (string) $node[$aspect];
$targetCode = $targetCode == '*' ? $code : $targetCode;

$target->setDataUsingMethod($targetCode, $value);
}
} else if ($targetIsExtensible) {
foreach ($fields as $code => $node) {
if (empty($node[$aspect])) {
continue;
}
$value = $this->getAttributeValueFromExtensibleDataObject($source, $code);
$targetCode = (string) $node[$aspect];
$targetCode = $targetCode == '*' ? $code : $targetCode;

$this->setAttributeValueFromExtensibleDataObject($target, $targetCode, $value);
}
} else if ($targetIsAbstract) {
foreach ($fields as $code => $node) {
if (empty($node[$aspect])) {
continue;
}
$value = $this->getAttributeValueFromExtensibleDataObject($source, $code);
$targetCode = (string) $node[$aspect];
$targetCode = $targetCode == '*' ? $code : $targetCode;

$target->setData($targetCode, $value);
}
} else {
throw new \InvalidArgumentException(
'Target should be array, Magento Object, ExtensibleDataInterface, or AbstractSimpleObject'
);
}
} else if ($sourceIsAbstract) {
if ($targetIsArray) {
foreach ($fields as $code => $node) {
if (empty($node[$aspect])) {
continue;
}
$sourceArray = $source->__toArray();
$value = isset($sourceArray[$code]) ? $sourceArray[$code] : null;
$targetCode = (string) $node[$aspect];
$targetCode = $targetCode == '*' ? $code : $targetCode;

$target = $this->_setFieldsetFieldValue($target, $targetCode, $value);
$target[$targetCode] = $value;
}
} else if ($targetIsDataObject) {
foreach ($fields as $code => $node) {
if (empty($node[$aspect])) {
continue;
}
$sourceArray = $source->__toArray();
$value = isset($sourceArray[$code]) ? $sourceArray[$code] : null;
$targetCode = (string) $node[$aspect];
$targetCode = $targetCode == '*' ? $code : $targetCode;

$target->setDataUsingMethod($targetCode, $value);
}
} else if ($targetIsExtensible) {
foreach ($fields as $code => $node) {
if (empty($node[$aspect])) {
continue;
}
$sourceArray = $source->__toArray();
$value = isset($sourceArray[$code]) ? $sourceArray[$code] : null;
$targetCode = (string) $node[$aspect];
$targetCode = $targetCode == '*' ? $code : $targetCode;

$this->setAttributeValueFromExtensibleDataObject($target, $targetCode, $value);
}
} else if ($targetIsAbstract) {
foreach ($fields as $code => $node) {
if (empty($node[$aspect])) {
continue;
}
$sourceArray = $source->__toArray();
$value = isset($sourceArray[$code]) ? $sourceArray[$code] : null;
$targetCode = (string) $node[$aspect];
$targetCode = $targetCode == '*' ? $code : $targetCode;

$target->setData($targetCode, $value);
}
} else {
throw new \InvalidArgumentException(
'Target should be array, Magento Object, ExtensibleDataInterface, or AbstractSimpleObject'
);
}
} else {

throw new \InvalidArgumentException(
'Source should be array, Magento Object, ExtensibleDataInterface, or AbstractSimpleObject'
);
}


$target = $this->dispatchCopyFieldSetEvent($fieldset, $aspect, $source, $target, $root, $targetIsArray);

return $target;
Expand Down Expand Up @@ -129,7 +341,11 @@ protected function dispatchCopyFieldSetEvent($fieldset, $aspect, $source, $targe
*/
public function getDataFromFieldset($fieldset, $aspect, $source, $root = 'global')
{
if (!(is_array($source) || $source instanceof \Magento\Framework\DataObject)) {
$sourceIsArray = is_array($source);
$sourceIsDataObject = $source instanceof \Magento\Framework\DataObject;
$sourceIsExtensible = $source instanceof \Magento\Framework\Api\ExtensibleDataInterface;
$sourceIsAbstract = $source instanceof \Magento\Framework\Api\AbstractSimpleObject;
if (!($sourceIsArray || $sourceIsDataObject)) {
return null;
}
$fields = $this->fieldsetConfig->getFieldset($fieldset, $root);
Expand All @@ -143,91 +359,19 @@ public function getDataFromFieldset($fieldset, $aspect, $source, $root = 'global
continue;
}

$value = $this->_getFieldsetFieldValue($source, $code);
if ($sourceIsArray) {
$value = isset($source[$code]) ? $source[$code] : null;
} elseif ($sourceIsDataObject) {
$value = $source->getDataUsingMethod($code);
}

$targetCode = (string)$node[$aspect];
$targetCode = $targetCode == '*' ? $code : $targetCode;
$data[$targetCode] = $value;
}

return $data;
}

/**
* Check if source and target are valid input for converting using fieldset
*
* @param array|\Magento\Framework\DataObject $source
* @param array|\Magento\Framework\DataObject $target
* @return bool
*/
protected function _isFieldsetInputValid($source, $target)
{
return (is_array($source) || $source instanceof \Magento\Framework\DataObject ||
$source instanceof \Magento\Framework\Api\ExtensibleDataInterface ||
$source instanceof \Magento\Framework\Api\AbstractSimpleObject) && (
is_array($target) || $target instanceof \Magento\Framework\DataObject ||
$target instanceof \Magento\Framework\Api\ExtensibleDataInterface ||
$target instanceof \Magento\Framework\Api\AbstractSimpleObject);
}

/**
* Get value of source by code
*
* @param mixed $source
* @param string $code
*
* @return mixed
* @throws \InvalidArgumentException
*/
protected function _getFieldsetFieldValue($source, $code)
{
if (is_array($source)) {
$value = isset($source[$code]) ? $source[$code] : null;
} elseif ($source instanceof \Magento\Framework\DataObject) {
$value = $source->getDataUsingMethod($code);
} elseif ($source instanceof \Magento\Framework\Api\ExtensibleDataInterface) {
$value = $this->getAttributeValueFromExtensibleDataObject($source, $code);
} elseif ($source instanceof \Magento\Framework\Api\AbstractSimpleObject) {
$sourceArray = $source->__toArray();
$value = isset($sourceArray[$code]) ? $sourceArray[$code] : null;
} else {
throw new \InvalidArgumentException(
'Source should be array, Magento Object, ExtensibleDataInterface, or AbstractSimpleObject'
);
}
return $value;
}

/**
* Set value of target by code
*
* @param mixed $target
* @param string $targetCode
* @param mixed $value
*
* @return mixed
* @throws \InvalidArgumentException
*/
protected function _setFieldsetFieldValue($target, $targetCode, $value)
{
$targetIsArray = is_array($target);

if ($targetIsArray) {
$target[$targetCode] = $value;
} elseif ($target instanceof \Magento\Framework\DataObject) {
$target->setDataUsingMethod($targetCode, $value);
} elseif ($target instanceof \Magento\Framework\Api\ExtensibleDataInterface) {
$this->setAttributeValueFromExtensibleDataObject($target, $targetCode, $value);
} elseif ($target instanceof \Magento\Framework\Api\AbstractSimpleObject) {
$target->setData($targetCode, $value);
} else {
throw new \InvalidArgumentException(
'Source should be array, Magento Object, ExtensibleDataInterface, or AbstractSimpleObject'
);
}

return $target;
}
}

/**
* Access the extension get method
Expand Down