magento2 icon indicating copy to clipboard operation
magento2 copied to clipboard

Published 20 hours ago • verified publisher icon magento

Fixes incorrect classes being referenced in Magento modules.

Open hostep opened this issue 2 years ago • 19 comments

Description (*)

These were found by running phpstan with level 0 on all code in app/code/Magento

The first commit deals with the description here, further commits are to fix failing tests.

Related Pull Requests

Similar fixes as in https://github.com/magento/magento2/pull/37629 (but that was for code in lib/internal/Magento/Framework)

Fixed Issues (if relevant)

Manual testing scenarios (*)

  1. Run: vendor/bin/phpstan clear-result-cache && bin/magento setup:di:compile && composer dump-autoload
  2. Run: vendor/bin/phpstan analyse --level=0 -c ./dev/tests/static/testsuite/Magento/Test/Php/_files/phpstan/phpstan.neon app/code/Magento/ (if you don't use a very modern computer with a very good processor and enough memory, it might take a long time to run, or even crash)
  3. After applying the changes from this PR and re-running steps 1 and 2 from above, you should have 16 fewer errors.
  4. Try printing a creditmemo as pdf file in the backoffice, the filename you get should be creditmemo{current-date}.pdf
  5. Make sure reindexing of stocks still works as expected
  6. All automated tests should keep running correctly I hope

Questions or comments

Changes include:

  • properly ignoring phpstan errors for classes that don't exist in the codebase but are declared as a virtualType (which phpstan doesn't know about)
  • fixing non existing classes in unit tests to existing classes (shows that all this mocking of classes in phpunit doesn't assure anything)
  • a real bugfix in Magento\CatalogInventory\Model\Indexer\Stock\Action\FullAction where if you inherit from the class, and call the parent constructor and do not provide a value for $batchSizeManagement, it will try to load a non-existing class and crash
  • a real bugfix in Magento\Sales\Controller\Adminhtml\Creditmemo\AbstractCreditmemo\PrintAction to fix the filename for creditmemo pdf files, it seems like this bug was accidentally introduced in https://github.com/magento/magento2/commit/d7ff74d07e49b81f172044e81a7348008ea75b08#diff-b6974c4da004f8f764ba347774b2f3dbfeebab760c91f2f4c647dff63fcb6bfe
  • implementing HttpGetActionInterface interface PrintAction class (revealed by failing static test in this PR)
  • fixing extra phpstan failures around activeTableSwitcher->switchTable line where the variables $indexer aren't guaranteed to exist
  • a bunch of static test failure fixes

This fixes the following phpstan errors:

 ------ --------------------------------------------------------------------------------------
  Line   Catalog/Model/ProductRepository.php
 ------ --------------------------------------------------------------------------------------
  809    Class Magento\Catalog\Model\Api\SearchCriteria\ProductCollectionProcessor not found.
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
 ------ --------------------------------------------------------------------------------------

 ------ ---------------------------------------------------------------------
  Line   Catalog/Test/Unit/Model/ResourceModel/AttributeTest.php
 ------ ---------------------------------------------------------------------
  82     Class Magento\ResourceConnections\DB\Select not found.
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
 ------ ---------------------------------------------------------------------

 ------ -----------------------------------------------------------------------------------
  Line   CatalogInventory/Model/Indexer/Stock/Action/Full.php
 ------ -----------------------------------------------------------------------------------
  128    Class Magento\CatalogInventory\Model\Indexer\Stock\BatchSizeManagement not found.
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
  206    Variable $indexer might not be defined.
  206    Variable $indexer might not be defined.
 ------ -----------------------------------------------------------------------------------

 ------ -----------------------------------------------------------------------------------
  Line   CatalogRule/Model/ResourceModel/Rule.php
 ------ -----------------------------------------------------------------------------------
  266    Class Magento\CatalogRule\Model\ResourceModel\Rule\AssociatedEntityMap not found.
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
 ------ -----------------------------------------------------------------------------------

 ------ -----------------------------------------------------------------------------------
  Line   CatalogRule/Model/ResourceModel/Rule/Collection.php
 ------ -----------------------------------------------------------------------------------
  168    Class Magento\CatalogRule\Model\ResourceModel\Rule\AssociatedEntityMap not found.
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
 ------ -----------------------------------------------------------------------------------

------ -------------------------------------------------------------------------------
  Line   Cms/Model/PageRepository.php
 ------ -------------------------------------------------------------------------------
  294    Class Magento\Cms\Model\Api\SearchCriteria\PageCollectionProcessor not found.
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
 ------ -------------------------------------------------------------------------------

 ------ ---------------------------------------------------------------------
  Line   Paypal/Observer/AddPaypalShortcutsObserver.php
 ------ ---------------------------------------------------------------------
  60     Class Magento\Paypal\Block\WpsExpress\Shortcut not found.
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
  61     Class Magento\Paypal\Block\WpsBml\Shortcut not found.
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
  62     Class Magento\Paypal\Block\PayflowExpress\Shortcut not found.
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
  63     Class Magento\Paypal\Block\Payflow\Bml\Shortcut not found.
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
 ------ ---------------------------------------------------------------------

 ------ --------------------------------------------------------------------------
  Line   Sales/Controller/Adminhtml/Creditmemo/AbstractCreditmemo/PrintAction.php
 ------ --------------------------------------------------------------------------
  76     Class creditmemo not found.
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
 ------ --------------------------------------------------------------------------

 ------ ---------------------------------------------------------------------
  Line   Sales/Test/Unit/Helper/ReorderTest.php
 ------ ---------------------------------------------------------------------
  81     Class Magento\Sales\Model\Store not found.
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
 ------ ---------------------------------------------------------------------

 ------ ---------------------------------------------------------------------------------
  Line   SalesRule/Model/ResourceModel/Rule.php
 ------ ---------------------------------------------------------------------------------
  88     Class Magento\SalesRule\Model\ResourceModel\Rule\AssociatedEntityMap not found.
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
 ------ ---------------------------------------------------------------------------------

 ------ ---------------------------------------------------------------------------------
  Line   SalesRule/Model/ResourceModel/Rule/Collection.php
 ------ ---------------------------------------------------------------------------------
  456    Class Magento\SalesRule\Model\ResourceModel\Rule\AssociatedEntityMap not found.
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
 ------ ---------------------------------------------------------------------------------

 ------ ---------------------------------------------------------------------
  Line   Theme/Test/Unit/Model/Config/ValidatorTest.php
 ------ ---------------------------------------------------------------------
  77     Class Magento\Email\Model\TemplateInterface not found.
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
  122    Class Magento\Email\Model\TemplateInterface not found.
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
 ------ ---------------------------------------------------------------------

I won't be adding more automated tests, unless it's really expected, but I might need some help for that...

Contribution checklist (*)

  • [x] Pull request has a meaningful description of its purpose
  • [x] All commits are accompanied by meaningful commit messages
  • [ ] All new or changed code is covered with unit/integration tests (if applicable)
  • [ ] README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
  • [ ] All automated tests passed successfully (all builds are green)

hostep avatar Jul 21 '23 15:07 hostep

Hi @hostep. Thank you for your contribution! Here are some useful tips on 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 give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

:exclamation: Automated tests can be triggered manually with an appropriate comment:

  • @magento run all tests - run or re-run all required tests against the PR changes
  • @magento run <test-build(s)> - run or re-run specific test build(s) For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names.

Allowed build names are:
  1. Database Compare
  2. Functional Tests CE
  3. Functional Tests EE
  4. Functional Tests B2B
  5. Integration Tests
  6. Magento Health Index
  7. Sample Data Tests CE
  8. Sample Data Tests EE
  9. Sample Data Tests B2B
  10. Static Tests
  11. Unit Tests
  12. WebAPI Tests
  13. Semantic Version Checker

You can find more information about the builds here :information_source: Run only required test builds during development. Run all test builds before sending your pull request for review.

For more details, review the Code Contributions documentation. Join Magento Community Engineering Slack and ask your questions in #github channel.

m2-assistant[bot] avatar Jul 21 '23 15:07 m2-assistant[bot]

@magento run all tests

hostep avatar Jul 21 '23 15:07 hostep

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

magento-automated-testing[bot] avatar Jul 21 '23 15:07 magento-automated-testing[bot]

@magento run all tests

hostep avatar Jul 21 '23 16:07 hostep

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

magento-automated-testing[bot] avatar Jul 21 '23 16:07 magento-automated-testing[bot]

@magento run all tests

hostep avatar Jul 21 '23 19:07 hostep

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

magento-automated-testing[bot] avatar Jul 21 '23 19:07 magento-automated-testing[bot]

@magento run all tests

hostep avatar Jul 22 '23 11:07 hostep

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

magento-automated-testing[bot] avatar Jul 22 '23 11:07 magento-automated-testing[bot]

Okay, everything seems to be stable again. Current failed tests don't look related to changes in this PR at first sight.

Can be reviewed & tested now, let me know if something is not clear, thanks! 🙂

hostep avatar Jul 22 '23 13:07 hostep

Rebased on latest 2.4-develop branch, fixed merge conflicts and force pushed, because the file app/code/Magento/CatalogRule/Model/ResourceModel/Rule.php was in conflict due to changes introduced in https://github.com/magento/magento2/commit/9ab6691125cfcbe2b6a9f384f7255f559b645ba6

@magento run all tests

hostep avatar Apr 17 '24 07:04 hostep

Okay, I'll attempt to solve them.

I've already identified the commits that introduced the conflicts (all related to phpunit 10 compatibility):

  • https://github.com/magento/magento2/commit/978a24fb21628dccda731e87c16a1fe817a082d8#diff-b11c649376c43c8d652885e7cd4a3e211cef9b6de591bd7c08a6ca3ea7cbaa55
  • https://github.com/magento/magento2/commit/2e200ab9169e4ad907dcd9f0b648be9a5bf78552#diff-42bc2e6c5a0041b429aa6bf03602e2fa0d5e7a93e52c33d52a69c7047e79eee4
  • https://github.com/magento/magento2/commit/3dfda8e44740f3475d8b7651bb4dee5692e55f50#diff-7267ac01a411b234c6e7fe51ad7cea967824112c1fb2b7c9a27a4c578fc660b2

hostep avatar Aug 20 '24 19:08 hostep

Rebased, solved conflicts and force pushed.

Only 13 errors got fixed now instead of 16 before, because in the commits referenced earlier some fixes were already done, these are the 13 ones that are fixed now:

 ------ --------------------------------------------------------------------------------------
  Line   Catalog/Model/ProductRepository.php
 ------ --------------------------------------------------------------------------------------
  817    Class Magento\Catalog\Model\Api\SearchCriteria\ProductCollectionProcessor not found.
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
 ------ --------------------------------------------------------------------------------------

 ------ -----------------------------------------------------------------------------------
  Line   CatalogInventory/Model/Indexer/Stock/Action/Full.php
 ------ -----------------------------------------------------------------------------------
  128    Class Magento\CatalogInventory\Model\Indexer\Stock\BatchSizeManagement not found.
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
 ------ -----------------------------------------------------------------------------------

 ------ -----------------------------------------------------------------------------------
  Line   CatalogRule/Model/ResourceModel/Rule.php
 ------ -----------------------------------------------------------------------------------
  133    Class Magento\CatalogRule\Model\ResourceModel\Rule\AssociatedEntityMap not found.
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
 ------ -----------------------------------------------------------------------------------

 ------ -----------------------------------------------------------------------------------
  Line   CatalogRule/Model/ResourceModel/Rule/Collection.php
 ------ -----------------------------------------------------------------------------------
  168    Class Magento\CatalogRule\Model\ResourceModel\Rule\AssociatedEntityMap not found.
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
 ------ -----------------------------------------------------------------------------------

 ------ -------------------------------------------------------------------------------
  Line   Cms/Model/PageRepository.php
 ------ -------------------------------------------------------------------------------
  294    Class Magento\Cms\Model\Api\SearchCriteria\PageCollectionProcessor not found.
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
 ------ -------------------------------------------------------------------------------

 ------ ---------------------------------------------------------------------
  Line   Paypal/Observer/AddPaypalShortcutsObserver.php
 ------ ---------------------------------------------------------------------
  60     Class Magento\Paypal\Block\WpsExpress\Shortcut not found.
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
  61     Class Magento\Paypal\Block\WpsBml\Shortcut not found.
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
  62     Class Magento\Paypal\Block\PayflowExpress\Shortcut not found.
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
  63     Class Magento\Paypal\Block\Payflow\Bml\Shortcut not found.
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
 ------ ---------------------------------------------------------------------

 ------ --------------------------------------------------------------------------
  Line   Sales/Controller/Adminhtml/Creditmemo/AbstractCreditmemo/PrintAction.php
 ------ --------------------------------------------------------------------------
  76     Class creditmemo not found.
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
 ------ --------------------------------------------------------------------------

 ------ ---------------------------------------------------------------------
  Line   Sales/Test/Unit/Helper/ReorderTest.php
 ------ ---------------------------------------------------------------------
  81     Class Magento\Sales\Model\Store not found.
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
 ------ ---------------------------------------------------------------------

 ------ ---------------------------------------------------------------------------------
  Line   SalesRule/Model/ResourceModel/Rule.php
 ------ ---------------------------------------------------------------------------------
  88     Class Magento\SalesRule\Model\ResourceModel\Rule\AssociatedEntityMap not found.
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
 ------ ---------------------------------------------------------------------------------

 ------ ---------------------------------------------------------------------------------
  Line   SalesRule/Model/ResourceModel/Rule/Collection.php
 ------ ---------------------------------------------------------------------------------
  467    Class Magento\SalesRule\Model\ResourceModel\Rule\AssociatedEntityMap not found.
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols
 ------ ---------------------------------------------------------------------------------

@magento run all tests

hostep avatar Aug 20 '24 19:08 hostep

Failed tests don't seem related to changes here, so can be reviewed again.

hostep avatar Aug 21 '24 05:08 hostep

@magento run all tests

engcom-Hotel avatar Aug 23 '24 03:08 engcom-Hotel

@magento run Static Tests, Integration Tests

engcom-Hotel avatar Aug 23 '24 08:08 engcom-Hotel

Hello @hostep,

Thank you for the collaboration and contribution!

✔️ QA Passed

Steps to reproduce:

  • Run: vendor/bin/phpstan clear-result-cache && bin/magento setup:di:compile && composer dump-autoload
  • Run: vendor/bin/phpstan analyse --level=0 -c ./dev/tests/static/testsuite/Magento/Test/Php/_files/phpstan/phpstan.neon app/code/Magento/

Before: ✖️ There used to be 210 phpstan errors. image

After: ✔️ 13 error's has been fixed. image

Since there are build failures, moving this to Extended Testing.

engcom-November avatar Aug 27 '24 11:08 engcom-November

@magento run Functional Tests B2B, Functional Tests EE, Static Tests

engcom-November avatar Aug 27 '24 11:08 engcom-November

The repeating Functional Test EE failure is falky and known issue.

Functional Tests B2B Run 1: image

Run 2: image

Functional Tests EE Run 1: image

Run 2: image

Known issues: ACQE-6331 - StorefrontCreateOrderAllQuantityGroupedProductOptionDefaultStockTest

Hence moving this to Merge in progress.

engcom-November avatar Aug 28 '24 12:08 engcom-November

@magento create issue

engcom-November avatar Aug 30 '24 06:08 engcom-November