Skip to content
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

[Console] fixed PHP7 Errors when not using Dispatcher #20736

Closed
wants to merge 3 commits into from

Conversation

keradus
Copy link
Member

@keradus keradus commented Dec 3, 2016

Q A
Branch? 2.7
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #17257, #20110, #20111
License MIT
Doc PR n/a

Original fix, #19813, works only when there is event dispatcher available.
This PR fix the issue also for scenario without event dispatcher.

Closes #20110 issue and #20111 PR connected to it.
Closing #17257 , as everywhere the error is converted to exception and it should be handled

@@ -159,6 +159,8 @@ public function run(InputInterface $input = null, OutputInterface $output = null
* @param OutputInterface $output An Output instance
*
* @return int 0 if everything went fine, or an error code
*
* @throws \Exception propagate the exception from `doRunCommand`
Copy link
Member Author

Choose a reason for hiding this comment

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

this is current behaviour, just documenting it as doRunCommand is protected, so it's not part of public interface, which should be documented anyway...

Copy link
Member

Choose a reason for hiding this comment

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

This can be removed as it does not bring any useful information (\Exception can be anything, we only document exceptions when we give an explanation about when a specific exception can occur).

} catch (\Exception $e) {
throw $e;
}
catch (\Throwable $e) {
Copy link
Member

Choose a reason for hiding this comment

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

extra newline

@SpacePossum
Copy link
Contributor

👍

@@ -159,6 +159,8 @@ public function run(InputInterface $input = null, OutputInterface $output = null
* @param OutputInterface $output An Output instance
*
* @return int 0 if everything went fine, or an error code
*
* @throws \Exception when the command being run threw an exception
Copy link
Member

Choose a reason for hiding this comment

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

I still think this should be removed as this is not actionnable.

Copy link
Member Author

Choose a reason for hiding this comment

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

very well.
btw, please check an email

@fabpot
Copy link
Member

fabpot commented Dec 5, 2016

Thank you @keradus.

fabpot added a commit that referenced this pull request Dec 5, 2016
…adus)

This PR was squashed before being merged into the 2.7 branch (closes #20736).

Discussion
----------

[Console] fixed PHP7 Errors when not using Dispatcher

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #17257, #20110, #20111
| License       | MIT
| Doc PR        | n/a

Original fix, #19813, works only when there is event dispatcher available.
This PR fix the issue also for scenario without event dispatcher.

Closes #20110 issue and #20111 PR connected to it.
Closing #17257 , as everywhere the error is converted to exception and it should be handled

Commits
-------

899fa79 [Console] fixed PHP7 Errors when not using Dispatcher
@fabpot fabpot closed this Dec 5, 2016
@keradus keradus deleted the 2.7_console branch December 5, 2016 13:29
@ogizanagi
Copy link
Member

ogizanagi commented Dec 14, 2016

Unless I'm misunderstanding something, I think this should be reverted accordingly to #19813 (comment).

The behavior is inconsistent depending on whether an event dispatcher is injected or not. When it's injected, the original \Throwable is thrown, for BC reasons. Here, the original throwable is transformed to an exception and caught.

See #20917 (comment) for more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants