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 error code on CLI Command failure #2164

Merged
merged 3 commits into from
Aug 28, 2019

Conversation

MGatner
Copy link
Member

@MGatner MGatner commented Aug 27, 2019

Description
Currently there is no way for a CLI command to indicate exit status to the shell. There are definitely better ways to do this, but this is an example of allowing commands to return false or a numeric code to indicate to spark that the CLI Command failed, and have spark exit appropriately for the shell.

Ref. #2163

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@jim-parry
Copy link
Contributor

It looks like this is patterned after HTTP status codes (>= 300 being bad).
Another though would be to pattern error codes after bash script codes (https://shapeshed.com/unix-exit-codes/), which could be more conventional for command line usage.

@MGatner
Copy link
Member Author

MGatner commented Aug 27, 2019

@jim-parry I definitely agree but I think in order to do that we'll want a separate CLI Response class (spark uses HTTPResponse right now) and I think that will be a pretty large change with lots of repercussions. HTTP\Response->setStatusCode() doesn't just set a straight numerical so it isn't as easy as passing in a Unix code.

@jim-parry
Copy link
Contributor

Makes sense ... twas just a thought :)

@MGatner
Copy link
Member Author

MGatner commented Aug 27, 2019

A good thought! If the admins wink wink are okay with implementing a big change like this pre-RC1 I am glad to take a look at it, but I thought this was more likely to get something in since right now there's absolutely no way to do this.

@jim-parry
Copy link
Contributor

I think that sticking to the HTTP codes is more sensible at the moment.
Down the road, it could be an idea to build a different CLI runner that was meant to play nicer with shell scripts.

@lonnieezell lonnieezell merged commit 6af9416 into codeigniter4:develop Aug 28, 2019
@jim-parry jim-parry mentioned this pull request Sep 1, 2019
@MGatner MGatner deleted the cli-returns branch September 2, 2019 02:27
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