Duplicate SKU possible if importing products

[piotrekkaminski]

From @pointia on June 28, 2017 8:10

It is possible in Magento to save a product with a sku that has a leading or trailing whitespace. This can lead to duplicate skus when you import products without the leading or trailing slash.

Preconditions

1. Magento version 1.x and 2.x - the code is the same in both versions
2. PHP 5.5 5.6 7.0 7.1
3. MySql 5.5 5.6

Steps to reproduce

1. First create a product in Magento Backend that has a trailing whitespace e.g. "SKU123 "
2. If you try to create a product with the sku "SKU123" you will get an error which is correct
3. Export de product data of the product with "SKU123 " and edit the file to remove the trailing whitespace
4. Import the new product data with the sku "SKU123" and a new product in Magento will be imported
5. Now we have 2 different products
6. Now open the first product with the sku "SKU123 " and edit the sku and remove the trailing whitespace
7. The product gets stored and now we have 2 products with the exact same sku - this is because the first product has a lower entity_id
8. If you now try to store the second product with the higher SKU you will get an error that the SKU does already exist

Expected result

• Import should trim whitespaces as uniuqe validator does OR
• Unique validator should not allow to save the product with the lowest ID if the SKU is the same

Actual result

• Import create 2 products
• Validator only displays a unique notice at products with a higher entity_id

Copied from original issue: magento/magento2#10080

Per Feb 16 import/export meeting:

SKUs and leading/trailing spaces

We will make the necessary changes to allow leading and trailing spaces in SKUs in the product edit section of the admin panel. This way Magento won't update the incorrect product when having 2 SKUs like "ABC" and "ABC ".

[piotrekkaminski]

From @koenner01 on June 29, 2017 14:11

Probably related magento/magento2#8235

[piotrekkaminski]

From @OleksandrBurianyk on July 7, 2017 11:17

Thx for your reporting. We've created intenal ticket MAGETWO-70528

[federivo]

Hi @piotrekkaminski and @dmanners

I tested this issue in current 2.3-develop.

After the 2nd reproduce step I don't get an error. Instead I get a message saying that Magento appended a "-1" at the end of the sku.

The same happens after reproduce step 8 (appends a "-2").

However, when you import the product as indicated in the reproduce steps, you end up with 2 different products with the same SKU.

Since Magento's behavior when creating the product manually is to append a "-1", should the import process do the same? Or should it throw an error not allowing to save the product as indicated in the description?

[dmanners]

@federivo thank you for looking into this.

My gut feeling would be that the admin and import should behave in the same way so that it does not cause confusion for the user. This being said I am open to discussion from @piotrekkaminski and @PieterCappelle on this matter and would be interested to see what the others would think.

[dmanners]

After the great investigation @federivo would you consider taking this on as your first task in this project? Of course with my help when you need it.

[federivo]

@dmanners sure!

[PieterCappelle]

In my opinion the SKU should never be changed automatic. 99% of all imports are connected with SAP or ERP. When you edit SKU you lose the connection with SAP or ERP. A space is also a character. It's bad, yes. I hate it. But it's a character. Just like SKU "ABCDEF" is possible should
" ABCDEF" be possible. It can be a different product.

The behaviour inside admin is also wrong in my opinion. When you create product with existing SKU Magento should throw exception in frontend and tell you to edit the SKU.

Just my 2 cents of course :)

[pointia]

Hi there,
for my personal point of view it is not a good idea to "just" append a -1 or -2 if a product is created (not in admin and not created by the import).

I would recommend to throw an exception and display to the user that a SKU with the same name already exists.
Otherwise it might happen that you run an import periodically and always get new products instead of updating the existing one.

[dmanners]

@PieterCappelle and @pointia thank you for the feedback on this issue.

[federivo]

For reference, this is the email I sent to the import/export project team, trying to clarify what the next steps should be:

===========

Hi all!

These are the problems I found and the next steps I think we should take to move this forward.

Problem 1 - The import process allows you to create products with leading and trailing spaces but the admin panel does not.

This leads to issues like:

• If you have a product with sku "ABC" and then import a product with sku "ABC ", when you try to save the 2nd product in the admin panel, its sku will be modified by appending a "-1" to it. This is incorrect since "ABC" and "ABC " are different skus but Magento applies a trim() function when checking sku uniqueness in \Magento\Catalog\Model\Product\Attribute\Backend\Sku::_generateUniqueSku:~91 getting the wrong sku which leads to generate a new sku variant with the "-1" for the product.

• Following the previous example if you import a csv file using "ABC " as the sku, Magento will update the product with the lowest ID (in my case "ABC", which is wrong).

Problem 2 - Import process and admin panel have different behavior.

