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.

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

Removes usage of classes which don't exist from DB migration scripts. #22446

Conversation

hostep
Copy link
Contributor

@hostep hostep commented Apr 21, 2019

Description (*)

This PR was sparked after some research in #22124
It might solve #22124 but it wasn't determined yet how to reproduce that issue, so I'm not going to mention it in the Fixed Issues list for now (but please let me know if I need to edit my post to add it).

Other issues solved in this PR were discovered by executing phpstan with level 0 over the DB migration scripts:

➜ vendor/bin/phpstan analyze -l 0 app/code/Magento/*/Setup/
 145/145 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 ------ --------------------------------------------------------------------------------
  Line   Catalog/Setup/CategorySetup.php
 ------ --------------------------------------------------------------------------------
  596    Class Magento\Catalog\Block\Adminhtml\Product\Helper\Form\BaseImage not found.
  629    Class Magento\Catalog\Setup\Media not found.
 ------ --------------------------------------------------------------------------------

 ------ -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  Line   Customer/Setup/Patch/Data/MigrateStoresAllowedCountriesToWebsite.php
 ------ -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  32     Property Magento\Customer\Setup\Patch\Data\MigrateStoresAllowedCountriesToWebsite::$allowedCountries has unknown class Magento\Directory\Model\AllowedCountriesFactory as its type.
 ------ -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

(fun fact: phpstan reports around 214 violations of these non-existing classes in the entire code base, if somebody wants to go fix all of these, be my guest :))

Issue 1 - BaseImage

The BaseImage class was removed a long time ago in 566be67#diff-6ab33404f7c9e2c38f69b1d172d923a9, but its usage wasn't removed from the DB Migration scripts.

Issue 2 - Media

The Media class was removed an even longer time ago in 0f0063045b5#diff-7efc962fb113e767187c2ce8d5017415, and its usage was also not removed from the DB Migration script. Later on, in dcd8b32#diff-7b8ac6c93b8d79968c8d4d8237621404L587, the Media class usage namespace was changed from Magento\Catalog\Model\Product\Attribute\Backend to Magento\Catalog\Setup without anyone noticing the class never existed at that point.

Issue 3 - AllowedCountriesFactory

The usage of class AllowedCountriesFactory was introduced in 6368e1eda02#diff-eceb8134fbc63b4fd17610ab3a3c95cc and later reverted in 8689dd7b0fa#diff-eceb8134fbc63b4fd17610ab3a3c95cc, but someone forgot to revert the changes to the docblock of the $allowedCountries variable.
(Btw: I've checked if it makes sense to use a Factory over here, and it doesn't make sense).

Fixed Issues (if relevant)

  1. ...

Manual testing scenarios (*)

  1. Clone 2.3-develop branch
  2. Run composer install
  3. Run composer require --dev phpstan/phpstan
  4. Run bin/magento setup:di:compile
  5. Run composer dump-autoload
  6. Run vendor/bin/phpstan analyze -l 0 app/code/Magento/*/Setup/

To test if DB migrations still work fine, run bin/magento setup:install ... --cleanup-database command before and after these changes, the following end result should remain the same:

  1. Check if the product attribute media_gallery (ID: 90) has the backend_model set to NULL in the table eav_attribute
  2. Check if the product attribute image (ID: 87) has the frontend_input_renderer set to NULL in the table catalog_eav_attribute

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)

@m2-assistant
Copy link

m2-assistant bot commented Apr 21, 2019

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

Copy link
Contributor

@orlangur orlangur left a comment

Choose a reason for hiding this comment

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

@hostep really nice finding! Are you going to proceed with Magento code improving using phpstan? 😉

@magento-engcom-team
Copy link
Contributor

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

@soleksii
Copy link

✔️ QA Passed

Before:

before

After:

after

@m2-assistant
Copy link

m2-assistant bot commented Apr 27, 2019

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

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.

5 participants