Improve code quality subscriber new action by arnoudhgz · Pull Request #17232 · magento/magento2

arnoudhgz · 2018-07-28T14:32:17Z

Refactor else block for returning success message to private function
Remove not thrown @thrown tag and add missing @thrown tag
Use import statements for classes
Because imports are being used an if statement can be written on one line
Use addExceptionMessage and addSuccessMessage instead of their deprecated counterparts

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)

magento-engcom-team · 2018-07-28T14:32:23Z

Hi @arnoudhgz. 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 {$VERSION} instance - deploy vanilla Magento instance

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

ihor-sviziev · 2018-07-28T22:46:00Z

In this case redirect will not be executed. Currently this change adds regression. It's better to revert it or suggest another solution

ihor-sviziev · 2018-07-28T22:50:26Z

In all the places Magento uses Zend_Validate::is(), but in this case you suggest to replace it with object creation.
https://github.com/magento/magento2/search?p=3&q=Zend_Validate&unscoped_q=Zend_Validate
In general I agree that using static methods not the best practice, but it's better to change it in all places, also not sure if this variant is really optimal.
If you would like to propose replacement to object creation - feel free to create separate PR, but in scope of this PR - please revert this change

Thank you for the feedback, I only changed it because Codacy mentioned it as a new issue from my commit (because of the import statements).

We're using codaxy just as additional information, not all issues we want to be fixed. As example this one :)

arnoudhgz · 2018-07-29T10:04:53Z

@ihor-sviziev I refactored it slightly different, using a private function instead of the if/else block

You're doing few thing at once, it's hard to understand what's going on there. Previously is was more simple to understand.

Please add following code as separate line (as it was before)

$status = $this->_subscriberFactory->create()->subscribe($email);

@ihor-sviziev I was indeed doubting about this one, the old way is indeed more understandable. 👍

ihor-sviziev · 2018-08-08T06:20:17Z

You're doing few thing at once, it's hard to understand what's going on there. Previously is was more simple to understand.

Please add following code as separate line (as it was before)

$status = $this->_subscriberFactory->create()->subscribe($email);

ihor-sviziev · 2018-08-08T06:21:29Z

Can we define parameter and return types there?

ihor-sviziev · 2018-08-08T06:21:53Z

can we use strict comparison there? maybe just convert to expected type before?

ihor-sviziev · 2018-08-08T06:22:12Z

Can we use strict comparison there?

ihor-sviziev · 2018-08-08T19:38:33Z

Looks like this type of exception should not be thrown. Am I wrong?

I got the message from PHPStorm that not all the necessary @throws tags were set, so I added this with the autocomplete in PHPStorm.

AFAIK \Zend_Validate_Exception could be thrown in case if validator wasn't found. So this method actually can't throw such exceptions.
Please remove it.
Everything else looks good.

In vendor/magento/zendframework1/library/Zend/Validate.php in the static is method, not all exceptions are being caught. So I should leave the @throws tag or write a try/catch.

Please see: https://github.com/magento/zf1/blob/48ac5e38c591afefb95e0d7127d00fe52d0fbb0f/library/Zend/Validate.php#L195

@ihor-sviziev I found out that Magento actually had an email validator class, I required this one through dependency injection. This saves some reflection :)

ihor-sviziev · 2018-08-08T19:41:43Z

Let's wait for Travis build results

ihor-sviziev · 2018-08-10T08:18:40Z

I think it's better to use alias EmailAddressValidator or EmailValidator there to be clear

ihor-sviziev · 2018-08-10T08:19:53Z

According to https://devdocs.magento.com/guides/v2.2/contributor-guide/backward-compatible-development/
Please make this parameter optional and get object from ObjectManager as fallback.

ihor-sviziev · 2018-08-10T08:25:00Z

Please revert this change. This changes changes behavior. For now we can declare strict types =1 only for new files

@ihor-sviziev in another PR of mine, the reviewer requested to enable strict types on an existing class: #16571 (comment)

