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

Exit with code 1 on failure #9

Merged
merged 2 commits into from
Apr 21, 2021
Merged

Conversation

mikaellanger
Copy link

Getting an error code on failure is pretty important when running migrations from a script.
For example, I would like to apply migrations in an init script in the official postgresql docker image in our CI, and also want to automate it in our deploy scripts. In both of these use-cases detecting that the migration failed is necessary.

@byjg
Copy link
Owner

byjg commented Apr 19, 2021

Hey, that's a good catch. Thank you for PR.

Quick Question - Instead of add "return" command everywhere isn't better to add in the method handleError the exit(1)?

@mikaellanger
Copy link
Author

The Symphony Command class wants the exit status returned from execute so I thought I'd better stick to that. No idea if it really matters though. 😄

@byjg
Copy link
Owner

byjg commented Apr 20, 2021

All right, it is good to me. We need to handle the parent::execute though.

@mikaellanger
Copy link
Author

Not quite sure I understand what you mean with "handle the parent::execute"?

@byjg
Copy link
Owner

byjg commented Apr 21, 2021

Something like this:

$result = parent::execute($input, $output);
if ($result == 0) {
   // Do what have to do
}
return $result;

@mikaellanger
Copy link
Author

Ok, updated. WDYT?
Didn't have time to even run this yet though.

@byjg byjg changed the base branch from master to 4.1.1 April 21, 2021 21:45
@byjg byjg merged commit 8c748a7 into byjg:4.1.1 Apr 21, 2021
@byjg
Copy link
Owner

byjg commented Apr 21, 2021

I am merging to branch 4.1.1 while I am doing some tests.

Meanwhile, you can download the version 4.1.1.x-dev to use this code.

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

Successfully merging this pull request may close these issues.

2 participants