-
Notifications
You must be signed in to change notification settings - Fork 9.4k
Fix Wrong Exception on Cli run when Command Not found #40063
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
base: 2.4-develop
Are you sure you want to change the base?
Conversation
|
Hi @Genaker. Thank you for your contribution!
Allowed build names are:
You can find more information about the builds here For more details, review the Code Contributions documentation. |
|
Just wonder if we can do a bit different approach - throw different type of exception when the command not found, and handle it differently. What do you think? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR improves error handling in the Magento CLI system when commands fail to initialize, providing developers with more detailed exception information instead of the generic "no commands defined in namespace" error. The change enhances debugging experience by showing the actual root cause when command classes are broken due to missing dependencies, constructor errors, or other issues.
- Adds exception tracking for command initialization failures
- Implements clearer error messaging that shows the original exception details
- Moves setup command initialization to occur before other commands for better error isolation
| * GetCommands exception. | ||
| * |
Copilot
AI
Jul 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docblock comment 'GetCommands exception' should be more descriptive. Consider: 'Exception thrown during command list initialization.'
| * GetCommands exception. | |
| * | |
| * Exception thrown during command list initialization. | |
| * |
| $combinedErrorMessage = "Exception during console commands initialization: " . | ||
| $this->getCommandsException->getMessage() . PHP_EOL; |
Copilot
AI
Jul 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The string concatenation spans multiple lines unnecessarily. Consider combining into a single line or using sprintf for better readability: sprintf('Exception during console commands initialization: %s%s', $this->getCommandsException->getMessage(), PHP_EOL)
| $combinedErrorMessage = "Exception during console commands initialization: " . | |
| $this->getCommandsException->getMessage() . PHP_EOL; | |
| $combinedErrorMessage = sprintf( | |
| 'Exception during console commands initialization: %s%s', | |
| $this->getCommandsException->getMessage(), | |
| PHP_EOL | |
| ); |
| if (class_exists(\Magento\Setup\Console\CommandList::class)) { | ||
| $setupCommandList = new \Magento\Setup\Console\CommandList($this->serviceManager); | ||
| $commands = array_merge($commands, $setupCommandList->getCommands()); | ||
| } | ||
|
|
||
| if ($this->objectManager->get(DeploymentConfig::class)->isAvailable()) { |
Copilot
AI
Jul 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The setup command initialization has been moved outside the deployment config check, which changes the execution order. This should be documented or the reasoning should be clearer, as it may affect command availability in different deployment states.
| if (class_exists(\Magento\Setup\Console\CommandList::class)) { | |
| $setupCommandList = new \Magento\Setup\Console\CommandList($this->serviceManager); | |
| $commands = array_merge($commands, $setupCommandList->getCommands()); | |
| } | |
| if ($this->objectManager->get(DeploymentConfig::class)->isAvailable()) { | |
| if ($this->objectManager->get(DeploymentConfig::class)->isAvailable()) { | |
| if (class_exists(\Magento\Setup\Console\CommandList::class)) { | |
| $setupCommandList = new \Magento\Setup\Console\CommandList($this->serviceManager); | |
| $commands = array_merge($commands, $setupCommandList->getCommands()); | |
| } |
| if (class_exists(\Magento\Setup\Console\CommandList::class)) { | ||
| $setupCommandList = new \Magento\Setup\Console\CommandList($this->serviceManager); | ||
| $commands = array_merge($commands, $setupCommandList->getCommands()); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please explain, why this change is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From reply in #40063 (comment):
We don't need these lines; it is probably due to a merge issue. You can
follow the logic. These lines don't change logic, just were touched by the
commit
These lines were added, so let's remove them if we don't need them
|
We don't need these lines; it is probably due to a merge issue. You can
follow the logic. These lines don't change logic, just were touched by the
commit
…On Thu, Jul 17, 2025 at 1:52 AM Ihor Sviziev ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lib/internal/Magento/Framework/Console/Cli.php
<#40063 (comment)>:
> + if (class_exists(\Magento\Setup\Console\CommandList::class)) {
+ $setupCommandList = new \Magento\Setup\Console\CommandList($this->serviceManager);
+ $commands = array_merge($commands, $setupCommandList->getCommands());
+ }
Could you please explain, why this change is needed?
—
Reply to this email directly, view it on GitHub
<#40063 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACGJNZRP4HH4GAEE6SC2L5L3I5P4JAVCNFSM6AAAAACBHOAE7KVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTAMRYGU3TQMJUGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
You can. The main issue is that we need to show which command failed, not
just the command that was not found. The best solution could be to allow
the execution of other commands and skip the command with an error.
…On Thu, Jul 17, 2025 at 1:48 AM Ihor Sviziev ***@***.***> wrote:
*ihor-sviziev* left a comment (magento/magento2#40063)
<#40063 (comment)>
Just wonder if we can do a bit different approach - throw different type
of exception when the command not found, and handle it differently. What do
you think?
—
Reply to this email directly, view it on GitHub
<#40063 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACGJNZVFDFNLAFOKNSXD4D33I5PO5AVCNFSM6AAAAACBHOAE7KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTAOBTGIYDGMZVGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
I've tried to debug a bit, and feels like the issue is coming from the masking original exception - if exception happened during loading list of the commands - we're just skipping it and trying to print the failure message after, but command is trying to be executed.
Ref:
magento2/lib/internal/Magento/Framework/Console/Cli.php
Lines 150 to 169 in 16c2c4a
| protected function getApplicationCommands() | |
| { | |
| $commands = []; | |
| try { | |
| if ($this->objectManager->get(DeploymentConfig::class)->isAvailable()) { | |
| /** @var CommandListInterface */ | |
| $commandList = $this->objectManager->create(CommandListInterface::class); | |
| $commands = array_merge($commands, $commandList->getCommands()); | |
| } | |
| $commands = array_merge( | |
| $commands, | |
| $this->getVendorCommands($this->objectManager) | |
| ); | |
| } catch (\Exception $e) { | |
| $this->initException = $e; | |
| } | |
| return $commands; | |
| } |
This code was introduced long time ago in 293ba1e
I think this logic has to be changed - we should not hide the exception, but print it directly when it happens - just remove try-catch in the getApplicationCommands method.
This will result in having a clear exception at the beginning:
$ php bin/magento -vvv
In ClassReader.php line 34:
[ReflectionException (-1)]
Class "Test\TEsctCli\Console\Command\TestWrong" does not exist
Exception trace:
at /var/www/html/vendor/magento/framework/Code/Reader/ClassReader.php:34
ReflectionClass->__construct() at /var/www/html/vendor/magento/framework/Code/Reader/ClassReader.php:34
Magento\Framework\Code\Reader\ClassReader->getConstructor() at /var/www/html/vendor/magento/framework/ObjectManager/Definition/Runtime.php:50
Magento\Framework\ObjectManager\Definition\Runtime->getParameters() at /var/www/html/vendor/magento/framework/ObjectManager/Factory/Dynamic/Developer.php:48
Magento\Framework\ObjectManager\Factory\Dynamic\Developer->create() at /var/www/html/vendor/magento/framework/ObjectManager/ObjectManager.php:73
Magento\Framework\ObjectManager\ObjectManager->get() at /var/www/html/vendor/magento/framework/ObjectManager/Factory/AbstractFactory.php:206
Magento\Framework\ObjectManager\Factory\AbstractFactory->parseArray() at /var/www/html/vendor/magento/framework/ObjectManager/Factory/AbstractFactory.php:182
Magento\Framework\ObjectManager\Factory\AbstractFactory->resolveArgument() at /var/www/html/vendor/magento/framework/ObjectManager/Factory/AbstractFactory.php:276
Magento\Framework\ObjectManager\Factory\AbstractFactory->getResolvedArgument() at /var/www/html/vendor/magento/framework/ObjectManager/Factory/AbstractFactory.php:239
Magento\Framework\ObjectManager\Factory\AbstractFactory->resolveArgumentsInRuntime() at /var/www/html/vendor/magento/framework/ObjectManager/Factory/Dynamic/Developer.php:34
Magento\Framework\ObjectManager\Factory\Dynamic\Developer->_resolveArguments() at /var/www/html/vendor/magento/framework/ObjectManager/Factory/Dynamic/Developer.php:59
Magento\Framework\ObjectManager\Factory\Dynamic\Developer->create() at /var/www/html/vendor/magento/framework/ObjectManager/ObjectManager.php:59
Magento\Framework\ObjectManager\ObjectManager->create() at /var/www/html/vendor/magento/framework/Console/Cli.php:156
Magento\Framework\Console\Cli->getApplicationCommands() at /var/www/html/vendor/magento/framework/Console/Cli.php:137
Magento\Framework\Console\Cli->getDefaultCommands() at /var/www/html/vendor/symfony/console/Application.php:1327
Symfony\Component\Console\Application->init() at /var/www/html/vendor/symfony/console/Application.php:692
Symfony\Component\Console\Application->find() at /var/www/html/vendor/symfony/console/Application.php:266
Symfony\Component\Console\Application->doRun() at /var/www/html/vendor/magento/framework/Console/Cli.php:118
Magento\Framework\Console\Cli->doRun() at /var/www/html/vendor/symfony/console/Application.php:175
Symfony\Component\Console\Application->run() at /var/www/html/bin/magento:23
What do you think about it?
Correct this approach works also but i would like to have more descriptive message to tell user something wrong not just default exception. For example check you extension not just : ReflectionException. Even we can check if it is file exist but has error, or it is file doesn't exist but in di.xml/ also we can show/log exception and continue or original command execution. Because after this you can't even cache clear you need do this manually. |
|
I agree that cache flush should work, but TBH I don’t like the current solution. Maybe @jakwinkler can suggest something more suitable? |
|
Hello @jakwinkler , |
|
Hello @jakwinkler @ihor-sviziev @Genaker — since there hasn’t been any feedback for quite some time, could you please advise if we can proceed with closing this PR due to inactivity? Thank you! |
|
Hi there,
Please do not close this Pull Request. The changes in PR #40063 are
essential for improving the Magento CLI debugging experience by providing
more informative exceptions when command initialization fails.
I understand the discussions around alternative approaches; however, the
current implementation significantly improves the masked error. I am ready
to address any further feedback to advance this PR.
This PR resolves a valid issue and should be merged, not closed due to
inactivity.
Thanks,
Yegor
…On Fri, Oct 31, 2025 at 7:01 AM Harshit Yadav ***@***.***> wrote:
*engcom-Dash* left a comment (magento/magento2#40063)
<#40063 (comment)>
Hello @jakwinkler <https://github.com/jakwinkler>
Following up to check if you had a chance to review or provide any updates
on this PR.
@ihor-sviziev <https://github.com/ihor-sviziev> @Genaker
<https://github.com/Genaker> — since there hasn’t been any feedback for
quite some time, could you please advise if we can proceed with closing
this PR due to inactivity?
Thank you!
—
Reply to this email directly, view it on GitHub
<#40063 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACGJNZQHBQNDVFTUWLMCKO332NTURAVCNFSM6AAAAACBHOAE7KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTINZTGIZDEMJRGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Hi @Genaker, @ihor-sviziev, @engcom-Hotel, please review the current implementation and share any additional feedback or suggestions so we can move this PR forward. @jakwinkler, if you have time, please provide any suggestions or input on this PR as well. |
|
@magento run all tests |
engcom-Hotel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @Genaker,
Thank you for your contribution!
I have reviewed the PR and your approach to solving the issue is good. However, I also agree with @ihor-sviziev's comment here about not masking the original exception.
I'd like to suggest a hybrid approach that provides the best of both solutions:
- Developer mode: Throw exceptions immediately (fail-fast for debugging)
- Production mode: Log errors and continue loading other commands (graceful degradation)
Modify getVendorCommands() to capture individual failures:
protected function getVendorCommands($objectManager)
{
$commands = [];
foreach (CommandLocator::getCommands() as $commandListClass) {
if (!class_exists($commandListClass)) {
continue;
}
try {
$commands[] = $objectManager->create($commandListClass)->getCommands();
} catch (\Exception $e) {
// Store failure in class property
$this->failedCommands[] = [
'class' => $commandListClass,
'error' => $e->getMessage(),
'type' => get_class($e),
'file' => $e->getFile() . ':' . $e->getLine()
];
// Log the error
$this->logger->error(
sprintf('Failed to load command class %s: %s', $commandListClass, $e->getMessage()),
['exception' => $e, 'trace' => $e->getTraceAsString()]
);
// Developer mode: fail immediately
if ($this->isDeveloperMode()) {
throw $e;
}
// Production mode: continue
}
}
return array_merge([], ...$commands);
}
Let me know your views.
Thank you
|
Hi,
Thank you for reviewing the suggestion and providing your perspective.
I agree that the hybrid approach you outlined—throwing the original
exception immediately in developer mode and gracefully logging the error
while continuing in production mode—represents a significant improvement.
This solution perfectly balances the need for a "fail-fast" debugging
experience with production stability, which is highly desirable for the
Magento CLI.
The goal has always been to stop masking critical exceptions, and your
proposed modification to `getVendorCommands()` achieves this while ensuring
the CLI remains usable in a production environment even if a single command
fails to initialize. I believe this hybrid solution is superior to the
current PR implementation and the previous masked error.
Please go ahead and adopt this logic.
I don't have time now.
Thank you once again for your detailed feedback and assistance in moving
this PR toward a high-quality solution.
Yegor
…On Wed, Nov 5, 2025 at 1:55 AM Abhinav Pathak ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Hello @Genaker <https://github.com/Genaker>,
Thank you for your contribution!
I have reviewed the PR and your approach to solving the issue is good.
However, I also agree with @ihor-sviziev <https://github.com/ihor-sviziev>'s
comment here
<#40063 (review)>
about not masking the original exception.
I'd like to suggest a hybrid approach that provides the best of both
solutions:
- Developer mode: Throw exceptions immediately (fail-fast for
debugging)
- Production mode: Log errors and continue loading other commands
(graceful degradation)
*Modify getVendorCommands() to capture individual failures:*
protected function getVendorCommands($objectManager)
{
$commands = [];
foreach (CommandLocator::getCommands() as $commandListClass) {
if (!class_exists($commandListClass)) {
continue;
}
try {
$commands[] = $objectManager->create($commandListClass)->getCommands();
} catch (\Exception $e) {
// Store failure in class property
$this->failedCommands[] = [
'class' => $commandListClass,
'error' => $e->getMessage(),
'type' => get_class($e),
'file' => $e->getFile() . ':' . $e->getLine()
];
// Log the error
$this->logger->error(
sprintf('Failed to load command class %s: %s', $commandListClass, $e->getMessage()),
['exception' => $e, 'trace' => $e->getTraceAsString()]
);
// Developer mode: fail immediately
if ($this->isDeveloperMode()) {
throw $e;
}
// Production mode: continue
}
}
return array_merge([], ...$commands);
}
Let me know your views.
Thank you
—
Reply to this email directly, view it on GitHub
<#40063 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACGJNZTZ666CEO5AZDKCVTL33HCSBAVCNFSM6AAAAACBHOAE7KVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTIMRQHE3DGNRRGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Before Fix error on Cli run:
After :
Description (*)
If you have ever tried to run bin/magento and been greeted with the maddening: "There is no command found in the namespace"
you already know how opaque Magento (Adobe Commerce) can be when a single CLI command class is broken. One constructor typo, a missing dependency, or a bad module upgrade and—boom—the whole console goes dark. No stack-trace, no filename, no clue. You are left spelunking through logs or disabling modules by trial and error. More than 10 years Adobe is not able to fix this critical issue! I'm tired and submitted the PR.
What My PR tiny but vital quality-of-life improvement:
Catches the original exception thrown while Magento is assembling its command list.
Prints the real message and class name before the fallback “no command found” notice.
The change is only a few lines, but the impact is huge: you immediately see which command class failed and why, so you can fix the root cause in minutes instead of hours.
Why every extension author & integrator should care:
Faster CI/CD – Broken modules fail loudly during automated builds; no more silent time-outs.
Happier merchants – Support teams pinpoint faulty third-party code instead of asking store owners for full backups.
Lean core – The patch touches only CLI bootstrap code; zero risk to storefront performance.
Thanks for backing real-world developer experience improvements—your click today could save countless debugging hundreds hours tomorrow!
Fixes #40006
Related Issue