Now I am confused :(

Ok. Sorry for confusing you. I will discuss it with Maintainers and we'll decide what to do in such cases. For now no need to change strict types declaration and arguments for methods, but need to fix optional parameter in constructor

After discussion - we decided that we can't use strict types for existing classes as it's not backward compatible change. The same for method arguments and return types in methods that already exists.
#16571 (comment)

Please adjust your code.
Thx

@ihor-sviziev thank you for discussing it, at least it is now clear for me :)

ihor-sviziev · 2018-08-10T08:25:59Z

Please revert adding types of arguments and return types for all methods that were existing there. It's signature change, that is not backward compatible

+ Refactor `else` block for returning success message to private function + Use import statements for classes + Because imports are being used an `if` statement can be written on one line + Use `addExceptionMessage` and `addSuccessMessage` instead of their deprecated counterparts + Include with DI the `Magento\Framework\Validator\EmailAddress` class which can be used to validate the email address. Therefore the static function call to the `is` method on the `Zend_Validate` class is not needed anymore.

magento-engcom-team · 2018-08-14T13:35:38Z

Hi @ihor-sviziev, thank you for the review.
ENGCOM-2788 has been created to process this Pull Request

magento-engcom-team · 2018-08-19T10:27:07Z

Hi @arnoudhgz. Thank you for your contribution.
We will aim to release these changes as part of 2.3.0.
Please check the release notes for final confirmation.

magento-engcom-team added Partner: MediaCT Pull Request is created by partner MediaCT partners-contribution Pull Request is created by Magento Partner labels Jul 28, 2018

magento-engcom-team added the Component: Newsletter label Jul 28, 2018

ihor-sviziev requested changes Jul 28, 2018

View reviewed changes

ihor-sviziev added the Progress: needs update label Jul 28, 2018

ihor-sviziev self-assigned this Jul 28, 2018

arnoudhgz commented Jul 29, 2018

View reviewed changes

ihor-sviziev removed the Progress: needs update label Aug 7, 2018

ihor-sviziev self-requested a review August 7, 2018 13:15

ihor-sviziev requested changes Aug 8, 2018

View reviewed changes

ihor-sviziev added the Progress: needs update label Aug 8, 2018

ihor-sviziev reviewed Aug 8, 2018

View reviewed changes

ihor-sviziev removed the Progress: needs update label Aug 8, 2018

ihor-sviziev self-requested a review August 8, 2018 19:41

ihor-sviziev requested changes Aug 10, 2018

View reviewed changes

ihor-sviziev added Progress: needs update Progress: on hold and removed Progress: needs update Progress: on hold labels Aug 10, 2018

sidolov added the Release Line: 2.3 label Aug 14, 2018

ihor-sviziev removed the Progress: needs update label Aug 14, 2018

ihor-sviziev approved these changes Aug 14, 2018

View reviewed changes

magento-engcom-team added this to the Release: 2.3.0 milestone Aug 14, 2018

magento-engcom-team added the Progress: accept label Aug 14, 2018

magento-engcom-team merged commit 6603a24 into magento:2.3-develop Aug 19, 2018

magento-engcom-team pushed a commit that referenced this pull request Aug 19, 2018

ENGCOM-2788: Improve code quality subscriber new action #17232

d217225

lewisvoncken mentioned this pull request Oct 6, 2018

[Backport] Improve code quality subscriber new action #18433

Merged

4 tasks

Conversation

arnoudhgz commented Jul 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution checklist

Uh oh!

magento-engcom-team commented Jul 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ihor-sviziev Aug 8, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ihor-sviziev commented Aug 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ihor-sviziev Aug 10, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

magento-engcom-team commented Aug 14, 2018

Uh oh!

magento-engcom-team commented Aug 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

arnoudhgz commented Jul 28, 2018 •

edited

Loading

ihor-sviziev Aug 8, 2018 •

edited

Loading

ihor-sviziev Aug 10, 2018 •

edited

Loading