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

exitcode 1 on exception in yii_test #433

Closed
wants to merge 1 commit into from

Conversation

rumours86
Copy link

Q A
Is bugfix? yes
New feature? no
Breaks BC? no
Tests pass? yes
Fixed issues yiisoft/yii2-app-basic#180

@samdark samdark requested a review from a team July 16, 2019 13:19
@samdark samdark added the status:to be verified Needs to be reproduced and validated. label Jul 16, 2019
@rob006
Copy link
Contributor

rob006 commented Jul 16, 2019

Does it change anything? It does not fix #180, since there is no uncaught exception in this case. And in case of uncaught exception PHP should already return non-zero exit code. So what is the point of this change?

@rumours86
Copy link
Author

rumours86 commented Jul 16, 2019

If an error occurs during the migration, php does not return a non-zero exit code.

In our project we use this code, and it corrects this situation. If an error occurs while applying migrations, the CI build stops.

@rumours86
Copy link
Author

you confused the issue
yiisoft/yii2-app-basic#180

@rumours86
Copy link
Author

but I use the author of the issue ./yii
but we use ./yii_test and because I fixed only this file and it was only in this repository

if you think you need to make this correction and in ./yii I can will do it

$exitCode = $application->run();
try {
$exitCode = $application->run();
} catch (\Exception $exception) {
Copy link
Member

@samdark samdark Jul 16, 2019

Choose a reason for hiding this comment

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

That would cause error handler to never fire preventing logging exceptions and errors. Also that would mess up formatting.

I think what you want to change is https://github.com/yiisoft/yii2/blob/master/framework/base/ErrorHandler.php#L112 but that can't be done since it would affect testing (tests would exit after first failure without suite completion).

@samdark
Copy link
Member

samdark commented Jul 16, 2019

In this state pull request can't be merged. Another solution should be found.

@samdark samdark closed this Jul 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:to be verified Needs to be reproduced and validated.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants