[EAV] Improving the EAV attribute code validation, by not allowing to use n… #20526

eduard13 · 2019-01-23T14:57:43Z

Description (*)

This PR improves the attribute code validation, namely while creating the attribute programmatically. Before you were able to create (programmatically) an attribute with not allowed chars.

Fixed Issues (if relevant)

No complete validation while creation of attributes. #20943: No complete validation while creation of attributes.
AttributeCode column name length validation throws wrong error message #20766: AttributeCode column name length validation throws wrong error message

Manual testing scenarios (*)

Try to create a new attribute using dots, or with a first char being not letter
Try to create an attribute with code lenth less than 1 or more than 30 characters

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)

…ot allowed chars

magento-engcom-team · 2019-01-23T14:57:47Z

Hi @eduard13. 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

rogyar · 2019-01-23T15:48:51Z

app/code/Magento/Eav/Model/Validator/Attribute/Code.php

@@ -0,0 +1,63 @@
+<?php
+/**


It's a good idea to use strict types within a scope of new implementations

Thank you, the strict typing was declared.
Also do you think it's a good idea to rename this variable name into a less meaningful name (in order to fix the failing pipeline)?

magento2/app/code/Magento/Eav/Model/Entity/Attribute.php

Line 134 in adb1a8f

Code $attributeCodeValidator = null

You have implemented the naming in the same manner as other validators. So, it's ok.
You could use an alias in the use section for this class. Something like:

use Magento\Eav\Model\Validator\Attribute\Code as AttributeCodeValidator

@rogyar the alias was added.

irajneeshgupta · 2019-02-03T13:25:44Z

@eduard13
can you please add caps pattern also in Pregmatch regex.
['pattern' => '/^[a-zA-Z\x{600}-\x{6FF}][a-zA-Z\x{600}-\x{6FF}_0-9]{0,30}$/u']
Thank you.

eduard13 · 2019-02-04T08:43:26Z

@irajneeshgupta, I don't think that we should add a such validation, as you're able to create attributes with capitalized letters from the admin panel, moreover I don't see what issue can occur by having such attributes.

irajneeshgupta · 2019-02-04T12:23:20Z

if (trim($attributeCode)
            && !preg_match('/^[a-z][a-z0-9_]*$/', trim($attributeCode))
        ) {

@eduard13

https://github.com/magento/magento2/blob/2.3-develop/app/code/Magento/Catalog/Controller/Adminhtml/Product/Attribute/Save.php#L200

I think validation pattern should be same for both admin and creation of attributes programmatically...if admin user can create such attributes , then why such limitations for developers hence i requested you to add it.

Thank you.

irajneeshgupta · 2019-02-04T22:14:32Z

@eduard13 👍

sivaschenko

Hi @eduard13 thanks for the pull request! Please see my code review notes

sivaschenko · 2019-02-12T21:35:26Z

app/code/Magento/Catalog/Controller/Adminhtml/Product/Attribute/Save.php

                $validatorAttrCode = new \Zend_Validate_Regex(
-                    ['pattern' => '/^[a-zA-Z\x{600}-\x{6FF}][a-zA-Z\x{600}-\x{6FF}_0-9]{0,30}$/u']
+                    ['pattern' => '/^[a-zA-Z]+[a-zA-Z0-9_]*$/u']


It would be nice to move this pattern to a constant, as it is used more than once (I think added validation class is a good place for such constant to be declared).

Makes sense.
Thanks.

sivaschenko · 2019-02-12T21:39:46Z

app/code/Magento/Eav/Model/Validator/Attribute/Code.php

+     * @return bool
+     * @throws LocalizedException
+     */
+    public function isValid($attributeCode): bool


The method named isValid should return bool and should not throw an exception. Validation messages should be collected using _addMessages parent method. Appropriate exceptions should be thrown if needed by client classes (that can access messages via getMessages validator method)

sivaschenko · 2019-02-12T21:42:56Z

app/code/Magento/Eav/Model/Validator/Attribute/Code.php

+ *
+ * Validation EAV attribute code
+ */
+class Code  extends AbstractValidator


Suggested change

class Code extends AbstractValidator

class Code extends AbstractValidator

sivaschenko · 2019-02-12T21:43:15Z

app/code/Magento/Eav/Model/Validator/Attribute/Code.php

