Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attribute option(with numeric value) cannot be added in case option with same option_id already exists #33073

Open
1 of 5 tasks
aleksandrvaimo opened this issue May 25, 2021 · 33 comments
Assignees
Labels
Area: Import / export Area: Product Component: Attributes Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Progress: dev in progress Reported on 2.4.x Indicates original Magento version for the Issue report. Reproduced on 2.4.x The issue has been reproduced on latest 2.4-develop branch

Comments

@aleksandrvaimo
Copy link

aleksandrvaimo commented May 25, 2021

Preconditions (*)

  1. Reproducible on Magento versions 2.4.1, 2.4.2
  2. Issue was found during importing products but also is reproducible in Admin on product attribute page.
    Attribute option(with numeric value) cannot be added in case option with same option_id already exists.

OptionManagement->add() methods call for AbstractSource->getOptionId() to find out if given option already exists for given attribute. As getOptionId param add() sends option label. Problem is that getOptionId() checks for both label and option_id (value). So if label is a numerical string and option with same id (but different label) exists, exception is thrown.

Model/Entity/Attribute/OptionManagement

public function add($entityType, $attributeCode, $option)
{
    $attribute = $this->loadAttribute($entityType, (string)$attributeCode);

    $label = trim($option->getLabel() ?: '');
    if (empty($label)) {
        throw new InputException(__('The attribute option label is empty. Enter the value and try again.'));
    }

    if ($attribute->getSource()->getOptionId($label) !== null) {
        throw new InputException(
            __(
                'Admin store attribute option label "%1" is already exists.',
                $option->getLabel()
            )
        );
    }

Model/Entity/Attribute/Source/AbstractSource

public function getOptionId($value)
    {
        foreach ($this->getAllOptions() as $option) {
            if ($this->mbStrcasecmp($option['label'], $value) == 0 || $option['value'] == $value) {
                return $option['value'];
            }
        }
        return null;
    }

Steps to reproduce (*)

1 Go to Admin > Stores > Attributes[Product].
1.1 Create or open any multiselect attribute.
1.2 Add new option and save attribute
1.3 Add one more option with value of first option id and save attribute
Screenshot 2021-05-26 at 12 32 20
2 Second option(import): In CSV import products file add new product with select/multiselect attribute with value of existing option_id in eav_attribute_option_value table(eg. in scv file attribute with value: 10,180. In eav_attribute_option_value must exist option with option_id = 10 or 180). Import product by using command:
bin/magento integration:job:run transport_product_import_std_csv

Expected result (*)

new attribute option will be successfully added
Screenshot 2021-05-26 at 12 43 40

Actual result (*)

Error msg in case of import:
Exception: Admin store attribute option label "180" is already exists.

Error msg in admin while saving new attribute option:
The value of Admin must be unique.(180)


Please provide Severity assessment for the Issue as Reporter. This information will help during Confirmation and Issue triage processes.

  • Severity: S0 - Affects critical data or functionality and leaves users without workaround.
  • Severity: S1 - Affects critical data or functionality and forces users to employ a workaround.
  • Severity: S2 - Affects non-critical data or functionality and forces users to employ a workaround.
  • Severity: S3 - Affects non-critical data or functionality and does not force users to employ a workaround.
  • Severity: S4 - Affects aesthetics, professional look and feel, “quality” or “usability”.
@m2-assistant
Copy link

m2-assistant bot commented May 25, 2021

Hi @aleksandrvaimo. Thank you for your report.
To help us process this issue please make sure that you provided the following information:

  • Summary of the issue
  • Information on your environment
  • Steps to reproduce
  • Expected and actual results

Please make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, please, add a comment to the issue:

@magento give me 2.4-develop instance - upcoming 2.4.x release

For more details, please, review the Magento Contributor Assistant documentation.

Please, add a comment to assign the issue: @magento I am working on this


⚠️ According to the Magento Contribution requirements, all issues must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of issues happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

@m2-assistant
Copy link

m2-assistant bot commented May 25, 2021

Hi @aleksandrvaimo! 👋
Thank you for collaboration. Only members of Community Contributors Team are allowed to be assigned to the issue. Please use @magento add to contributors team command to join Contributors team.

@engcom-Alfa engcom-Alfa self-assigned this May 26, 2021
@m2-assistant
Copy link

m2-assistant bot commented May 26, 2021

Hi @engcom-Alfa. Thank you for working on this issue.
In order to make sure that issue has enough information and ready for development, please read and check the following instruction: 👇

  • 1. Verify that issue has all the required information. (Preconditions, Steps to reproduce, Expected result, Actual result).

    DetailsIf the issue has a valid description, the label Issue: Format is valid will be added to the issue automatically. Please, edit issue description if needed, until label Issue: Format is valid appears.

  • 2. Verify that issue has a meaningful description and provides enough information to reproduce the issue. If the report is valid, add Issue: Clear Description label to the issue by yourself.

  • 3. Add Component: XXXXX label(s) to the ticket, indicating the components it may be related to.

  • 4. Verify that the issue is reproducible on 2.4-develop branch

    Details- Add the comment @magento give me 2.4-develop instance to deploy test instance on Magento infrastructure.
    - If the issue is reproducible on 2.4-develop branch, please, add the label Reproduced on 2.4.x.
    - If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!

  • 5. Add label Issue: Confirmed once verification is complete.

  • 6. Make sure that automatic system confirms that report has been added to the backlog.

@aleksandrvaimo
Copy link
Author

aleksandrvaimo commented May 26, 2021

Patch for fixing this issue:

Fix: Check if attribute exists with numeric option value

Issue was found during product import. Also is reproducible in Admin on product attribute page.

Before adding the attribute option \Magento\Eav\Model\Entity\Attribute\Source\AbstractSource::getOptionId()
searches for attribute value by value and label at the same time, so in case label exists error will be thrown.

@steps

  1. Go to Admin > Stores > Attributes[Product]. Create or open any multiselect attribute and try to save attribute with same value as already existing option_id of any other attribute

@expected

  1. new attribute option should be added

@Actual

  1. thrown error message: Admin store attribute option label "%1" is already exists.

@link #33073
@Package magento/module-eav
@Version >=102.1.0

--- Model/Entity/Attribute/Source/AbstractSource.php.org
+++ Model/Entity/Attribute/Source/AbstractSource.php
@@ -95,7 +95,23 @@
         }
         return null;
     }
