Treating empty selects and multiselects

bigbridge-nl · Dec 1, 2018 · 87423e8 · 87423e8
1 parent 21f8565
commit 87423e8
Show file tree

Hide file tree

Showing 6 changed files with 108 additions and 31 deletions.
diff --git a/Api/Data/ProductStoreView.php b/Api/Data/ProductStoreView.php
@@ -431,7 +431,7 @@ public function setSelectAttribute(string $attributeCode, string $option = null)
         $this->unresolvedSelects[trim($attributeCode)] = ($option === null) ? null : trim($option);
     }
 
-    public function setSelectAttributeOptionId(string $attributeCode, int $optionId)
+    public function setSelectAttributeOptionId(string $attributeCode, $optionId = null)
     {
         $this->attributes[$attributeCode] = $optionId;
     }
@@ -440,14 +440,14 @@ public function setSelectAttributeOptionId(string $attributeCode, int $optionId)
      * @param string $attributeCode
      * @param array $options The admin names of the attribute options
      */
-    public function setMultipleSelectAttribute(string $attributeCode, array $options)
+    public function setMultipleSelectAttribute(string $attributeCode, array $options = null)
     {
-        $this->unresolvedMultipleSelects[trim($attributeCode)] = array_map('trim', $options);
+        $this->unresolvedMultipleSelects[trim($attributeCode)] = ($options === null) ? null : array_map('trim', $options);
     }
 
-    public function setMultiSelectAttributeOptionIds(string $attributeCode, array $optionIds)
+    public function setMultiSelectAttributeOptionIds(string $attributeCode, array $optionIds = null)
     {
-        $this->attributes[$attributeCode] = implode(',', array_map('trim', $optionIds));
+        $this->attributes[$attributeCode] = ($optionIds === null) ? null : implode(',', array_map('trim', $optionIds));
     }
 
     /**

diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,6 +1,6 @@
 # Changelog
 
-## 1.2.7 : FIX XSD related products - 1-12-2018
+## 1.2.7 : FIX XSD related products / empty select values - 1-12-2018
 
 Pull request by Jeroen Nijhuis / Epartment
 
@@ -9,6 +9,10 @@ Pull request by Jeroen Nijhuis / Epartment
 
 Updated integration test to import all example xml files.
 
+I made the importer's behaviour for empty values more explicit in import.md, especially for empty select and multiselect attributes.
+
+Fixed the fatal error that occurred when a select was set to null.
+
 ## 1.2.6 : FIX impossible to import custom attributes via XML - 21-11-2018
 
 Pull request by Antonino Bonumore / Emergento

diff --git a/Model/Data/EavAttributeInfo.php b/Model/Data/EavAttributeInfo.php
@@ -54,8 +54,16 @@ public function __construct(string $attributeCode, int $attributeId, bool $isReq
         $this->scope = $scope;
     }
 
+    /**
+     * This flag determines if this field contains free-form text. Text that may just be an empty string.
+     *
+     * @return bool
+     */
     public function isTextual()
     {
-        return in_array($this->backendType, [self::TYPE_TEXT, self::TYPE_VARCHAR]);
+        return
+            in_array($this->backendType, [self::TYPE_TEXT, self::TYPE_VARCHAR])
+            // a multiselect field is stored in a varchar table, as a comma separated list of ids
+            && ($this->frontendInput != "multiselect");
     }
 }
