[Console] Add domain exceptions to replace generic exceptions #14894

GromNaN · 2015-06-06T14:34:56Z

Creates domain specific exception classes for the case where a user type an invalid command name or option name.

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#14873
License	MIT
Doc PR	N/A

TODO:

Replace \InvalidArgumentException by Symfony\Component\Console\Exception\InvalidArgumentException
Add Symfony\Component\Console\Exception\ExceptionInterface

GromNaN · 2015-06-06T14:48:54Z

src/Symfony/Component/Console/Tests/Fixtures/application_renderexception4.txt

+
+  [Symfony\Component\Console\Exception\CommandNotDefinedException]  
+  Command "foo" is not define                                       
+  d.                                                                


I'm not sure this is the correct behavior. The line is wrapped, but not the class name, and the rest of the line is filled with spaces.

I think d should be on the same line as define.

GromNaN · 2015-06-07T20:24:23Z

Any idea how to fix this failing test ?
https://travis-ci.org/symfony/symfony/jobs/65716176#L438

 1) Symfony\Component\Console\Tests\ApplicationTest::testFindAlternativeNamespace
 Failed asserting that Array &0 (
    0 => 'foo'
    1 => 'foo1'
    2 => 'foo3'
) is identical to Array &0 (
    0 => 'foo3'
    1 => 'foo1'
    2 => 'foo'
).
 /home/travis/build/symfony/symfony/src/Symfony/Component/Console/Tests/ApplicationTest.php:444

phansys · 2015-06-07T20:39:29Z

AFAIK, the alternative names was originally ordered with levenshtein(), but I don't know if your issue is related to this.

phansys · 2015-06-07T20:46:19Z

src/Symfony/Component/Console/Exception/CommandNotDefinedException.php

+    {
+        return $this->alternatives;
+    }
+}


Please, check missing new lines at EOFs.

fabpot · 2015-08-01T14:20:54Z

src/Symfony/Component/Console/Tests/ApplicationTest.php

@@ -176,7 +176,7 @@ public function testSilentHelp()
    }

    /**
-     * @expectedException        \InvalidArgumentException
+     * @expectedException        Symfony\Component\Console\Exception\CommandNotDefinedException


Missing leading \ on all @expectedException tags

Can you fix this? We are using a leading \ everywhere else in Symfony for such PHPUnit annotations.

fabpot · 2015-08-01T14:24:34Z

As you are moving exceptions to specific ones, what about doing it for the whole component?

fabpot · 2015-09-14T07:07:48Z

@GromNaN Any news on this one? Can you address the comments?

GromNaN · 2015-09-22T11:38:26Z

OK for moving remaining exception to specific ones. Is it necessary to keep the distinction between InvalidArgumentException and LogicException in classes like Console/Question/Question ?

fabpot · 2015-09-22T11:42:49Z

Yes, I think we need to keep the distinction here (at least to keep BC).

GromNaN · 2015-09-22T12:10:32Z

I'll create Symfony\Component\Console\Exception\InvalidArgumentException and Symfony\Component\Console\Exception\LogicException

And the interface Symfony\Component\Console\Exception\ExceptionInterface

…rgumentException

…entException

Replace exception thrown by specific exception implementing the same interface.

GromNaN · 2015-09-23T19:57:50Z

Rebased and made all changes.

I still have this error on HHVM :

1) Symfony\Component\Console\Tests\ApplicationTest::testFindAlternativeNamespace
Failed asserting that Array &0 (
    0 => 'foo'
    1 => 'foo1'
    2 => 'foo3'
) is identical to Array &0 (
    0 => 'foo3'
    1 => 'foo1'
    2 => 'foo'
).

fabpot · 2015-09-24T11:42:00Z

src/Symfony/Component/Console/Exception/CommandNotDefinedException.php

+ *
+ * @author Jérôme Tamarelle <jerome@tamarelle.net>
+ */
+class CommandNotDefinedException extends \InvalidArgumentException implements ExceptionInterface


I would have named it CommandNotFoundException to be more consistent with other exception names in other components.

That's close to the RouteNotFoundException from the Routing component. Have you something in mind ?

CommandNotFoundException

fabpot · 2015-09-24T11:45:02Z

I made some minor comments, but I'm 👍 on this change.

ping @symfony/deciders