-
+    //PATCH fix-incorrectly-checked-attribute-option-number-value
+    /**
+     * Get option id by label.
+     *
+     * @param string $value
+     * @return null|string
+     */
+    public function getOptionIdByLabel($value)
+    {
+        foreach ($this->getAllOptions() as $option) {
+            if ($this->mbStrcasecmp($option['label'], $value) === 0) {
+                return $option['value'];
+            }
+        }
+        return null;
+    }
+    //~PATCH
     /**
      * Add Value Sort To Collection Select
      *
--- Model/Entity/Attribute/OptionManagement.php.org
+++ Model/Entity/Attribute/OptionManagement.php
@@ -60,12 +60,16 @@
     {
         $attribute = $this->loadAttribute($entityType, (string)$attributeCode);

+        //PATCH fix-incorrectly-checked-attribute-option-number-value
         $label = trim($option->getLabel() ?: '');
-        if (empty($label)) {
+        //~PATCH
+        if ($label === null || $label === '') {
             throw new InputException(__('The attribute option label is empty. Enter the value and try again.'));
         }

-        if ($attribute->getSource()->getOptionId($label) !== null) {
+        //PATCH
+        if ($attribute->getSource()->getOptionIdByLabel($label) !== null) {
+        //~PATCH
             throw new InputException(
                 __(
                     'Admin store attribute option label "%1" is already exists.',

@aleksandrvaimo aleksandrvaimo changed the title Impossible to create attribute option in case option label is provided as number Attribute option(with numeric value) cannot be added in case option with same option_id already exists May 26, 2021
@engcom-Alfa engcom-Alfa added Reproduced on 2.4.x The issue has been reproduced on latest 2.4-develop branch Reported on 2.4.x Indicates original Magento version for the Issue report. Component: Attributes Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed labels May 31, 2021
@magento-engcom-team
Copy link
Contributor

✅ Confirmed by @engcom-Alfa
Thank you for verifying the issue. Based on the provided information internal tickets MC-42512 were created

Issue Available: @engcom-Alfa, You will be automatically unassigned. Contributors/Maintainers can claim this issue to continue. To reclaim and continue work, reassign the ticket to yourself.

@engcom-Alfa
Copy link
Contributor

Tried on Magento 2.4 Develop able to replicate.

@github-jira-sync-bot github-jira-sync-bot added Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Progress: ready for dev labels May 31, 2021
@SilinMykola
Copy link
Contributor

@magento I am working on this

@SilinMykola
Copy link
Contributor

@magento give me 2.4-develop instance

@magento-deployment-service
Copy link

Hi @SilinMykola. Thank you for your request. I'm working on Magento instance for you.

@SilinMykola
Copy link
Contributor

@engcom-Alfa issue is not reproducible and can be closed.
I created multiselect product attribute in admin panel and import test product.
I created product with one option and added new option depend on previous option option_id
admin_create_multiselect_attribute
I imported new product with custom attribute and some values.
admin_product_import_1
admin_product_import_2
admin_product_import_3
admin_product_import_4
admin_product_import_5

@ganeddact
Copy link

ganeddact commented Jul 9, 2021

@engcom-Alfa issue is not reproducible and can be closed.
I created multiselect product attribute in admin panel and import test product.
I created product with one option and added new option depend on previous option option_id
admin_create_multiselect_attribute
I imported new product with custom attribute and some values.
[...]

No as far as I can see this is not a reproduction of the issue, in your example you need to import the product with a package_size with a label identical to an existing option_id AND a new label value. To reproduce with the data available in those screenshots, your product should have product_size 214. With that you should be able to reproduce the issue.

@ganeddact
Copy link

@magento give me 2.4-develop instance

@magento-deployment-service
Copy link

Hi @ganeddact. Thank you for your request. I'm working on Magento instance for you.

@bobemoe
Copy link
Contributor

bobemoe commented Feb 19, 2022

Confirming this is an valid issue I just ran across on 2.4.2-p1

Can also confirm the patch provided in #33073 (comment) fixes the issue for me.

Thanks @aleksandrvaimo :)

@nisha-vaghela
Copy link
Contributor

@magento i am working on this

@DiegoItaly
Copy link

I confirm that the problem is reproducible on magento 2.4.4.
the patch works, just the name of the function is "getOptionId",
not anymore getOptionIdByLabel.

@bricht
Copy link

bricht commented Feb 10, 2023

Indeed broken and patch works.

@indunie
Copy link

indunie commented Mar 28, 2023

Hello there,
I also faced to the same issue in both Magento 2.4.5 and 2.4.6 community versions.
I created this patch for M 2.4.6 worked for me.
Thanks

https://drive.google.com/file/d/1vsOx_qQ-qDnEtgyQkH9Y7AdQoP9D0EPQ/view?usp=sharing

@giuseppemorelli
Copy link
Member

thanks @aleksandrvaimo for the patch!

@PachisPachis
Copy link

Reproducible on 2.4.3-p2, patch is working on it.
Thanks @aleksandrvaimo .

@pmzandbergen
Copy link
Contributor

When using "0" as label it will still fail using the patch provided by @aleksandrvaimo , caused by: $label = trim($option->getLabel() ?: ''); and also the if (empty($label)) { statement in the update(...) method (should be $label !== '', checking for null is unnecessary since trim(...) never returns null).

This will result in "0" being converted to "". We could use a null collapse operator or simply cast to a string:

  • Null collapse: $label = trim($option->getLabel() ?? '');
  • Cast to string: $label = trim((string) $option->getLabel());

This, including the original issue, should be fixed. I'll create a new patch addressing both of them.

@pmzandbergen
Copy link
Contributor

I'll create a new patch addressing both of them.

New patch:

--- Model/Entity/Attribute/Source/AbstractSource.php.org	2023-06-27 09:35:30
+++ Model/Entity/Attribute/Source/AbstractSource.php	2023-06-27 09:39:03
@@ -97,6 +97,22 @@
     }
 
     /**
+     * Get option id by label.
+     *
+     * @param string $label
+     * @return null|string
+     */
+    public function getOptionIdByLabel($label)
+    {
+        foreach ($this->getAllOptions() as $option) {
+            if ($this->mbStrcasecmp($option['label'], $label) === 0) {
+                return $option['value'];
+            }
+        }
+        return null;
+    }
+
+    /**
      * Add Value Sort To Collection Select
      *
      * @param \Magento\Eav\Model\Entity\Collection\AbstractCollection $collection
--- Model/Entity/Attribute/OptionManagement.php.org	2023-06-27 09:30:43
+++ Model/Entity/Attribute/OptionManagement.php	2023-06-27 09:39:03
@@ -60,12 +60,12 @@
     {
         $attribute = $this->loadAttribute($entityType, (string)$attributeCode);
 
-        $label = trim($option->getLabel() ?: '');
-        if (empty($label)) {
+        $label = trim((string) $option->getLabel());
+        if ($label === '') {
             throw new InputException(__('The attribute option label is empty. Enter the value and try again.'));
         }
 
-        if ($attribute->getSource()->getOptionId($label) !== null) {
+        if ($attribute->getSource()->getOptionIdByLabel($label) !== null) {
             throw new InputException(
                 __(
                     'Admin store attribute option label "%1" is already exists.',
@@ -93,8 +93,8 @@
         if (empty($optionId)) {
             throw new InputException(__('The option id is empty. Enter the value and try again.'));
         }
-        $label = trim($option->getLabel() ?: '');
-        if (empty($label)) {
+        $label = trim((string) $option->getLabel());
+        if ($label === '') {
             throw new InputException(__('The attribute option label is empty. Enter the value and try again.'));
         }
         if ($attribute->getSource()->getOptionText($optionId) === false) {
@@ -106,7 +106,7 @@
                 )
             );
         }
-        $optionIdByLabel = $attribute->getSource()->getOptionId($label);
+        $optionIdByLabel = $attribute->getSource()->getOptionIdByLabel($label);
         if (!empty($optionIdByLabel) && (int)$optionIdByLabel !== (int)$optionId) {
             throw new InputException(
                 __(
@@ -275,13 +275,14 @@
         EavAttributeInterface $attribute,
         AttributeOptionInterface $option
     ) : string {
-        $label = trim($option->getLabel());
-        $optionId = $attribute->getSource()->getOptionId($label);
+        $label = trim((string) $option->getLabel());
+        $source = $attribute->getSource();
+        $optionId = $source->getOptionIdByLabel($label);
         if ($optionId) {
             $option->setValue($optionId);
         } elseif (is_array($option->getStoreLabels())) {
             foreach ($option->getStoreLabels() as $label) {
-                $optionId = $attribute->getSource()->getOptionId($label->getLabel());
+                $optionId = $attribute->getSource()->getOptionIdByLabel($label->getLabel());
                 if ($optionId) {
                     break;
                 }

@PachisPachis
Copy link

PachisPachis commented Jul 22, 2023

When using "0" as label it will still fail using the patch provided by @aleksandrvaimo , caused by: $label = trim($option->getLabel() ?: ''); and also the if (empty($label)) { statement in the update(...) method (should be $label !== '', checking for null is unnecessary since trim(...) never returns null).

This will result in "0" being converted to "". We could use a null collapse operator or simply cast to a string:

  • Null collapse: $label = trim($option->getLabel() ?? '');
  • Cast to string: $label = trim((string) $option->getLabel());

This, including the original issue, should be fixed. I'll create a new patch addressing both of them.

Please notice that at least in 2.4.6 version there are code updates affecting OptionManagement.php. Patch differences can be circunvented but could be problematic to pull the code directly to the current development branch.

2.4.3-p2

public function add($entityType, $attributeCode, $option)
    {
        $attribute = $this->loadAttribute($entityType, (string)$attributeCode);

        $label = trim($option->getLabel() ?: '');
        if (empty($label)) {
            throw new InputException(__('The attribute option label is empty. Enter the value and try again.'));
        }

        if ($attribute->getSource()->getOptionId($label) !== null) {
            throw new InputException(
                __(
                    'Admin store attribute option label "%1" is already exists.',
                    $option->getLabel()
                )
            );
        }

        $optionId = $this->getNewOptionId($option);
        $this->saveOption($attribute, $option, $optionId);

        return $this->retrieveOptionId($attribute, $option);
    }

2.4.6-p1:

public function add($entityType, $attributeCode, $option)
    {
        $attribute = $this->loadAttribute($entityType, (string)$attributeCode);

        $label = trim((string)$option->getLabel());
        if ($label === '') {
            throw new InputException(__('The attribute option label is empty. Enter the value and try again.'));
        }

        if ($attribute->getSource()->getOptionId($label) !== null) {
            throw new InputException(
                __(
                    'Admin store attribute option label "%1" is already exists.',
                    $option->getLabel()
                )
            );
        }

        $optionId = $this->getNewOptionId($option);
        $this->saveOption($attribute, $option, $optionId);

        return $this->retrieveOptionId($attribute, $option);
    }

This is the diff file I'm using for 2.4.6-p1 version:

--- /Model/Entity/Attribute/Source/AbstractSource.php.org
+++ /Model/Entity/Attribute/Source/AbstractSource.php
@@ -95,7 +95,23 @@
         }
         return null;
     }
-
+    //PATCH fix-incorrectly-checked-attribute-option-number-value
+    /**
+     * Get option id by label.
+     *
+     * @param string $value
+     * @return null|string
+     */
+    public function getOptionIdByLabel($value)
+    {
+        foreach ($this->getAllOptions() as $option) {
+            if ($this->mbStrcasecmp($option['label'], $value) === 0) {
+                return $option['value'];
+            }
+        }
+        return null;
+    }
+    //~PATCH
     /**
      * Add Value Sort To Collection Select
      *
--- /Model/Entity/Attribute/OptionManagement.php.org
+++ /Model/Entity/Attribute/OptionManagement.php
@@ -65,7 +65,7 @@
             throw new InputException(__('The attribute option label is empty. Enter the value and try again.'));
         }

-        if ($attribute->getSource()->getOptionId($label) !== null) {
+        if ($attribute->getSource()->getOptionIdByLabel($label) !== null) {
             throw new InputException(
                 __(
                     'Admin store attribute option label "%1" is already exists.',

@sdsoldi
Copy link

sdsoldi commented Apr 19, 2024

First report on 2021.... 2024 and still not fixed

@pmzandbergen
Copy link
Contributor

2.4.6-p1

Patch supplied by @PachisPachis is still OK for 2.4.7-p2.

Too bad this issue is still not being resolved, if I've some spare time I'll open a PR (need to create some tests as well).

@JoryHogeveen
Copy link

Encountered the same issue here, thank you @PachisPachis for the patch, I can confirm it also works on 2.4.7-p3 and hope it will be merged a.s.a.p.

@danieljoeblack
Copy link

@PachisPachis where should this patch be applied?

@danieljoeblack
Copy link

@PachisPachis where should this patch be applied?

Nevermind, I was able to track it down. This patch can be applied to creating the patch file in /vendor/magento/module-eav folder and then running the git patch <file> command. After this I am able to create the options as expected. Thanks for the patch!

@pmzandbergen
Copy link
Contributor

@PachisPachis where should this patch be applied?

Nevermind, I was able to track it down. This patch can be applied to creating the patch file in /vendor/magento/module-eav folder and then running the git patch <file> command. After this I am able to create the options as expected. Thanks for the patch!

Manually patching is not recommended, since the patch will be removed when reinstalling the package. Steps:

  1. Allow composer patches plugin: composer config --no-plugins allow-plugins.cweagans/composer-patches true
  2. Install composer patches: composer require cweagans/composer-patches:~1.0
  3. Add the patch to your code base, for example app/patches/issue-33073.diff with the following contents:
--- /Model/Entity/Attribute/Source/AbstractSource.php.org
+++ /Model/Entity/Attribute/Source/AbstractSource.php
@@ -95,7 +95,23 @@
         }
         return null;
     }
-
+    //PATCH fix-incorrectly-checked-attribute-option-number-value
+    /**
+     * Get option id by label.
+     *
+     * @param string $value
+     * @return null|string
+     */
+    public function getOptionIdByLabel($value)
+    {
+        foreach ($this->getAllOptions() as $option) {
+            if ($this->mbStrcasecmp($option['label'], $value) === 0) {
+                return $option['value'];
+            }
+        }
+        return null;
+    }
+    //~PATCH
     /**
      * Add Value Sort To Collection Select
      *
--- /Model/Entity/Attribute/OptionManagement.php.org
+++ /Model/Entity/Attribute/OptionManagement.php
@@ -65,7 +65,7 @@
             throw new InputException(__('The attribute option label is empty. Enter the value and try again.'));
         }

-        if ($attribute->getSource()->getOptionId($label) !== null) {
+        if ($attribute->getSource()->getOptionIdByLabel($label) !== null) {
             throw new InputException(
                 __(
                     'Admin store attribute option label "%1" is already exists.',
  1. Add the patch to your composer.json:
{
    [...],
    "extra": {
        "patches": {
            "magento/module-eav": {
                "Fix issue #33073": "app/patches/issue-33073.diff"
            }
        }
    }
}
  1. Reinstall the patched package: composer reinstall magento/module-eav

From now on everytime you reinstall this package the patch will automatically be applied.

@danieljoeblack
Copy link

@pmzandbergen ok that makes sense. I appreciate the details, I'll look at getting this setup to avoid regression. Thanks!

@engcom-Delta
Copy link
Contributor

Hi @aleksandrvaimo ,

Thanks for your reporting and collaboration.
We have verified the issue in latest 2.4-develop instance and the issue is reproducible.
Kindly refer the screenshots.

Steps to reproduce

1 Go to Admin > Stores > Attributes[Product].
1.1 Create or open any multiselect attribute.
1.2 Add new option and save attribute
1.3 Add one more option with value of first option id and save attribute
2 Second option(import): In CSV import products file add new product with select/multiselect attribute with value of existing option_id in eav_attribute_option_value table(eg. in scv file attribute with value: 10,180. In eav_attribute_option_value must exist option with option_id = 10 or 180). Import product by using command:
Unable to import the products.

Image

Image

Image

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Import / export Area: Product Component: Attributes Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Progress: dev in progress Reported on 2.4.x Indicates original Magento version for the Issue report. Reproduced on 2.4.x The issue has been reproduced on latest 2.4-develop branch
Projects
None yet
Development

No branches or pull requests