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.

Sign up for GitHub

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

Jump to bottom

Re saving product attribute #11617

Merged
merged 1 commit into from
Dec 4, 2017
Merged

Re saving product attribute #11617

merged 1 commit into from
Dec 4, 2017

Conversation

raumatbel
Copy link
Contributor

Description

To re save product attribute, the attribute code is not retrieve correctly.

Fixed Issues (if relevant)

  1. magento/magento2#6770: M2.1.1 : Re-saving a product attribute with a different name than it's code results in an error

This was referenced Oct 21, 2017
Merged
Merged
orlangur
orlangur requested changes Oct 21, 2017
View reviewed changes
app/code/Magento/Catalog/Controller/Adminhtml/Product/Attribute/Save.php Outdated
$attributeId = $this->getRequest()->getParam('attribute_id');
$attributeCode = $this->getRequest()->getParam('attribute_code')
?: $this->generateCode($this->getRequest()->getParam('frontend_label')[0]);
if($attributeId){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove @codingStandardsIgnoreFile in the top of file, such formatting violates PSR-2 but it is not checked until there is an annotation.

app/code/Magento/Catalog/Controller/Adminhtml/Product/Attribute/Save.php Outdated
$model->load($attributeId);
$attributeCode = $model->getAttributeCode();
}else{
$attributeCode = $this->getRequest()->getParam('attribute_code')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we actually passing attribute_code in some scenarios in current implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After of this, in this function checks if the attributeCode is valid or not using Zend_Validate_Regex. I think that this I can improve it in the next commit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, is attribute_code present in UI or it is only auto-generated and then displayed as disabled field?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, ok, just checked admin may enter attribute code manually when creating new attribute.

app/code/Magento/Catalog/Controller/Adminhtml/Product/Attribute/Save.php
CollectionFactory $groupCollectionFactory,
FilterManager $filterManager,
Product $productHelper,
LayoutFactory $layoutFactory
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for cleanup 👍

@orlangur orlangur self-assigned this Oct 21, 2017
@orlangur orlangur added this to the October 2017 milestone Oct 23, 2017
@orlangur orlangur added Release Line: 2.2 2.2.x bug report Component: Catalog Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release labels Oct 23, 2017
@raumatbel
Copy link
Contributor Author

raumatbel commented Oct 24, 2017 •
edited
Loading

@orlangur can you check this PR with the new changes please?

@orlangur
Copy link
Contributor

@raumatbel sure, just need some time to understand the whole logic of the method, will have time for review in 5-7 hours.

@raumatbel raumatbel changed the title Fix re saving product attribute Re saving product attribute Oct 29, 2017
@okorshenko okorshenko modified the milestones: October 2017, November 2017 Nov 1, 2017
orlangur
orlangur requested changes Nov 27, 2017
View reviewed changes
app/code/Magento/Catalog/Controller/Adminhtml/Product/Attribute/Save.php Outdated
@@ -127,8 +143,10 @@ public function execute()
$attributeId = $this->getRequest()->getParam('attribute_id');
$attributeCode = $this->getRequest()->getParam('attribute_code')
?: $this->generateCode($this->getRequest()->getParam('frontend_label')[0]);
if (strlen($attributeCode) > 0) {
$validatorAttrCode = new \Zend_Validate_Regex(['pattern' => '/^[a-z\x{600}-\x{6FF}][a-z\x{600}-\x{6FF}_0-9]{0,30}$/u']);
if (!$attributeId && strlen($attributeCode) > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it would be safer to validate attribute code even for existing attributes like it was before.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically here is the appropriate logic change as to me: #6770 (comment)

We just should not obtain attribute code from request or generate it for already existing attributes.

@raumatbel raumatbel force-pushed the FR#ReSavingProductAttribute22 branch from 3895d64 to 20f81d3 Compare November 29, 2017 22:40
@raumatbel
Copy link
Contributor Author

@orlangur Codacy has failed by an error in the name of parameter of Magento. That name has not been modified by me.

@orlangur
Copy link
Contributor

@raumatbel surely, Codacy can be ignored safely. Changes looks good to me, I first was concerned about the $model->load but then noticed you simply moved it from the very bottom like it was before and not introduced by yourself.

Please squash them into single commit.

@raumatbel
Copy link
Contributor Author

raumatbel commented Nov 30, 2017 •
edited
Loading

Yes @orlangur. I have moved the model to up. I have not used repository as in the example to don't add new param in the constructor.

Done in 1 commit.

Also, I have changed $this->messageManager->addError() by $this->messageManager->addErrorMessage() and the same with succesMessage() and exceptionMessage()

@raumatbel raumatbel force-pushed the FR#ReSavingProductAttribute22 branch from 912a5b3 to c8ddf6b Compare November 30, 2017 13:24
@okorshenko okorshenko merged commit c8ddf6b into magento:2.2-develop Dec 4, 2017
okorshenko pushed a commit that referenced this pull request Dec 4, 2017
MAGETWO-82464: Fix re saving product attribute #11617
b1e5c25
@magento-engcom-team magento-engcom-team added the Fixed in 2.2.x The issue has been fixed in 2.2 release line label Dec 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@orlangur orlangur orlangur approved these changes

Assignees

@orlangur orlangur

Labels
bug report bugfix Component: Catalog Fixed in 2.2.x The issue has been fixed in 2.2 release line Progress: accept Release Line: 2.2 Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release
Projects
None yet
Milestone
December 2017
Development

Successfully merging this pull request may close these issues.
5 participants
@raumatbel @orlangur @magento-team @okorshenko @magento-engcom-team