Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Console] Fix dispatching throwables from ConsoleEvents::COMMAND#22435

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

Merged
fabpot merged 1 commit intosymfony:2.7fromnicolas-grekas:console-exception
Apr 25, 2017

Conversation

@nicolas-grekas
Copy link
Member

@nicolas-grekasnicolas-grekas commentedApr 14, 2017•
edited
Loading

QA
Branch?2.7
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets-
LicenseMIT
Doc PR-

Exceptions thrown byConsoleEvents::COMMAND listeners should be triggerConsoleEvents::EXCEPTION events.
(best reviewedignoring whitespaces)

@wouterj
Copy link
Member

wouterj commentedApr 14, 2017•
edited
Loading

I don't think it should. Symfony 3.3 introduces a newConsoleEvents::ERROR, deprecating the oldConsoleEvents::EXCEPTION (#18140). The new event has some advantages and supports Throwables.

@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedApr 14, 2017•
edited
Loading

I think it should. That's the expected lifecycle of dispatched events to me, thus just a bug fix.
About throwable, they are already handled since 2.7.0, via this cast to FatalThrowableError.

Copy link
Member

@chalasrchalasr left a comment

Choose a reason for hiding this comment

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

Far better 👍
For the record, we try to handle errors properly from 2.7 since#19813, this should not impact the ability of the new error event to handle errors AFAIK.

@fabpot
Copy link
Member

Thank you@nicolas-grekas.

@fabpotfabpot merged commit6d6b04a intosymfony:2.7Apr 25, 2017
fabpot added a commit that referenced this pull requestApr 25, 2017
…OMMAND (nicolas-grekas)This PR was merged into the 2.7 branch.Discussion----------[Console] Fix dispatching throwables from ConsoleEvents::COMMAND| Q             | A| ------------- | ---| Branch?       | 2.7| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | -Exceptions thrown by `ConsoleEvents::COMMAND` listeners should be trigger `ConsoleEvents::EXCEPTION` events.(best reviewed [ignoring whitespaces](https://github.com/symfony/symfony/pull/22435/files?w=1))Commits-------6d6b04a [Console] Fix dispatching throwables from ConsoleEvents::COMMAND
@nicolas-grekasnicolas-grekas deleted the console-exception branchApril 25, 2017 14:04
fabpot added a commit that referenced this pull requestApr 26, 2017
…as-grekas)This PR was merged into the 3.3-dev branch.Discussion----------[Console] Review console.ERROR related behavior| Q             | A| ------------- | ---| Branch?       | 3.3| Bug fix?      | yes| New feature?  | yes| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | -This PR is a follow up of#18140 that I wanted to do since a few weeks.It enhances this work with fixes and behavior changes.It embeds#22435 and resolves issues like the one described in#20808.- makes ConsoleErrorEvent *not* extend the deprecated ConsoleExceptionEvent- replace ConsoleErrorEvent::markErrorAsHandled by ConsoleErrorEvent::setExitCode- triggers the deprecation in a more appropriate moment- renames ExceptionListener to ErrorListener- tweaks the behavior in relation to#22435Commits-------a7c67c9 [Console] Review console.ERROR related behavior
This was referencedMay 1, 2017
}catch (\Exception$e) {
}catch (\Exception$x) {
$e =$x;
}catch (\Throwable$x) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Casting from Error (it can't be anything else) to Exception here is a behaviour change - applications that use console will find their fatal error handlers are never triggered.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, then something else in this commit is causing the behaviour change in PhpSpec reported here#22678

Copy link
Contributor

@ogizanagiogizanagiMay 10, 2017•
edited
Loading

Choose a reason for hiding this comment

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

The original error was re-thrown before (the\Throwable, notFatalThrowableError):https://github.com/symfony/symfony/pull/22435/files/6d6b04ae977d322213a4ac1bd1c84bfd32313611#diff-38eebdda7d6ebfcb7405b91167f05c9cL874

Actually, being more accurate, the original error was re-thrownif you used an event dispatcher (and the event listeners don't set another exception), as I mentioned inthis issue andthis pr

This exception and error handling is hell and inconsistent, making it so hard to keep BC by trying to handle exception and errors the same way. I still believe two different flags for exceptions & errors would have saved us from some headaches. 😅

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@chalasrchalasrchalasr approved these changes

+2 more reviewers

@ciaranmcnultyciaranmcnultyciaranmcnulty left review comments

@ogizanagiogizanagiogizanagi left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

2.7

Development

Successfully merging this pull request may close these issues.

7 participants

@nicolas-grekas@wouterj@fabpot@ciaranmcnulty@ogizanagi@chalasr@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp