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

Fixes incorrect classes being referenced in Magento modules. #37784

Open
wants to merge 5 commits into
base: 2.4-develop
Choose a base branch
from

Conversation

hostep
Copy link
Contributor

@hostep hostep commented Jul 21, 2023

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 #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 d7ff74d#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 (*)

  • 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)
  • 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)

@m2-assistant
Copy link

m2-assistant bot commented Jul 21, 2023

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

❗ 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
ℹ️ 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.

@hostep
Copy link
Contributor Author

hostep commented Jul 21, 2023

@magento run all tests

@magento-automated-testing
Copy link

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.

@m2-community-project m2-community-project bot added this to Pending Review in Pull Requests Dashboard Jul 21, 2023
@hostep hostep force-pushed the cleanup-some-non-existing-classes-from-magento-modules branch from f5663d3 to 09f3deb Compare July 21, 2023 16:15
@hostep
Copy link
Contributor Author

hostep commented Jul 21, 2023

@magento run all tests

@magento-automated-testing
Copy link

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.

@hostep
Copy link
Contributor Author

hostep commented Jul 21, 2023

@magento run all tests

@magento-automated-testing
Copy link

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.

@hostep
Copy link
Contributor Author

hostep commented Jul 22, 2023

@magento run all tests

@magento-automated-testing
Copy link

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.

@hostep
Copy link
Contributor Author

hostep commented Jul 22, 2023

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! 🙂

@engcom-Charlie engcom-Charlie added the Priority: P2 A defect with this priority could have functionality issues which are not to expectations. label Jul 25, 2023
@hostep hostep force-pushed the cleanup-some-non-existing-classes-from-magento-modules branch from 4a962f5 to 105ef98 Compare April 17, 2024 07:23
@hostep
Copy link
Contributor Author

hostep commented Apr 17, 2024

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 9ab6691

@magento run all tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Progress: pending review
Projects
Pull Requests Dashboard
  
Pending Review
Development

Successfully merging this pull request may close these issues.

None yet

2 participants