+ */
+declare(strict_types=1);
+
+namespace Magento\Eav\Model\Validator\Attribute;


Please cover the validator class with a unit test

The class was covered by Unit Test, and also all the related tests (unit, integration) were updated.

…tion

… Covering the new validator class by Unit Test

eduard13 · 2019-02-13T13:06:46Z

@sivaschenko thank you for the review, I've updated the implementation.
Please note that I've completely removed the validation from Save controller, and moved it into Validate one, as you shouldn't reach the Save if the validation is failed.

Could you please review the updates?
Thank you.

sivaschenko · 2019-02-13T15:05:17Z

I believe this pull request also resolves #20766

magento-engcom-team · 2019-02-18T08:35:36Z

Hi @rogyar, thank you for the review.
ENGCOM-4246 has been created to process this Pull Request

VasylShvorak · 2019-03-27T11:15:19Z

✔️ QA passed

…tion

m2-assistant · 2019-03-30T21:02:00Z

Hi @eduard13, 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.

…t allowing to use n… #20526

eduard13 · 2019-04-01T09:02:48Z

@magento-engcom-team is there any reason of marking the PR just as an Improvement? Not even a Bug fix or Test Coverage?
cc: @sidolov, @sivaschenko
Thank you.

sidolov · 2019-04-01T15:29:47Z

Hi @eduard13 , right now only members of the Community Engineering team are able to set up achievements for pull requests, that why our automated tool removed them. I reverted the changes because your pull request really complex and should be rewarded.
Thank you!

eduard13 · 2019-04-01T17:43:11Z

Hi @sidolov, glad to hear that, but whom should I ping for recalculating the reward points for March?
Thank you.

sidolov · 2019-04-01T17:58:45Z

Hi @eduard13 , points already recalculated

Improving the eav attribute code validation, by not allowing to use n…

6f83604

…ot allowed chars

eduard13 assigned rogyar Jan 23, 2019

eduard13 requested a review from rogyar January 23, 2019 14:57

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

rogyar reviewed Jan 23, 2019

View reviewed changes

eduard13 added 3 commits January 23, 2019 23:14

Declaring strict typing

adb1a8f

Using an alias for attribute code validator

3c8acbb

Adding returning strict typing

82f5511

eduard13 mentioned this pull request Feb 3, 2019

Fixed #20943 No complete validation while creation of attributes. #20941

Closed

Allowing usage of the capital letters in attribute code

4488371

eduard13 added 3 commits February 6, 2019 09:30

Improving the validation rules on product attribute save

e028d6f

Fixing the Regex expr

8129886

Using the same regex rule as it is in attribute code validator

079bb15

sivaschenko requested changes Feb 12, 2019

View reviewed changes

sivaschenko self-assigned this Feb 12, 2019

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

eduard13 added 2 commits February 13, 2019 14:59

Merge branch '2.3-develop' into 2.3-develop-eav-attribute-code-valida…

5666a85

…tion

Refactoring the validator usage. Updating the Unit/Integration tests.…

50b78da

… Covering the new validator class by Unit Test

eduard13 removed the Progress: needs update label Feb 13, 2019

sivaschenko mentioned this pull request Feb 13, 2019

AttributeCode column name length validation throws wrong error message #20766 #20767

Closed

4 tasks

rogyar added Award: complex Award: test coverage labels Feb 20, 2019

sidolov added the Progress: ready for testing label Feb 28, 2019

VasylShvorak self-assigned this Mar 27, 2019

ghost added Progress: testing in progress and removed Progress: ready for testing labels Mar 27, 2019

ghost added Progress: extended testing and removed Progress: testing in progress labels Mar 27, 2019

p-bystritsky self-assigned this Mar 27, 2019

p-bystritsky added 3 commits March 27, 2019 17:18

Merge branch '2.3-develop' into 2.3-develop-eav-attribute-code-valida…

6878668

…tion

magento#20526: Absent ObjectManager fix.

40683de

magento#20526: Static test fix.

392cd1b

ghost added Progress: accept and removed Progress: extended testing labels Mar 28, 2019

This was referenced Mar 30, 2019

AttributeCode column name length validation throws wrong error message #20766

Closed

No complete validation while creation of attributes. #20943

Closed

