Remove "Undefined fields" from app folder

[adrian-martinez-interactiv4]

Remove "Undefined fields" from app folder

Description

This PR adds missing properties to class, and optimize / reorder imports, under app folder.

Fixed Issues (if relevant)

None, improvement, since it's not really a problem, it makes no sense to backport it.

Manual testing scenarios

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)

[orlangur]

@adrian-martinez-interactiv4 are you interested in running "Undefined field" PhpStorm inspection all over the code maybe and fix all "Field declared dynamically" warnings in this PR?

I remember there was even some Fatal Error reported due to field not initialized in constructor.

[dmanners]

@orlangur I would suggest that a complete update of "Undefined fields" should be extracted to a separate issue/pr. I see this as a good fix for now and we can improve in steps.

[orlangur]

@dmanners I wouldn't want to process them as 10-20 PRs on the other hand :) Let's wait for the PR author response and if he will not find my suggestion interesting let's maybe report generic 'up for grabs' issue.

[adrian-martinez-interactiv4]

@orlangur @dmanners I'll give a try to fix all Undefined fields errors, I'll keep you updated

[adrian-martinez-interactiv4]

This PR will try to fix "Undefined fields" from app folder, lib and dev folders will have its own PR.

[adrian-martinez-interactiv4]

@orlangur Hi, commit 9980202 is the main commit of this PR.

I've left Magento/Sitemap/Model/Observer.php untouched, because it will be fixed by open PR #11320.

About class properties added, visibility has been set according to neighbour properties, and since dynamic declared variables have open visibility, most of defined properties are protected (although some that clearly could be private have been defined so, I already know they all should be private), to ensure the system still works properly. Reviewing if all the protected variables can be changed to private, and make the proper changes if needed is out of the scope of this task.

[adrian-martinez-interactiv4]

@orlangur I don't know what Codacy is complaining about:

Also, I find static tests have failed due to two types of errors, this one was an existing error:
/home/travis/build/magento/magento2/app/code/Magento/Catalog/Observer/Compare/BindCustomerLoginObserver.php:38 Avoid unused parameters such as '$observer'. /home/travis/build/magento/magento2/app/code/Magento/Catalog/Observer/Compare/BindCustomerLogoutObserver.php:38 Avoid unused parameters such as '$observer'.

They fail after adding the variable even when @SuppressWarnings(PHPMD.UnusedLocalVariable) is already used in execute() method. What should I do with them?

The second type:
/home/travis/build/magento/magento2/app/code/Magento/Fedex/Model/Carrier.php:26 The class Carrier has 16 fields. Consider redesigning Carrier to keep the number of fields under 15. /home/travis/build/magento/magento2/app/code/Magento/Shipping/Test/Unit/Controller/Adminhtml/Order/Shipment/NewActionTest.php:17 The class NewActionTest has 16 fields. Consider redesigning NewActionTest to keep the number of fields under 15. /home/travis/build/magento/magento2/app/code/Magento/Widget/Model/Widget/Instance.php:27 The class Instance has 16 fields. Consider redesigning Instance to keep the number of fields under 15.

It looks like missing fields may have been left out on purpose to fit the 15 fields limit. Should I suppress warnings for this classes?

[orlangur]

Hi @adrian-martinez-interactiv4, I don't have time to review this PR fully this night but here are the quick answers on your questions:

• simply ignore Codacy, it is not mandatory and complaining on things like else and local variable longer than 20 symbols
• use @SuppressWarnings(PHPMD.UnusedFormalParameter) annotation
• yeah, we cannot remove even unused existing fields due to backward compatibility, please suppress those errors

[adrian-martinez-interactiv4]

@orlangur done

[ihor-sviziev]

Hi @adrian-martinez-interactiv4 and @orlangur.
As for me it looks like this PR is too big and has quite big list of conflicts.
I think it's better to close this PR and prepare few smaller PRs that could be processed much faster.

What do you think about it?

[sidolov]

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