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

[7.x] Fix exit code of down/up commands when already down/up #32199

Closed
wants to merge 1 commit into from
Closed

[7.x] Fix exit code of down/up commands when already down/up #32199

wants to merge 1 commit into from

Conversation

JaZo
Copy link
Contributor

@JaZo JaZo commented Apr 2, 2020

This will return exit code 0 instead of boolean true. The boolean will be cast to an int resulting in exit code 1, but if the application is already down/up exit code 0 would be desirable.

Change

$ php artisan down
Application is now in maintenance mode.
$ echo $?
0
$ php artisan down
Application is already down.
$ echo $?
- 1
+ 0

$ php artisan up
Application is now live.
$ echo $?
0
$ php artisan up
Application is already up.
$ echo $?
- 1
+ 0

Context

Our deploymenttool (DeployHQ) performs php artisan down before uploading the changes, but stops if this fails. If the deployment fails later in the process - composer install or a migration might fail - our tool stops and the application is still in maintenance mode. If we fix the error and restart the deployment php artisan down now returns an exit code other than 0 and our tool thinks it failed and stops the deployment. This results in a 'locked' state where we have to manually bring the application back up before we can start a new deployment.

The boolean will be cast to an int resulting in exit code 1, but if the application is already down/up exit code 0 would be desirable.
@GrahamCampbell
Copy link
Member

Please send to 8.x, since this is a breaking change.

@JaZo
Copy link
Contributor Author

JaZo commented Apr 2, 2020

@GrahamCampbell This is partially true, but the down command was changed in #30422 making it an unintentional breaking change back then in 6.x. Should I fix the down command in 6.x and fix the up command in 8.x then?

@GrahamCampbell
Copy link
Member

I think it's best to only make the changes on 8.x, and document this in the upgrading guide, so we don't break people's deployments.

@madman-81
Copy link
Contributor

I've run into this issue several times now. It would be much appreciated if this could also be merged into 7.x

@JaZo
Copy link
Contributor Author

JaZo commented Jun 25, 2020

I agree, based on Dries' reaction in #32746 (comment) and the addition to the upgrade guide this seems like a forgotten change for Symfony 5 to me.

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.

3 participants