Fixed Magento2 Attribute option does not validate for existing records before insert #20852

ravi-chandra3197 · 2019-01-31T11:59:38Z

Description (*)

Fixed Magento2 Attribute option does not validate for existing records before insert

Fixed Issues (if relevant)

Magento2 Attribute option does not validate for existing records before insert #16852: Magento2 Attribute option does not validate for existing records before insert

Manual testing scenarios (*)

Create a product eav attribute dynamically using class (Magento\Catalog\Model\ResourceModel\Eav\Attribute) and set the entity type Id to the type id of catalog_product (See sample code below) (eg: attribute code = colors)
generate a list of possible options and then add those options into that product EAV attribute by using (Magento\Eav\Setup\EavSetup -> AddOption()) (optionarray = 'Black, Blue, White')
Generate some more options (pick some of those which are already existed) and add then add them into the product attribute by following the same step describe in step 2. ('White, Green, Purple')

Contribution checklist (*)

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds on Travis CI are green)

…s before insert

magento-engcom-team · 2019-01-31T11:59:42Z

Hi @ravi-chandra3197. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento-engcom-team give me test instance - deploy test instance based on PR changes
@magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

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

swnsma

Checking option existence with attribute_id and sort_order is the not best option.
From my point of view, query to check should be like this:
select * from eav_attribute_option as eao inner join eav_attribute_option_value as eaov using (option_id) where value = 'Black' and store_id = 0 and attribute_id = 93;
So we do next:

Check if option exists.
If option does not exist - create new option.
If option exists - check the sort order of the option.
If option exists in DB and option in the request has other sort order than option in DB - update the sort order.
If option in DB exists and has the same sort order in DB - do nothing.

…s before insert (check option id)

ravi-chandra3197 · 2019-02-04T04:56:33Z

Hello @swnsma
I have update my code can you review

swnsma · 2019-02-04T08:58:09Z

Hi @ravi-chandra3197
Looks like this is not the case, because variable $intOptionId is not initialized.
Please check my previous comment.

…s before insert (check value of option)

ravi-chandra3197 · 2019-02-04T13:34:33Z

Hello @swnsma
I have update my code can you review

swnsma · 2019-02-04T13:58:06Z

Hi @ravi-chandra3197
Thank you for your changes.
Look like your last commit has problems with the PHP syntax.
At least, I have received next message when tried to run it:

PHP Parse error: syntax error, unexpected '=>' (T_DOUBLE_ARROW), expecting ',' or ')'

Could you please check?

…s before insert (update change)

…s before insert (check label)

ravi-chandra3197 · 2019-02-05T05:01:08Z

Hello @swnsma
I have update my code can you review

swnsma · 2019-02-05T12:15:51Z

Hi @ravi-chandra3197,

Look like usage of \Magento\Framework\DB\Adapter\AdapterInterface::select is not meet by method declaration in \Magento\Framework\DB\Adapter\AdapterInterface:

Method does not expect parameters.
Method always return object of class \Magento\Framework\DB\Select.

swnsma · 2019-02-17T11:17:57Z

Hi @ravi-chandra3197,
I am closing this PR now due to inactivity.
Please reopen and update if you wish to continue.
Thank you for the collaboration!

ghost · 2019-02-17T11:17:58Z

Hi @ravi-chandra3197, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

…s before insert (check size of select)

ravi-chandra3197 · 2019-02-20T06:13:58Z

Hello @swnsma
I have update my code can you review

sivaschenko

Hi @ravi-chandra3197 thanks for the update. Can you please fix code styles and take a look at my code review note.

sivaschenko · 2019-02-21T12:53:58Z

app/code/Magento/Eav/Setup/EavSetup.php

@@ -930,13 +930,16 @@ public function addAttributeOption($option)
            }
        } elseif (isset($option['values'])) {
            foreach ($option['values'] as $sortOrder => $label) {
+$checkOpionId = $this->setup->getConnection()->select($optionTable, ['attribute_id =?' =>$option['attribute_id'], 'sort_order =?' => $sortOrder, 'value =?'=> $label]);


Please consider moving this select out of the loop if possible

we have to check each option before insert that's I have put select inside loop.
Any suggestion?

…s before insert (check with fetchrow)

…s before insert (fix travis)

…s before insert (check fetchRow)

…s before insert (check fetchAll)

…s before insert (check row data)

…s before insert (check row)

…s before insert (check optionValue)

orlangur

@ravi-chandra3197 this is not a right way to validate uniqueness.

Unique index must be added on database level instead and then constraint violation properly handled on PHP side.

ravi-chandra3197 · 2019-04-17T09:03:04Z

@ravi-chandra3197 this is not a right way to validate uniqueness.

Unique index must be added on database level instead and then constraint violation properly handled on PHP side.

Hello @orlangur
I have checked in DB and found that it's not possible to add Unique index because of its EAV table for store value, it's a possibility to repeat the same value for other attributes option.
like the color attribute black option value repeat in other custom attributes bg_color attribute also have a black option value,
In the above situation, it's not possible to add Unique index for the eav_attribute_option_value table.

orlangur · 2019-04-17T09:15:07Z

@ravi-chandra3197 not the option value must be unique but a combination <attribute, option_label> or even <attribute, option_label, store_id>.

ravi-chandra3197 · 2019-04-17T09:34:07Z

@ravi-chandra3197 not the option value must be unique but a combination <attribute, option_label> or even <attribute, option_label, store_id>.

@orlangur
In eav_attribute_option table only store
option_id, attribute_id, sort_order

https://prnt.sc/nd3tng

eav_attribute_option_value table only store
value_id, option_id, store_id, value

https://prnt.sc/nd3twl

so it's not possible to resolve issue using adding Unique index for any of above table.

orlangur · 2019-04-17T10:00:56Z

Thanks for explanation, @ravi-chandra3197, finally figured it out. Closing PR in favor of abovementioned one.

m2-assistant · 2019-04-17T10:00:58Z

Hi @ravi-chandra3197, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

ravi-chandra3197 · 2019-04-17T10:27:32Z

@ravi-chandra3197 not the option value must be unique but a combination <attribute, option_label> or even <attribute, option_label, store_id>.

@orlangur
In eav_attribute_option table only store
option_id, attribute_id, sort_order

https://prnt.sc/nd3tng

eav_attribute_option_value table only store
value_id, option_id, store_id, value

https://prnt.sc/nd3twl

so it's not possible to resolve issue using adding Unique index for any of above table.

@orlangur
In backend it's validate attribute option value using unique array value on ajax call.
for validate attribute option using setup, it's only one way which I have use in this PR

orlangur · 2019-04-17T11:16:11Z

@ravi-chandra3197 why did you reopen this PR?

ravi-chandra3197 · 2019-04-17T11:25:32Z

@ravi-chandra3197 why did you reopen this PR?

@orlangur
Because my PR has a proper solution.
it's not possible to add Unique index in DB.

in the backend, it uses ajax call validates unique attribute option value in an array.
for adding attribute option value using setup script there is only one way validate which I have proposed in this PR.

orlangur · 2019-04-17T13:42:33Z

@ravi-chandra3197 could your please read my comment again?

orlangur · 2019-06-22T13:04:39Z

Hi @ravi-chandra3197, so basically #21424 solves the same issue.

m2-assistant · 2019-06-22T13:04:41Z

Hi @ravi-chandra3197, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

Fixed Magento2 Attribute option does not validate for existing record…

83b3504

…s before insert

magento-engcom-team added Component: Eav Release Line: 2.3 Partner: Krish TechnoLabs partners-contribution Pull Request is created by Magento Partner labels Jan 31, 2019

swnsma self-requested a review February 2, 2019 09:18

swnsma requested changes Feb 3, 2019

View reviewed changes

swnsma added the Progress: needs update label Feb 3, 2019

Fixed Magento2 Attribute option does not validate for existing record…

5c34961

…s before insert (check option id)

swnsma self-assigned this Feb 4, 2019

Fixed Magento2 Attribute option does not validate for existing record…

b724fb8

…s before insert (check value of option)

ravi-chandra3197 added 2 commits February 5, 2019 10:06

Fixed Magento2 Attribute option does not validate for existing record…

fcba9b2

…s before insert (update change)

Fixed Magento2 Attribute option does not validate for existing record…

9c32184

…s before insert (check label)

swnsma closed this Feb 17, 2019

Fixed Magento2 Attribute option does not validate for existing record…

2bacf70

…s before insert (check size of select)

ravi-chandra3197 reopened this Feb 20, 2019

sivaschenko removed the Progress: needs update label Feb 21, 2019

sivaschenko requested changes Feb 21, 2019

View reviewed changes

sivaschenko added the Progress: needs update label Feb 21, 2019

sivaschenko self-assigned this Feb 21, 2019

ravi-chandra3197 added 7 commits February 26, 2019 17:25

Fixed Magento2 Attribute option does not validate for existing record…

1a22392

…s before insert (check with fetchrow)

Fixed Magento2 Attribute option does not validate for existing record…

259c42f

…s before insert (fix travis)

Fixed Magento2 Attribute option does not validate for existing record…

f5ee546

…s before insert (check fetchRow)

Fixed Magento2 Attribute option does not validate for existing record…

d863313

…s before insert (check fetchAll)

Fixed Magento2 Attribute option does not validate for existing record…

dfd1171

…s before insert (check row data)

Fixed Magento2 Attribute option does not validate for existing record…

cc776ed

…s before insert (check row)

Fixed Magento2 Attribute option does not validate for existing record…

17d36dd

…s before insert (check optionValue)

orlangur requested changes Apr 6, 2019

View reviewed changes

ghost assigned orlangur Apr 6, 2019

orlangur mentioned this pull request Apr 17, 2019

Magento2 attribute option does not validate for existing records before insert 16852 #21424

Merged

4 tasks

orlangur closed this Apr 17, 2019

ravi-chandra3197 reopened this Apr 17, 2019

ghost unassigned orlangur, sivaschenko and swnsma Apr 17, 2019

orlangur assigned orlangur, sivaschenko and swnsma Apr 17, 2019

orlangur closed this Jun 22, 2019

Fixed Magento2 Attribute option does not validate for existing records before insert #20852

Fixed Magento2 Attribute option does not validate for existing records before insert #20852

Uh oh!

Conversation

ravi-chandra3197 commented Jan 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description (*)

Fixed Issues (if relevant)

Manual testing scenarios (*)

Contribution checklist (*)

Uh oh!

magento-engcom-team commented Jan 31, 2019

Uh oh!

swnsma left a comment

Choose a reason for hiding this comment

Uh oh!

ravi-chandra3197 commented Feb 4, 2019

Uh oh!

swnsma commented Feb 4, 2019

Uh oh!

ravi-chandra3197 commented Feb 4, 2019

Uh oh!

swnsma commented Feb 4, 2019

Uh oh!

ravi-chandra3197 commented Feb 5, 2019

Uh oh!

swnsma commented Feb 5, 2019

Uh oh!

swnsma commented Feb 17, 2019

Uh oh!

ghost commented Feb 17, 2019

Uh oh!

ravi-chandra3197 commented Feb 20, 2019

Uh oh!

sivaschenko left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sivaschenko Feb 21, 2019

Choose a reason for hiding this comment

Uh oh!

ravi-chandra3197 Feb 21, 2019

Choose a reason for hiding this comment

Uh oh!

orlangur left a comment

Choose a reason for hiding this comment

Uh oh!

ravi-chandra3197 commented Apr 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orlangur commented Apr 17, 2019

Uh oh!

ravi-chandra3197 commented Apr 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orlangur commented Apr 17, 2019

Uh oh!

m2-assistant bot commented Apr 17, 2019

Uh oh!

ravi-chandra3197 commented Apr 17, 2019

Uh oh!

orlangur commented Apr 17, 2019

Uh oh!

ravi-chandra3197 commented Apr 17, 2019

Uh oh!

orlangur commented Apr 17, 2019

Uh oh!

orlangur commented Jun 22, 2019

Uh oh!

m2-assistant bot commented Jun 22, 2019

Uh oh!

Uh oh!

ravi-chandra3197 commented Jan 31, 2019 •

edited

Loading

sivaschenko left a comment •

edited

Loading

ravi-chandra3197 commented Apr 17, 2019 •

edited

Loading

ravi-chandra3197 commented Apr 17, 2019 •

edited

Loading