[FrameworkBundle] Catch Fatal errors in commands registration #23836

chalasr · 2017-08-09T08:52:15Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#23610
License	MIT
Doc PR	n/a

before

after

Trying to run the failing command itself

Exceptions/errors thrown in registerCommands() (bundles/non-lazy command services registration) are caught and displayed as a warning, allowing to run other valid commands.

stof · 2017-08-09T09:04:59Z

src/Symfony/Bundle/FrameworkBundle/Console/Application.php

        return parent::doRun($input, $output);
    }

    /**
     * {@inheritdoc}
     */
+    final protected function doRunCommand(Command $command, InputInterface $input, OutputInterface $output)


making it final when overwriting the method is a BC break. People extending the class were allowed to overwrite the method previously

nicolas-grekas · 2017-08-09T09:08:47Z

src/Symfony/Bundle/FrameworkBundle/Console/Application.php

+    private function renderRegistrationErrors(OutputInterface $output)
+    {
+        $output->writeln('', OutputInterface::VERBOSITY_QUIET);
+        $this->doRenderException(new CommandRegistrationFailedException('Some commands could not be registered.'), $output);


I would use a RuntimeException instead of adding another exception

nicolas-grekas · 2017-08-09T09:41:08Z

src/Symfony/Bundle/FrameworkBundle/Console/Application.php

+        $this->doRenderException(new RuntimeException('Some commands could not be registered.'), $output);
+        foreach ($this->registrationErrors as $error) {
+            $this->doRenderException($error, $output);
+        }


should registrationErrors be resetted once rendered?

nicolas-grekas · 2017-08-09T09:42:27Z

src/Symfony/Component/Console/Application.php

+    }
+
+    /**
+     * @internal


I think I'd suggest removing this one: using things cross components/bundles is a trigger that this is not so internal anymore - cross-version constraints would anyway prevent doing non BC things here

ro0NL · 2017-08-09T11:37:06Z

do we care some commands could not be registered while running another command? When running the actual command i think 1 red block (the middle) is fine =/

ro0NL · 2017-08-09T11:42:36Z

src/Symfony/Bundle/FrameworkBundle/Console/Application.php

@@ -70,12 +73,28 @@ public function doRun(InputInterface $input, OutputInterface $output)

        $this->setDispatcher($this->kernel->getContainer()->get('event_dispatcher'));

+        if ($this->registrationErrors) {
+            $this->renderRegistrationErrors($output);


does this happen?

yes, when trying to run the failing command which could not be registered thus doesn't exist

but it will render in doRunCommand no?

nope, doRunCommand is never reached if the command cannot be found

Right. Still not sure whats populating registrationErrors :S id expected it to be set after parent::doRun.

https://github.com/symfony/symfony/blob/3.4/src/Symfony/Component/Console/Application.php#L216 being the trigger

actually it's triggered at application construction https://github.com/symfony/symfony/blob/3.4/src/Symfony/Component/Console/Application.php#L92 :) (calls add() which then calls registerCommands())

hence these errors are actually uncaught, they prevent the app from being constructed

Of course :) thx.

Still.. this could also happen for lazy commands from the command loader. I dont think thats covered and happens in between currently...

edit: never mind... those are caught already.

not sure to understand. Lazy command names are known beforehand and cannot fail before being ran since they are lazy, nothing to change for them to me

edit: just see your edit :)

nicolas-grekas · 2017-08-09T11:52:32Z

I agree with @ro0NL about the "1 red block is enough". We could just list the command that fail to be loaded, and not display the corresponding exception. But then, when that failing command is run, we should display the exception of course. WDYT?

unapplicable as the failure prevents knowing the name of the command

ro0NL · 2017-08-09T12:26:08Z

Maybe simple and effective; just render all registration exceptions and abort the run. It's an invalid state and currently already crashes. Not sure tracking command<>exception is worth it in effort to run another valid command (this is new behavior ;-)).

edit: omg.. this day :) it's feature request not a bugfix :) my bad. Still not sure about it though.

nicolas-grekas · 2017-08-09T12:32:34Z

worth it in effort to run another valid command

that's exactly what this PR tries to fix: running cache:clear while some other commands are broken. So YES, it's worth the effort :)

ro0NL · 2017-08-09T12:41:05Z

You're right. i went over the related issue way to fast.

But im not sure if linking exceptions from auto-discovery with command names is worth it, especially since thats going to be deprecated.

Also warnings being red bites.

chalasr · 2017-08-09T12:53:21Z

do we care some commands could not be registered while running another command?

we don't know what command failed since we don't have their names before they are successfully booted, so we have to display the warning + errors when running any valid command.

chalasr · 2017-08-09T15:15:22Z

PR updated, first exception replaced by a warning.

nicolas-grekas · 2017-08-09T15:20:34Z

src/Symfony/Bundle/FrameworkBundle/Console/Application.php

+    protected function doRunCommand(Command $command, InputInterface $input, OutputInterface $output)
+    {
+        if ($this->registrationErrors) {
+            $this->renderRegistrationErrors($input, output);


chalasr · 2017-08-09T16:39:38Z

(updated to make use of stderr when available)

fabpot · 2017-08-10T06:58:23Z

Thank you @chalasr.

…tration (chalasr) This PR was merged into the 3.4 branch. Discussion ---------- [FrameworkBundle] Catch Fatal errors in commands registration | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #23610 | License | MIT | Doc PR | n/a #### before ![before](https://image.prntscr.com/image/QDmEVSPiQY6VCY7PoNAIbg.png) #### after ![after](https://image.prntscr.com/image/B2EuZtzwTfiVS6F3kTAa4Q.png) Trying to run the failing command itself ![failing](https://image.prntscr.com/image/7RgdtCVyQXyVit8TPkh98A.png) Exceptions/errors thrown in `registerCommands()` (bundles/non-lazy command services registration) are caught and displayed as a warning, allowing to run other valid commands. Commits ------- 46b6b42 [FrameworkBundle] Catch Fatal errors in commands registration

jean-pasqualini · 2017-08-11T07:52:24Z

🎉

carsonbot added Status: Needs Review FrameworkBundle Feature labels Aug 9, 2017

chalasr added the DX DX = Developer eXperience (anything that improves the experience of using Symfony) label Aug 9, 2017

chalasr force-pushed the console/catch-unregistrable-command branch from 2f4ad61 to 56bd99a Compare August 9, 2017 08:53

chalasr mentioned this pull request Aug 9, 2017

[Console] DX: Catch Fatal errors in Command collection #23610

Closed

stof requested changes Aug 9, 2017

View reviewed changes

nicolas-grekas reviewed Aug 9, 2017

View reviewed changes

chalasr force-pushed the console/catch-unregistrable-command branch 2 times, most recently from 48b3ade to 7dd1aaf Compare August 9, 2017 09:31

nicolas-grekas reviewed Aug 9, 2017

View reviewed changes

ro0NL reviewed Aug 9, 2017

View reviewed changes

chalasr force-pushed the console/catch-unregistrable-command branch from 7dd1aaf to ae844b3 Compare August 9, 2017 13:20

nicolas-grekas reviewed Aug 9, 2017

View reviewed changes

nicolas-grekas approved these changes Aug 9, 2017

View reviewed changes

chalasr force-pushed the console/catch-unregistrable-command branch from ae844b3 to 26b0c95 Compare August 9, 2017 15:22

ro0NL approved these changes Aug 9, 2017

View reviewed changes

ogizanagi approved these changes Aug 9, 2017

View reviewed changes

chalasr force-pushed the console/catch-unregistrable-command branch from 26b0c95 to ff67dd8 Compare August 9, 2017 16:37

[FrameworkBundle] Catch Fatal errors in commands registration

46b6b42

chalasr force-pushed the console/catch-unregistrable-command branch from ff67dd8 to 46b6b42 Compare August 9, 2017 16:38

chalasr added this to the 3.4 milestone Aug 9, 2017

fabpot merged commit 46b6b42 into symfony:3.4 Aug 10, 2017

chalasr deleted the console/catch-unregistrable-command branch August 10, 2017 07:25

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FrameworkBundle] Catch Fatal errors in commands registration #23836

[FrameworkBundle] Catch Fatal errors in commands registration #23836

chalasr commented Aug 9, 2017 •

edited

Loading

stof Aug 9, 2017

nicolas-grekas Aug 9, 2017

nicolas-grekas Aug 9, 2017

nicolas-grekas Aug 9, 2017 •

edited

Loading

ro0NL commented Aug 9, 2017

ro0NL Aug 9, 2017

chalasr Aug 9, 2017

ro0NL Aug 9, 2017

chalasr Aug 9, 2017

ro0NL Aug 9, 2017

chalasr Aug 9, 2017

chalasr Aug 9, 2017

ro0NL Aug 9, 2017 •

edited

Loading

chalasr Aug 9, 2017 •

edited

Loading

nicolas-grekas commented Aug 9, 2017 •

edited

Loading

ro0NL commented Aug 9, 2017 •

edited

Loading

nicolas-grekas commented Aug 9, 2017 •

edited

Loading

ro0NL commented Aug 9, 2017

chalasr commented Aug 9, 2017

chalasr commented Aug 9, 2017

nicolas-grekas Aug 9, 2017

chalasr Aug 9, 2017

chalasr commented Aug 9, 2017

fabpot commented Aug 10, 2017

jean-pasqualini commented Aug 11, 2017

[FrameworkBundle] Catch Fatal errors in commands registration #23836

[FrameworkBundle] Catch Fatal errors in commands registration #23836

Conversation

chalasr commented Aug 9, 2017 • edited Loading

before

after

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

ro0NL commented Aug 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0NL Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

chalasr Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas commented Aug 9, 2017 • edited Loading

ro0NL commented Aug 9, 2017 • edited Loading

nicolas-grekas commented Aug 9, 2017 • edited Loading

ro0NL commented Aug 9, 2017

chalasr commented Aug 9, 2017

chalasr commented Aug 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr commented Aug 9, 2017

fabpot commented Aug 10, 2017

jean-pasqualini commented Aug 11, 2017

chalasr commented Aug 9, 2017 •

edited

Loading

nicolas-grekas Aug 9, 2017 •

edited

Loading

ro0NL Aug 9, 2017 •

edited

Loading

chalasr Aug 9, 2017 •

edited

Loading

nicolas-grekas commented Aug 9, 2017 •

edited

Loading

ro0NL commented Aug 9, 2017 •

edited

Loading

nicolas-grekas commented Aug 9, 2017 •

edited

Loading