magento-engcom-team merged commit 392cd1b into magento:2.3-develop Mar 30, 2019

magento-engcom-team pushed a commit that referenced this pull request Mar 30, 2019

ENGCOM-4246: [EAV] Improving the EAV attribute code validation, by no…

427bf13

…t allowing to use n… #20526

magento-engcom-team removed Award: complex Award: test coverage labels Mar 30, 2019

magento-engcom-team added this to the Release: 2.3.2 milestone Mar 30, 2019

magento-engcom-team added Award: complex Award: test coverage labels Apr 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EAV] Improving the EAV attribute code validation, by not allowing to use n… #20526

[EAV] Improving the EAV attribute code validation, by not allowing to use n… #20526

eduard13 commented Jan 23, 2019 •

edited by sivaschenko

Loading

magento-engcom-team commented Jan 23, 2019

rogyar Jan 23, 2019

eduard13 Jan 23, 2019 •

edited

Loading

rogyar Jan 24, 2019 •

edited

Loading

eduard13 Jan 25, 2019

irajneeshgupta commented Feb 3, 2019

eduard13 commented Feb 4, 2019

irajneeshgupta commented Feb 4, 2019

irajneeshgupta commented Feb 4, 2019

sivaschenko left a comment

sivaschenko Feb 12, 2019 •

edited

Loading

eduard13 Feb 13, 2019

sivaschenko Feb 12, 2019 •

edited

Loading

sivaschenko Feb 12, 2019

sivaschenko Feb 12, 2019

eduard13 Feb 13, 2019

eduard13 commented Feb 13, 2019

sivaschenko commented Feb 13, 2019

magento-engcom-team commented Feb 18, 2019

VasylShvorak commented Mar 27, 2019

m2-assistant bot commented Mar 30, 2019

eduard13 commented Apr 1, 2019

sidolov commented Apr 1, 2019

eduard13 commented Apr 1, 2019

sidolov commented Apr 1, 2019

	class Code extends AbstractValidator
	class Code extends AbstractValidator

		@@ -0,0 +1,63 @@
		<?php
		/**

[EAV] Improving the EAV attribute code validation, by not allowing to use n… #20526

[EAV] Improving the EAV attribute code validation, by not allowing to use n… #20526

Conversation

eduard13 commented Jan 23, 2019 • edited by sivaschenko Loading

Description (*)

Fixed Issues (if relevant)

Manual testing scenarios (*)

Contribution checklist (*)

magento-engcom-team commented Jan 23, 2019

rogyar Jan 23, 2019

Choose a reason for hiding this comment

eduard13 Jan 23, 2019 • edited Loading

Choose a reason for hiding this comment

rogyar Jan 24, 2019 • edited Loading

Choose a reason for hiding this comment

eduard13 Jan 25, 2019

Choose a reason for hiding this comment

irajneeshgupta commented Feb 3, 2019

eduard13 commented Feb 4, 2019

irajneeshgupta commented Feb 4, 2019

irajneeshgupta commented Feb 4, 2019

sivaschenko left a comment

Choose a reason for hiding this comment

sivaschenko Feb 12, 2019 • edited Loading

Choose a reason for hiding this comment

eduard13 Feb 13, 2019

Choose a reason for hiding this comment

sivaschenko Feb 12, 2019 • edited Loading

Choose a reason for hiding this comment

sivaschenko Feb 12, 2019

Choose a reason for hiding this comment

sivaschenko Feb 12, 2019

Choose a reason for hiding this comment

eduard13 Feb 13, 2019

Choose a reason for hiding this comment

eduard13 commented Feb 13, 2019

sivaschenko commented Feb 13, 2019

magento-engcom-team commented Feb 18, 2019

VasylShvorak commented Mar 27, 2019

m2-assistant bot commented Mar 30, 2019

eduard13 commented Apr 1, 2019

sidolov commented Apr 1, 2019

eduard13 commented Apr 1, 2019

sidolov commented Apr 1, 2019

eduard13 commented Jan 23, 2019 •

edited by sivaschenko

Loading

eduard13 Jan 23, 2019 •

edited

Loading

rogyar Jan 24, 2019 •

edited

Loading

sivaschenko Feb 12, 2019 •

edited

Loading

sivaschenko Feb 12, 2019 •

edited

Loading