diff --git a/Model/Resource/Resolver/ReferenceResolver.php b/Model/Resource/Resolver/ReferenceResolver.php
@@ -151,31 +151,33 @@ public function resolveExternalReferences(array $products, ImportConfig $config)
                 }
 
                 foreach ($storeView->getUnresolvedSelects() as $attribute => $optionName) {
-                    if ($optionName === "") {
-                        continue;
-                    }
-                    list ($id, $error) = $this->optionResolver->resolveOption($attribute, $optionName, $config->autoCreateOptionAttributes);
-                    if ($error === "") {
-                        $storeView->setSelectAttributeOptionId($attribute, $id);
+                    if ($optionName === "" || $optionName === null) {
+                        $storeView->setSelectAttributeOptionId($attribute, $optionName);
                     } else {
-                        $product->addError($error);
-                        $storeView->removeAttribute($attribute);
+                        list ($id, $error) = $this->optionResolver->resolveOption($attribute, $optionName, $config->autoCreateOptionAttributes);
+                        if ($error === "") {
+                            $storeView->setSelectAttributeOptionId($attribute, $id);
+                        } else {
+                            $product->addError($error);
+                            $storeView->removeAttribute($attribute);
+                        }
                     }
                 }
 
                 foreach ($storeView->getUnresolvedMultipleSelects() as $attribute => $optionNames) {
-                    $nonEmptyNames = array_filter($optionNames, function ($val) {
-                        return $val !== "";
-                    });
-                    if (empty($nonEmptyNames)) {
-                        continue;
-                    }
-                    list ($ids, $error) = $this->optionResolver->resolveOptions($attribute, $nonEmptyNames, $config->autoCreateOptionAttributes);
-                    if ($error === "") {
-                        $storeView->setMultiSelectAttributeOptionIds($attribute, $ids);
+                    if ($optionNames === null) {
+                        $storeView->setMultiSelectAttributeOptionIds($attribute, null);
                     } else {
-                        $product->addError($error);
-                        $storeView->removeAttribute($attribute);
+                        $nonEmptyNames = array_filter($optionNames, function ($val) {
+                            return ($val !== "") && ($val !== null);
+                        });
+                        list ($ids, $error) = $this->optionResolver->resolveOptions($attribute, $nonEmptyNames, $config->autoCreateOptionAttributes);
+                        if ($error === "") {
+                            $storeView->setMultiSelectAttributeOptionIds($attribute, $ids);
+                        } else {
+                            $product->addError($error);
+                            $storeView->removeAttribute($attribute);
+                        }
                     }
                 }
             }

diff --git a/Test/Integration/ImportTest.php b/Test/Integration/ImportTest.php
@@ -5,6 +5,7 @@
 use BigBridge\ProductImport\Api\Data\BundleProductSelection;
 use BigBridge\ProductImport\Api\Data\CustomOptionValue;
 use BigBridge\ProductImport\Api\Data\ProductStockItem;
+use BigBridge\ProductImport\Api\Importer;
 use Magento\Catalog\Api\ProductRepositoryInterface;
 use Magento\Framework\Exception\NoSuchEntityException;
 use BigBridge\ProductImport\Api\Data\BundleProduct;
@@ -1127,6 +1128,60 @@ public function testSelectAttributes()
         ");
 
         $this->assertEquals($colorGroupOptionId1 . ',' . $colorGroupOptionId2, $value);
+
+        // set to "", [] => ignore
+        $result = $this->getSelectValues($importer, $product1, "", []);
+        $this->assertEquals([$colorOptionId, $colorGroupOptionId1 . ',' . $colorGroupOptionId2], $result);
+
+        // set to null, null
+        $result = $this->getSelectValues($importer, $product1, null, null);
+        $this->assertSame([null, null], $result);
+
+        // reset to 'grey', ['red', 'blue']
+        $result = $this->getSelectValues($importer, $product1, "grey", ['red', 'blue']);
+        $this->assertEquals([$colorOptionId, $colorGroupOptionId1 . ',' . $colorGroupOptionId2], $result);
+
+        // set config to treat "" as null
+        $config->emptyNonTextValueStrategy = ImportConfig::EMPTY_NONTEXTUAL_VALUE_STRATEGY_REMOVE;
+        $importer = self::$factory->createImporter($config);
+
+        $result = $this->getSelectValues($importer, $product1, "", []);
+        $this->assertSame([null, null], $result);
+    }
+
+    /**
+     * @param Importer $importer
+     * @param SimpleProduct $product
+     * @return array
+     * @throws Exception
+     */
+    protected function getSelectValues(Importer $importer, SimpleProduct $product, $color, $colGroup)
+    {
+        $colorAttributeId = self::$metaData->productEavAttributeInfo['color']->attributeId;
+        $colorGroupAttributeId = self::$metaData->productEavAttributeInfo['color_group_product_importer']->attributeId;
+
+        $product->global()->setSelectAttribute('color', $color);
+        // note: empty value
+        $product->global()->setMultipleSelectAttribute('color_group_product_importer', $colGroup);
+
+        $importer->importSimpleProduct($product);
+        $importer->flush();
+
+        $this->assertEquals([], $product->getErrors());
+
+        $value1 = self::$db->fetchSingleCell("
+            SELECT value
+            FROM " . self::$metaData->productEntityTable . "_int
+            WHERE entity_id = {$product->id} AND attribute_id = {$colorAttributeId} AND store_id = 0
+        ");
+
+        $value2 = self::$db->fetchSingleCell("
+            SELECT value
+            FROM " . self::$metaData->productEntityTable . "_varchar
+            WHERE entity_id = {$product->id} AND attribute_id = {$colorGroupAttributeId} AND store_id = 0
+        ");
+
+        return [$value1, $value2];
     }
 
     protected function getOptionValue($attributeCode, $name)

diff --git a/doc/importer.md b/doc/importer.md
@@ -144,19 +144,27 @@ When Magento 2 uses "name" where I would use "code", I follow Magento. "attribut
 
 ## Empty values and removing attributes
 
-Any simple (scalar) attribute may be removed from the database (on a global level, or per store view) by setting it to null. For example:
+Imports often contain empty fields. When this happens this can mean one of two things:
 
-    $global->setMsrp(null);
+- IGNORE: the value of the attribute is not given and should not be imported
+- REMOVE: the attribute should not have a value, remove the existing value
 
-Attributes with the empty string value ("") are ignored by default. They are not imported.
+It is up to you, the writer of the import, how the empty value "" is being treated.
+Check if the value is "", and select your approach:
 
-If that to remove the attribute value, you have two options:
+- IGNORE: just skip the attribute, do not call setMyAttribute()
+- REMOVE: call setMyAttribute(null)
+
+If you decide to import "" as is, the importer will opt for the safest choice: IGNORE, because it prevents unwanted data destruction.
+Note: even an empty array of multiple select values will be ignored.
+
+However, you can still use config options to change the behaviour of the importer.
 
 For textual attributes (datatype varchar and text):
 
     $config->emptyTextValueStrategy = ImportConfig::EMPTY_TEXTUAL_VALUE_STRATEGY_REMOVE;
 
-For non-textual attributes (datetime, decimal and integer):
+For non-textual attributes (datetime, decimal and integer), including selects and multiple selects:
 
     $config->emptyNonTextValueStrategy = ImportConfig::EMPTY_NONTEXTUAL_VALUE_STRATEGY_REMOVE;