Having the import process behave in a way and the admin save process behave differently is confusing and causes issues like the ones described above. We agreed in our weekly calls that it would make more sense to have both processes behave the same regarding sku management. If Magento agreees with this statement, we need help in making a decision on which way would be the most convenient (to allow leading and trailing spaces in skus or not).

• As Pieter Cappelle pointed out in the Github issue there are situations when the merchant doesn't have control of the sku, for example when importing products from an external source, like an ERP. This argument goes in favor of allowing leading and trailing spaces.

• As a counter-argument of the previous comment, Andra Lungu mentioned in a weekly call that allowing leading and trailing spaces in skus could be problematic when a merchant accidentally adds a space in the sku field of the product edition. Also Andra had a great suggestion, let's take a look at how Magento 1 managed this situation (I did not check yet).

Problem 3 - Admin panel save process modifies the sku when the desired sku is already in use.

As commented above, when you enter an sku that already exists in DB in the create product section, Magento appends a dash + a sequence number calculated to be unique. Per our weekly meetings, this seems to be an odd behavior causing unexpected data saved in the DB. Our thesis is that it would be better to validate uniqueness and present an error message back to the user.

Next steps:

• First we need a definition from Magento to verify if the "both processes should behave equally" statement is correct.

• If yes, then we need a decision on what's the correct sku management process, to allow or forbid leading/trailing spaces in skus.

• With a decision on allowing or forbidding leading/trailing spaces in skus, we need to modify both import process and admin panel save process in order to match the decision taken.

• If problem 3 is accepted as a valid issue, then we also need to modify the bits of the create product section to validate sku uniqueness and present an error message to the user instead of modifying the sku. I experimented a bit trying to get this validation work so I already made some progress towards this goal.

I hope this clarifies a bit the big picture.

Happy to hear feedback on the diagnostic as well as the next steps to get to a resolution.

[federivo]

Per Feb 16 import/export meeting -

We identified 2 issues that have been raised by this:

1. SKUs and leading/trailing spaces

Should the import process behave the same as the product creation in admin panel? Decision: Yes.

Should Magento allow leading/trailing spaces? Decision: Yes, they are valid characters and after discussing the decision is that Magento should accept them as valid characters in SKUs.

2. Trying to create a product with an already existing SKU

Magento modifies the SKU when it detects that you are trying to save an already existing SKU. It will append a "-1" to the SKU to preserve uniqueness. We discussed and agreed that this behavior should be changed so the user is notified of the repeated SKU. @dmanners said that he will talk this through with a Magento product owner. This part was moved to a separate issue: #101

[dmanners]

@piotrekkaminski could you have a look through the point 2 of the comment from @federivo .

Magento modifies the SKU when it detects that you are trying to save an already existing SKU. It will append a "-1" to the SKU to preserve uniqueness. We discussed and agreed that this behavior should be changed so the user is notified of the repeated SKU.

[federivo]

For reference: @piotrekkaminski agreed that when trying to save an existing sku in the admin panel, the desired behavior is to return an error to the user instead of modifying the sku to be unique

[dmanners]

@springerin has agreed to help out with this task, while we figure out what to do with her current task.

Work to be done here is as follows.

SKUs and leading/trailing spaces

Should the import process behave the same as the product creation in admin panel? Decision: Yes.

Should Magento allow leading/trailing spaces? Decision: Yes, they are valid characters and after discussing the decision is that Magento should accept them as valid characters in SKUs.

At the end of this task leading and trailing spaces should be allowed for a sku via the admin, import, and API processes.

[tadhgbowe]

Hello.
I've started to look into this issue. Currently retesting all scenarios. I'm seeing some slightly different results than originally reported. At the moment I'm focused on the Admin Panel and Product Import. Will post my results soon. Hopefully then a definite path to SKU happiness. :-)
Cheers.

[tadhgbowe]

Hello.

In our quest for SKU happiness I've been doing some testing. Here's what I've found so far:

TESTS:

1. Admin Panel: Create product with SKU " XYZ" - whitespace before.

2. Admin Panel: Create product with SKU "XYZ " - whitespace after. All fine.

3. Admin Panel: Create product with SKU "XYZ" - no whitespaces. Expected Result: Should create fine. Result: "-1" appended to SKU.

---

4. Admin Panel: Create product with SKU "ABC" - no whitespaces.

5. Import product with SKU "ABC " - whitespace after. All fine.

6. Import product with SKU " ABC" - whitespace after. All fine.

7. Delete "ABC" SKU (from test 4) and re-import it. All fine.

Then Edit "ABC" and change just something like the description. A Save will append a "-1" to the end of the SKU.

---

8. Create product with SKU "DEF " - whitespace after.

9. Import product with SKU "DEF" - no whitespaces. All fine.

10. Edit the SKU "DEF " (created in test 8) and remove the trailing space to "DEF". It accepts the change! Problem: we now have duplicate SKUs.