GromNaN · 2015-09-24T14:40:02Z

src/Symfony/Component/Console/Helper/SymfonyQuestionHelper.php

@@ -38,7 +39,7 @@ public function ask(InputInterface $input, OutputInterface $output, Question $qu

            // make required
            if (!is_array($value) && !is_bool($value) && 0 === strlen($value)) {
-                throw new \Exception('A value is required.');
+                throw new LogicException('A value is required.');


Here I've replaced the only instance of the basic \Exception by a more precise LogicException.

…sistency

dunglas · 2015-09-24T19:39:30Z

👍

fabpot · 2015-09-25T06:45:05Z

Thank you @GromNaN.

xabbuh · 2015-09-27T13:46:17Z

@GromNaN I had a look at the failing tests: #15940

…test (xabbuh) This PR was merged into the 2.8 branch. Discussion ---------- [Console] don't rely on internal sort implementation om test | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #14894 | License | MIT | Doc PR | PHP does not guarantuee how array elements with the same value will be sorted when applying `asort()`. Since all namespaces used in the test produce the same Levenshtein value, we should only check for presence of these namespaces instead of comparing the exact order. Commits ------- 3011fa0 don't rely on internal sort implementation in test

GromNaN reviewed Jun 6, 2015
View reviewed changes

GromNaN force-pushed the console-exception branch from 557e52d to bd025a3 Compare June 6, 2015 15:33

GromNaN changed the title ~~[Console] Add CommandNotDefinedException to replace generic exception~~ [Console] Add domain exceptions to replace generic exceptions Jun 6, 2015

GromNaN force-pushed the console-exception branch from bd025a3 to 0077045 Compare June 6, 2015 19:46

phansys reviewed Jun 7, 2015
View reviewed changes

GromNaN force-pushed the console-exception branch from 0077045 to c367dc8 Compare June 7, 2015 21:10

xabbuh added the Console label Jun 16, 2015

GromNaN force-pushed the console-exception branch from c367dc8 to b09375b Compare June 21, 2015 20:18

GromNaN mentioned this pull request Jun 21, 2015

[DX][Console] Use specific exception classes for not defined and ambiguous command names #14873

Closed

fabpot reviewed Aug 1, 2015
View reviewed changes

GromNaN added 3 commits September 22, 2015 23:27

[Console] Add CommandNotDefinedException to replace generic \InvalidA…

7b2c75f

…rgumentException

[Console] Add InvalidOptionException to replace generic \InvalidArgum…

64aae63

…entException

Use CommandNotDefinedException in ApplicationDescription

185e506

GromNaN force-pushed the console-exception branch from ae7d14d to 515f392 Compare September 23, 2015 19:26

[Console] Use domain specific exceptions

63f5af6

Replace exception thrown by specific exception implementing the same interface.

GromNaN force-pushed the console-exception branch from 515f392 to 63f5af6 Compare September 23, 2015 19:28

fabpot reviewed Sep 24, 2015
View reviewed changes

GromNaN reviewed Sep 24, 2015
View reviewed changes

Rename CommandNotDefinedException to CommandNotFoundException for con…

882ad75

…sistency

fabpot closed this in dacbfe9 Sep 25, 2015

xabbuh mentioned this pull request Sep 27, 2015

[Console] don't rely on internal sort implementation om test #15940

Merged

fabpot mentioned this pull request Nov 16, 2015

Release v2.8.0-BETA1 #16550

Merged

GromNaN deleted the console-exception branch November 16, 2015 15:14

Uh oh!

[Console] Add domain exceptions to replace generic exceptions #14894

[Console] Add domain exceptions to replace generic exceptions #14894

Uh oh!

Conversation

GromNaN commented Jun 6, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GromNaN commented Jun 7, 2015

Uh oh!

phansys commented Jun 7, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Aug 1, 2015

Uh oh!

fabpot commented Sep 14, 2015

Uh oh!

GromNaN commented Sep 22, 2015

Uh oh!

fabpot commented Sep 22, 2015

Uh oh!

GromNaN commented Sep 22, 2015

Uh oh!

GromNaN commented Sep 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Sep 24, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dunglas commented Sep 24, 2015

Uh oh!

fabpot commented Sep 25, 2015

Uh oh!

xabbuh commented Sep 27, 2015

Uh oh!

Uh oh!