magento/magento2#20773: Do not throw exception during autoload

[Vinai]

Description (*)

Avoid uncaught exceptions during autoloading. PSR-4 dictates that must not happen. Return false instead.
Instead, try to log the exception message.

Fixed Issues (if relevant)

#20773

Manual testing scenarios (*)

Run class_exists(FooFactory::class);.
More information to reproduce the problem is in the issue thread.

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)

[magento-engcom-team]

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

[davidverholen]

Hey @Vinai , since autoloader registration happens before the objectmanager is fully initialized in \Magento\Framework\App\ObjectManagerFactory::create, it now (ironically) throws an exception during the construction of the autoloader.

Instead you would have to manually create the logger instance and inject it here \Magento\Framework\ObjectManager\DefinitionFactory::createClassDefinition

Not sure how complicated it is to initialize the logger without the OM. Unfortunately I did not find it in any of the calling classes to maybe pass it through.

Just another thing: Wouldn't it be more helpful (even if it's nor psr4 compliant) to keep the exceptions at least in developer mode so they are just displayed directly instead of "hidden" in a log file?

[Vinai]

He, stupid of me, sorry. The contribution day ended so I ran out of time to test it.
I’m going to fix it tomorrow.
Thanks!

[Vinai]

This was a typical case of Unit Tests green, missing integration tests.
Fixed now :)

[Vinai]

@davidverholen I decided against letting the exception bubble up in developer mode, because that still would prohibit using another autoloader later in the autoloader chain (which is the original reason for the PR). Instead, the code now just tries to log the message if the ObjectManager has been initialized, otherwise there is nothing that can be done and the exception message is lost.
In almost all cases the logger should be available during a generation failure.

[magento-engcom-team]

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

[Vinai]

Thanks for reviewing, @davidverholen! I just noticed some typos and couldn't resist fixing them and updating the PR. Sorry if that means you have to repeat the review - at least it's a small patch.

[orlangur]

Could you please inline this method? I don't see how it improves readability and also ObjectManager::getInstance() is used as is at least once.

[Vinai]

The return type hint fixes the type inference in PHPStorm (see screenshot).

[Vinai]

Apropos: I should make a PR to fix the return annotation on getInstance on the test framework ObjectManager - this has been annoying me for 2.5 years now.

[orlangur]

@Vinai yep, there is no need to bloat each single test.

[orlangur]

Maybe just call them setUp/tearDown? Why public?

[Vinai]

I prefer not to override parent class methods. The @before and @after annotations on public methods with descriptive names are preferable.

[orlangur]

Such name does not add any value compared to just setUp. I have nothing against such annotations in general but then we should make it an official recommendation, otherwise the code become inconsistent.

[Vinai]

The warnings that are causing Codacy to fail are unrelated to the PR, but because of outdated rulesets.

[magento-engcom-team]

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

[Vinai]

I think the failing test is unrelated to this PR.
Is there anything I can do to move this PR forward?

[ghost]

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

[Vinai]

Thanks for merging 👍 👍

I'd like to take the survey, but the app wants full write access to all my orgs:

This seems like a bad idea, so I'll give it a pass.

[orlangur]

@Vinai LOOOL :) Thanks for pointing out, I'll pass it to EngCom guys.

[Vinai]

The same survey app is used for devdoc PRs, too, by the way.