Issue-3139. Visual Swatch not showing on product page when once set as Text Swatch #15316
Conversation
mmularski
changed the title
mmularski
assigned mmularski and ishakhsuvarov and unassigned ishakhsuvarov and mmularski
| ) { | ||
| $this->swatchCollectionFactory = $collectionFactory; | ||
| $this->swatchFactory = $swatchFactory; | ||
| $this->swatchHelper = $swatchHelper; | ||
| $this->serializer = $serializer ?: ObjectManager::getInstance()->create(Json::class); | ||
| $this->storeManager = $storeManager ?: ObjectManager::getInstance()->create(StoreManagerInterface::class); |
There was a problem hiding this comment.
Plugins are not intended to follow Backwards compatibility policy, so the dependency may be added in a regular way. This would also require to fix Json dependency
There was a problem hiding this comment.
@ishakhsuvarov fixed
| /** | ||
| * @var StoreManagerInterface | ||
| */ | ||
| protected $storeManager; |
There was a problem hiding this comment.
It is not encouraged to create more protected properties, as Technical Guidelines suggest to use composition over inheritance. Additionally, Plugins are not supposed to be extended in any way, so property should be private to enforce that.
There was a problem hiding this comment.
@ishakhsuvarov fixed
| ]; | ||
| } | ||
|
|
||
| foreach ($visualSwatches as $optionId => $storeValues) { |
There was a problem hiding this comment.
Is there any specific reason to introduce another foreach? It looks like logic may be merged in the first one
There was a problem hiding this comment.
@ishakhsuvarov merged with another foreach
| $swatchType = $this->determineSwatchType($value); | ||
|
|
||
| $this->saveSwatchData($swatch, $optionId, $storeId, $swatchType, $value); | ||
| $this->isSwatchExists = null; |
There was a problem hiding this comment.
It is not really good idea for a Plugin to have a state like this.
Definitely not required in the scope of this Pull Request but whole class may be refactored, if you are interested in this.
There was a problem hiding this comment.
@ishakhsuvarov Unfortunately, I cannot find extra time for whole class refactoring for now. I agree with you this state is not a good idea, but if I remove it, it could affect other logic in this class. Is it ok to leave this until whole class will be refactored?
|
Hi @mmularski Also, in case you are interested, this class looks like it would need a complete refactoring, so that might be done as well, optionally. |
|
Hi @ishakhsuvarov |
|
@mmularski Thanks for the update. I'll run tests on internal CICD and let you know if anything else is needed. |
magento-engcom-team
added
partners-contribution
|
Hi, |
|
Hi @mmularski |
|
@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. |
|
@ishakhsuvarov I've reproduced this issue on clear 2.2-develop Magento instance with this 4 steps: |
|
Hi, any news here? |
|
Hi @mmularski , we still cannot reproduce this issue Manual testing scenario:
|
|
Hi @mmularski , I am closing this PR now due to inactivity. |
Description
Added default store ID value for visual swatches
Fixed Issues (if relevant)
Manual testing scenarios
Acceptance criteria:
Visual/Text swatches are properly visible on the product page
Contribution checklist