2.2 develop 11032 Unable to add new options to swatch attribute #11628
Conversation
|
gonna fix what failed this weekend. |
fix unused parameters
dmanners
added
Release Line: 2.2
2.2.x
bug report
Component: Catalog
Reproduced on 2.1.x
| /** | ||
| * @var Config | ||
| */ | ||
| protected $eavConfig; |
There was a problem hiding this comment.
since we have a new class here the default is to stay away from protected in properties and functions. Recommended is private unless the method is needed outside the class.
| SwatchHelper $swatchHelper, | ||
| OptionCollection $attrOptionCollectionFactory | ||
| ) { | ||
| $this->eavConfig = $eavConfig; |
There was a problem hiding this comment.
spacing looks a bit off here
| $optionKey = self::PREFIX_OPTION . $isSwatch; | ||
| $swatchKey = self::PREFIX_SWATCH . $isSwatch; | ||
|
|
||
| if (!empty($isSwatch) && $attribute->getData($optionKey) === null |
There was a problem hiding this comment.
Could you maybe update this to look as follows for readability:
if (
!empty($isSwatch)
&& $attribute->getData($optionKey) === null
&& $attribute->getData($swatchKey) === null
) {
| { | ||
| $options = $this->getOptionsByAttributeIdWithSortOrder($attributeId); | ||
| $attributeData = $this->getOptionsForSwatch($options, $optionKey); | ||
| return $attributeData; |
There was a problem hiding this comment.
You could just return $this->getOptionsForSwatch($options, $optionKey); here rather than set $attributeData.
| protected function getStoreLabels($optionId) | ||
| { | ||
| $optionCollection = $this->optionCollection->create(); | ||
| $connection = $optionCollection->getConnection(); |
There was a problem hiding this comment.
Again spacing seems a bit off
| ) { | ||
| $this->eavConfig = $eavConfig; | ||
| $this->swatchHelper = $swatchHelper; | ||
| $this->optionCollection = $attrOptionCollectionFactory; |
There was a problem hiding this comment.
Technically at this point this is the factory still right? Might be good to keep that obvious in the naming of the property.
| * | ||
| * @return null|string | ||
| */ | ||
| protected function isSwatch($attribute) |
There was a problem hiding this comment.
The naming of this method is a bit confusing to me. is would suggest to me that it will return a boolean but it actually returns a mix of string and null. I think it might be a bit more readable if there was two methods, 1 for returning a boolean and one for getting the swatch type or something like that.
| * | ||
| * @return array | ||
| */ | ||
| protected function getStoreLabels($optionId) |
There was a problem hiding this comment.
Part of me thinks that both this method and getOptionsByAttributeIdWithSortOrder should not be in the plugin but in another class.
There was a problem hiding this comment.
fixed for getOptionsByAttributeWithSortOrder but still to be fixed for storeLabels
| * | ||
| * @return array | ||
| */ | ||
| protected function getOptionsForSwatch($options, $optionKey) |
There was a problem hiding this comment.
we can make sure an array is passed in here with array $options if it is always going to be an array.
magento-engcom-team
added
bugfix
Issue: Cannot Reproduce
|
@springerin the following API test is failing. |
|
sure I will check it out today and let you know. |
|
locally tested with the last updates and still green for me. Waiting for more info to reproduce the error. |
magento-engcom-team
added
Fixed in 2.1.x
|
@springerin it seems that this has now be fixed by #12036. Using the |
ProductAttributeOptionManagementInterface#add() doesn't do anything.
Description
On the beforeBeforeSave(Attribute $attribute) in the case of a swatch attribute, expects an array with key swatchtext / swatchvisual ( first in case of textswatch, second in case of visualswatch) that overwrites the option array originally given, so if this is empty, it will be emptied as well. So I added a plugin that right before doing this, creates the expected array that will be used later for the creation of the actual option.
Fixed Issues (if relevant)
Manual testing scenarios
{ "option": { "label": "new color", "value": "", "sort_order": 0, "is_default": true, "store_labels": [ { "store_id": 0, "label": "new color" }, { "store_id": 1, "label": "new color" } ] } }Contribution checklist