Fix for running bin/magento without proper database connection (removes db connection attempts from DI constructors)

[hostep]

Fix for running bin/magento without proper database connection (removes db connection attempts from DI constructors)

Remove obsolete ResourceModel\Product dependency and reworked DateTime to DateTimeFactory. Both of these caused unnecessary connections to be made to the database when executing bin/magento commands.

Description (*)

Whenever you execute bin/magento (with our without explicit commands), Magento will load all registered Commands and load all their dependencies through DI. Those dependencies will load their own dependencies, and so on and so forth.

I've noticed that through the whole depenencies tree, some dependencies tried to fetch something from the database. This is really not needed when executing bin/magento without providing an explicit command to actually do something with the database.

The removal of the Magento\Catalog\Model\ResourceModel\Product dependency should be safe, as it wasn't being used in the class anyways.
The change of the DateTime to DateTimeFactory causes that no locale info from the database is being requested in the constructor. It should only happen at runtime and was inspired by this code here.

These 2 dependencies were loaded from the commands bin/magento media-gallery:sync & bin/magento media-content:sync

Related Pull Requests

Fixed Issues (if relevant)

1. Fixes Running setup:config:set can crash in certain situations, seems to be bug introduced in Magento 2.4.1 #37409
2. Fixes php bin/magento setup:di:compile requires database connection #36845

Manual testing scenarios (*)

1. Setup clean Magento and install it
2. Modify the app/etc/env.php file and change the correct database user to an incorrect one
3. Run bin/magento
4. Expected is that no error messages show up until you actually want to do something to the database with an explicit command
5. Change back to a correct database user in the app/etc/env.php file
6. Make sure both the commands bin/magento media-gallery:sync & bin/magento media-content:sync still work as expected, those should be the only real commands affected by these changes

Questions or comments

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]

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]

@magento run all tests

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

[ihor-sviziev]

@hostep thank you so much for your improvement!
Your changes look good!
PS: I hope one day someone will finalize this one #29355, it will way more significantly improve performance of CLI

[ihor-sviziev]

✔ Approved.

Failing tests look not related to changes from this PR.

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

[ihor-sviziev]

@magento run all tests

[magento-automated-testing]

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.

[engcom-Echo]

@magento run all tests

[magento-automated-testing]

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]

@magento run all tests

[magento-automated-testing]

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.

[engcom-Echo]

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

[magento-automated-testing]

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.

[engcom-Echo]

Functional Tests B2B and Functional Tests EE failures are different on last two run on same commit.
Functional Tests EE
Run1

Run2

Functional Tests B2B
Run1

Run2

Hence moving it to Merge In Progress

[engcom-Charlie]

@magento run all tests

[magento-automated-testing]

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]

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]

It looks like this PR got merged in 2.4-develop recently via: d4a9791

But this PR is not marked as merged, which is quite strange? Should I close it myself?

[ihor-sviziev]

This pull request was merged in 4095595.
Unfortunately, it's not marked as merged due to not merged a "merge commit" :(
Since changes are already in 2.4-develop branch, I'm closing this PR.

[dfelton]

@ihor-sviziev Just want to call out that this certainly appears to be a backwards incompatible change, and for that reason I'm surprised it was merged into 2.4-develop as opposed to 2.5-develop.

• The revision to app/code/Magento/MediaContentCatalog/Model/ResourceModel/GetEntityContent.php is rather harmless... the removed constructor parameter was the last argument. And therefore, if any existing code (thinking third party code as opposed to Magento native) was extending it and calling parent::__construct() with a (now extra) argument. This will merely be ignored per how PHP treats such things.

• However, the change to app/code/Magento/MediaGallerySynchronization/Model/SynchronizeFiles.php is definitely backwards breaking. Any existing code that extends this class, that defines its own constructor, and attempts to call parent::__construct() without having DateTime updated to DateTimeFactory will now be broken.

[hostep]

@dfelton: I certainly agree that it's backwards compatible breaking in case 3rd party modules extends those classes, but Magento has this rule they only disallow such code changes on classes that are annotated with @api, which isn't the case here.

I don't think a lot of 3rd party modules will have extended these classes and fixing them would be pretty easy (unless they have to support both older Magento versions and upcoming ones at the same time, but even that can be solved, it's only a bit more complex though, here's an example).

[dfelton]

but Magento has this rule they only disallow such code changes on classes that are annotated with @api

Ah, thank you for this piece of info @hostep. This makes sense.

I felt compelled to mention it because I have had an open pr with Magento since 2019 for a much smaller change, that is taking forever to get merged in due to backwards incompatibility. However, as you point out, classes marked @api get this extra scrutiny.

Cheers!