Reason: The product being updated has a lower entity_id and Magento's checkAttributeUniqueValue will find this first and assume it's fine.

11. Edit SKU "DEF" (created in test 9) and try changing just the product name. A Save will append a "-1" to the end of the SKU.

---

12. Delete SKUs from tests 8 and 9. Create product with SKU "DEF " - whitespace after.

13. Import product with SKU "DEF" - no whitespaces. All fine.

14. Edit the SKU "DEF" (created in test 13) and add the trailing space. A Save will append a "-1" to the end of the SKU.

Reason: The product being updated has a higher entity_id and Magento's checkAttributeUniqueValue will find the other first and return false = not unique.

---

EXAMINING in more detail Test 10 above:

SELECT catalog_product_entity.entity_id FROM catalog_product_entity WHERE (sku = 'DEF')

This will return both entity_id's because mySQL strips out trailing spaces when using the = comparison operator.

The first entity_id returned matches the current product being saved so it accepts it as okay. But we know the current SKU in the DB is "DEF " and
the new value being saved is "DEF" => not the same but in mySQL they are the same.
It should be returning only the entity_id in test 9.

FINDINGS:

SELECT catalog_product_entity.entity_id FROM catalog_product_entity WHERE (sku = 'DEF')

This will also return a SKU with 'DEF ' because mySQL strips out trailing spaces. Oops!
SKU is defined as a varchar in catalog_product_entity.

• The product import is working fine from tests 5, 6, 7 above. It doesn't append any "-1" values. More tests planned.

POSSIBLE SOLUTIONS:

a) checkAttributeUniqueValue (Eav/Model/Entity/AbstractEntity.php)

Inside if ($attributeBackend->getType() === 'static') {

1. we need to remove the trim around $value.

2. We also need to bind the length of the current $value and test it matches up:

->where('length(' . $attribute->getAttributeCode() . ') = :length')

I'm not sure what affect this will have on all other static attribute checks though.

Does SKU need to have it's own special entity check function? Worth discussing in next meeting.

NEXT UP:

• There are still many test cases to work through (Admin Panel and Product Import).

• In function _generateUniqueSku (Catalog/Model/Product/Attribute/Backend/Sku.php)

We need to return a failure if another exact SKU has been found.

Magento Ref: the desired behavior is to return an error to the user instead of modifying the sku to be unique.

Thanks,
T :-)

[tadhgbowe]

@dmanners

From our meeting today David quite rightly pointed out that in POSSIBLE SOLUTIONS above, the LIKE comparison operator may also solve our problem:

a) In checkAttributeUniqueValue (Eav/Model/Entity/AbstractEntity.php) we have this query example:

SELECT catalog_product_entity.entity_id FROM catalog_product_entity WHERE (sku like 'DEF ')

I've just done some tests and it works well.

SKU happiness is very much heading down it's own path in terms of uniqueness and not sure it quite fits into the generic checkAttributeUniqueValue in AbstractEntity.php anymore.

Thanks,
Tadhg :-)

[km9484]

to fix thi issue just replace function

protected function _generateUniqueSku($object)
{
$attribute = $this->getAttribute();
$entity = $attribute->getEntity();
$attributeValue = $object->getData($attribute->getAttributeCode());
$increment = null;
while (!$entity->checkAttributeUniqueValue($attribute, $object)) {
if ($increment === null) {
$increment = $this->_getLastSimilarAttributeValueIncrement($attribute, $object);
}
$sku = trim($attributeValue);
if (strlen($sku . '-' . ++$increment) > self::SKU_MAX_LENGTH) {
$sku = substr($sku, 0, -strlen($increment) - 1);
}
$sku = $sku . '-' . $increment;
$object->setData($attribute->getAttributeCode(), $sku);
}
}

with

protected function _generateUniqueSku($object)
{
$attribute = $this->getAttribute();
$entity = $attribute->getEntity();
$attributeValue = $object->getData($attribute->getAttributeCode());
$increment = null;
while (!$entity->checkAttributeUniqueValue($attribute, $object)) {
if ($increment === null) {
$increment = $this->_getLastSimilarAttributeValueIncrement($attribute, $object);
}
$sku = trim($attributeValue);
if (strlen($sku . '-' . ++$increment) > self::SKU_MAX_LENGTH) {
$sku = substr($sku, 0, -strlen($increment) - 1);
}
if($increment){
throw new \Magento\Framework\Exception\LocalizedException(
__('%1 already exist for other product.', $attribute->getAttributeCode())
);
}

        //$sku = $sku . '-' . $increment;
        $object->setData($attribute->getAttributeCode(), $sku);
    }
}

inside file vendor/magento/module-catalog/Model/Product/Attribute/Backend/Sku.php