magento/magento2#12695: Unable to change attribute type from swatch #12771
Conversation
- fixed the process of switching attribute input type
|
|
magento-engcom-team
added
bugfix
Component: Catalog
mageconf
Reproduced on 2.1.x
There was a problem hiding this comment.
@eugene-shab , Thank you for the contribution!
Please, check review comments
| /** | ||
| * @var SwatchResource | ||
| */ | ||
| protected $swatchResource; |
| @@ -68,13 +74,15 @@ class EavAttribute | |||
| public function __construct( | |||
| \Magento\Swatches\Model\ResourceModel\Swatch\CollectionFactory $collectionFactory, | |||
| \Magento\Swatches\Model\SwatchFactory $swatchFactory, | |||
| SwatchResource $swatchResource, | |||
There was a problem hiding this comment.
For make it backward compatible, set new dependency as optional and use ObjectManager to instantiating it
| @@ -157,13 +165,35 @@ protected function convertSwatchToDropdown(Attribute $attribute) | |||
| if (!empty($additionalData)) { | |||
| $additionalData = $this->serializer->unserialize($additionalData); | |||
| if (is_array($additionalData) && isset($additionalData[Swatch::SWATCH_INPUT_TYPE_KEY])) { | |||
| if ($additionalData[Swatch::SWATCH_INPUT_TYPE_KEY] == Swatch::SWATCH_INPUT_TYPE_VISUAL) { | |||
There was a problem hiding this comment.
This issue relevant only for visusal swatches?
There was a problem hiding this comment.
Yes. Only when switching from visual to dropdown input type.
| $optionsIDs = []; | ||
| if (count($attribute->getOption())) { | ||
| $options = $attribute->getOption(); | ||
| foreach ($options['value'] as $id => $item) { |
There was a problem hiding this comment.
array_column can be used for simplify code
| if (count($attribute->getOptiontext())) { | ||
| $options = $attribute->getOptiontext(); | ||
| if (count($options)) { | ||
| foreach ($options['value'] as $id => $item) { |
There was a problem hiding this comment.
array_column can be used for simplify code
| } | ||
| } | ||
|
|
||
| public function clearSwatchOptionTextByOptionId($optionIDs, $type) |
There was a problem hiding this comment.
Doc missed.
Can we merge both methods? It looks like "type" is redundant if we already have option id for delete
| @@ -67,7 +67,7 @@ public function __construct( | |||
| * | |||
| * @return array | |||
| */ | |||
| public function getCodes() | |||
| public function getCodes() | |||
| ) { | ||
| $this->typeConfigurable = $typeConfigurable; | ||
| $this->swatchAttributeCodes = $swatchAttributeCodes; | ||
| $this->swatchHelper = $swatchHelper; |
There was a problem hiding this comment.
For make it backward compatible, set new dependency as optional and use ObjectManager to instantiating it
| $swatchAttributes[$configurableAttribute->getAttributeId()] | ||
| = $configurableAttribute->getProductAttribute(); | ||
| // Due to $this->swatchAttributeCodes->getCodes() - return swatch data for every attribute. | ||
| if ($this->swatchHelper->isSwatchAttribute($configurableAttribute->getProductAttribute())) { |
There was a problem hiding this comment.
For more readability, these conditions can be moved to private method with description of intention of extra condition
There was a problem hiding this comment.
Also, mark this method as deprecated and add comment that this method introduced only for case when customer already has converted attribute.
| @@ -36,10 +40,12 @@ class SwatchAttributesProvider | |||
| */ | |||
| public function __construct( | |||
| Configurable $typeConfigurable, | |||
| SwatchAttributeCodes $swatchAttributeCodes | |||
| SwatchAttributeCodes $swatchAttributeCodes, | |||
| \Magento\Swatches\Helper\Data\Proxy $swatchHelper | |||
There was a problem hiding this comment.
Please, set proxy $swatchHelper in di.xml (remove \Proxy from construct and move it to di.xml)
Proxies and interceptors MUST NEVER be explicitly requested in constructors. See:
http://devdocs.magento.com/guides/v2.2/coding-standards/technical-guidelines.html
- fixes after code review.
- fixes after code review
- fixes after code review.
- fixes after code review.
- fixes after code review.
- fixes after code review.
|
Hi @eugene-shab , please, merge latest mainline to your branch, resolve conflicts and we continue work with PR. Thanks! |
|
After last changes in 2.2-develop: it seems that method hasSwatchAttribute can be removed. Please continue. |
Merged with last commits
| * @return bool | ||
| * @deprecated | ||
| */ | ||
| private function hasSwatchAttribute($productAttribute) |
There was a problem hiding this comment.
I think this method left from the previous implementation and useless for now, please, revert
| ); | ||
|
|
||
| $this->swatchAttributeCodes = $this->createMock(SwatchAttributeCodes::class); | ||
|
|
||
| $this->productMock = $this->createPartialMock(\Magento\Catalog\Model\Product::class, ['getId', 'getTypeId']); | ||
| $this->swatchTypeChecker = $this->createMock(SwatchAttributeType::class); | ||
|
|
||
| // $this->swatchHelper = (new ObjectManager($this)) |
There was a problem hiding this comment.
Please, remove commented code
|
Hi @eugene-shab , just a few fixes needed and please, take a look on failed tests |
|
Hi @sidolov, thank you for the review. |
|
@magento-engcom-team give me 2.2-develop instance |
|
Hi @ishakhsuvarov. Thank you for your request. I'm working on Magento 2.2-develop instance for you |
|
Hi @ishakhsuvarov, here is your Magento instance. |
|
@magento-engcom-team give me new test instance |
|
Hi @ishakhsuvarov. Thank you for your request. I'm working on Magento instance for you |
|
Hi @ishakhsuvarov, here is your new Magento instance. |
|
Hi @eugene-shab. Thank you for your contribution. Please, consider to port this solution to 2.3 release line. |
Unable to change attribute type from swatch to dropdown
Description
Fixed Magento _Swatches module.
Fixed Issues (if relevant)