NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

GromNaN wants to merge5 commits intosymfony:2.8fromGromNaN:console-exception

Closed

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

GromNaN wants to merge5 commits intosymfony:2.8fromGromNaN:console-exception

Conversation

Copy link

Member

GromNaN commentedJun 6, 2015

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 bySymfony\Component\Console\Exception\InvalidArgumentException
AddSymfony\Component\Console\Exception\ExceptionInterface

GromNaN reviewed

Jun 6, 2015

View reviewed changes

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

Copy link

MemberAuthor

GromNaNJun 6, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

Member

fabpotSep 24, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I thinkd should be on the same line asdefine.

GromNaN force-pushed theconsole-exception branch from557e52d tobd025a3Compare

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 theconsole-exception branch frombd025a3 to0077045Compare

June 6, 2015 19:46

Copy link

MemberAuthor

GromNaN commentedJun 7, 2015

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

Copy link

Contributor

phansys commentedJun 7, 2015

AFAIK, the alternative names was originallyordered withlevenshtein(), but I don't know if your issue is related to this.

phansys reviewed

Jun 7, 2015

View reviewed changes

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

Copy link

Contributor

phansysJun 7, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Please, check missing new lines at EOFs.

Copy link

MemberAuthor

GromNaNJun 7, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Fixed.

GromNaN force-pushed theconsole-exception branch from0077045 toc367dc8Compare

June 7, 2015 21:10

xabbuh added the Console label

Jun 16, 2015

GromNaN force-pushed theconsole-exception branch fromc367dc8 tob09375bCompare

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

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

Copy link

Member

fabpotAug 1, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Missing leading \ on all@expectedException tags

Copy link

Member

fabpotSep 24, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

fabpot commentedAug 1, 2015

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

Copy link

Member

fabpot commentedSep 14, 2015

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

Copy link

MemberAuthor

GromNaN commentedSep 22, 2015

OK for moving remaining exception to specific ones. Is it necessary to keep the distinction betweenInvalidArgumentException andLogicException in classes likeConsole/Question/Question ?

Copy link

Member

fabpot commentedSep 22, 2015

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

Copy link

MemberAuthor

GromNaN commentedSep 22, 2015

I'll createSymfony\Component\Console\Exception\InvalidArgumentException andSymfony\Component\Console\Exception\LogicException

And the interfaceSymfony\Component\Console\Exception\ExceptionInterface

GromNaN added3 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 theconsole-exception branch fromae7d14d to515f392Compare

September 23, 2015 19:26

[Console] Use domain specific exceptions

63f5af6

Replace exception thrown by specific exception implementing thesame interface.

GromNaN force-pushed theconsole-exception branch from515f392 to63f5af6Compare

September 23, 2015 19:28

Copy link

MemberAuthor

GromNaN commentedSep 23, 2015

Rebased and made all changes.

I still have this error on HHVM :

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

fabpot reviewed

Sep 24, 2015

View reviewed changes

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

Copy link

Member

fabpotSep 24, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

MemberAuthor

GromNaNSep 24, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

fabpotSep 24, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

CommandNotFoundException

Copy link

Member

fabpot commentedSep 24, 2015

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

ping @symfony/deciders

GromNaN reviewed

Sep 24, 2015

View reviewed changes

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

Copy link

MemberAuthor

GromNaNSep 24, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Rename CommandNotDefinedException to CommandNotFoundException for con…

882ad75

…sistency

Copy link

Member

dunglas commentedSep 24, 2015

👍

Copy link

Member

fabpot commentedSep 25, 2015

Thank you@GromNaN.

fabpot closed this indacbfe9

Sep 25, 2015

xabbuh mentioned this pull request

Sep 27, 2015

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

Merged

Copy link

Member

xabbuh commentedSep 27, 2015

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

fabpot added a commit that referenced this pull request

Sep 27, 2015

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

4b94274

…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 besorted when applying `asort()`. Since all namespaces used in the testproduce the same Levenshtein value, we should only check for presence ofthese namespaces instead of comparing the exact order.Commits-------3011fa0 don't rely on internal sort implementation in test

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

Labels

Console

Movatterモバイル変換

Uh oh!

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

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

Uh oh!

Conversation

GromNaN commentedJun 6, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GromNaN commentedJun 7, 2015

Uh oh!

phansys commentedJun 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 commentedAug 1, 2015

Uh oh!

fabpot commentedSep 14, 2015

Uh oh!

GromNaN commentedSep 22, 2015

Uh oh!

fabpot commentedSep 22, 2015

Uh oh!

GromNaN commentedSep 22, 2015

Uh oh!

GromNaN commentedSep 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 commentedSep 24, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dunglas commentedSep 24, 2015

Uh oh!

fabpot commentedSep 25, 2015

Uh oh!

xabbuh commentedSep 27, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants