Skip to content

Conversation

drpayyne
Copy link
Contributor

@drpayyne drpayyne commented Oct 20, 2020

Description

This PR adds validation through annotations to the new controller dialog and removes the unnecessary com.magento.idea.magento2plugin.actions.generation.dialog.validator.NewControllerValidator

Fixed Issues

  1. Change validation for the new controller dialog window #297: Change validation for the new controller dialog window

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 integration/functional tests (if applicable)
  • All automated tests passed successfully (all builds are green)

@VitaliyBoyko
Copy link
Contributor

VitaliyBoyko commented Oct 21, 2020

QA failed

Case:
controller should begin with the capital letter ⛔
image
wrong exception message
image

Same for action name.
Please check all the cases and messages.

Copy link
Contributor

@VitaliyBoyko VitaliyBoyko left a comment

Choose a reason for hiding this comment

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

Please consider checking all validation cases.

@VitaliyBoyko VitaliyBoyko self-requested a review October 21, 2020 13:57
@drpayyne
Copy link
Contributor Author

Hi @VitaliyBoyko, I've fixed the issues. Please review. Thanks!

Copy link
Contributor

@VitaliyBoyko VitaliyBoyko left a comment

Choose a reason for hiding this comment

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

QA failed ⛔
Fill out the Controller Name with the not valid symbols.
image
Wrong message
image

For the Action Name works well. ✅
image

@drpayyne
Copy link
Contributor Author

@VitaliyBoyko, controller name already does a namespace check. Would it be better if we fix that instead of adding another rule to this field to limit it to alphanumeric?

@VitaliyBoyko
Copy link
Contributor

@VitaliyBoyko, controller name already does a namespace check. Would it be better if we fix that instead of adding another rule to this field to limit it to alphanumeric?

Yes

@drpayyne
Copy link
Contributor Author

Hi @VitaliyBoyko, I've remove the PhpNamespaceRule check and instead used AlphanumericRule check. The namespace rule actually only checks if the value is a PHP identifier and not a reserved keyword. This doesn't work for our use case.

@drpayyne drpayyne requested a review from VitaliyBoyko October 21, 2020 16:53
@VitaliyBoyko
Copy link
Contributor

image
image

Classes names can begin from a number.

Copy link
Contributor

@VitaliyBoyko VitaliyBoyko left a comment

Choose a reason for hiding this comment

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

Please check the case with a number at the beginning of a class name.

@drpayyne
Copy link
Contributor Author

Hi @VitaliyBoyko, all these validations have confused me on valid PHP namespaces and PHP classes. Can you please check the below links and guide me on how to proceed?

Thanks.

…plugin into issue-297

� Conflicts:
�	src/com/magento/idea/magento2plugin/actions/generation/dialog/validator/rule/PhpDirectoryRule.java
@VitaliyBoyko VitaliyBoyko merged commit bbfc850 into magento:2.1.0-develop Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants