Skip to content

Set CLI exit code to 1 when invalid parameters are passed #5093

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.

Sign up for GitHub

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

Jump to bottom
Closed
diafour wants to merge 5 commits into from
Closed

Set CLI exit code to 1 when invalid parameters are passed #5093

diafour wants to merge 5 commits into from

Conversation

@diafour
Copy link
Contributor

@diafour diafour commented Jan 17, 2020 •
edited
Loading

Hello! This is a continuation of #4466 with some enhancements:

  • php_opt_error returns -2 instead of '?' to distinguish between requested help and errors
  • set show_error argument to true, so cli and cgi print error to sdterr

@diafour
Copy link
Contributor Author

diafour commented Jan 18, 2020

This PR tries to fulfill some thoughts from this review: #4466 (review)

nikic
nikic reviewed Jan 21, 2020
View reviewed changes
Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

This looks good to me, I'd only like to see the new -2 value given a proper name. Like PHP_GETOPT_INVALID_ARG or so.

diafour
diafour commented Jan 23, 2020
View reviewed changes
@nikic
Copy link
Member

nikic commented Jan 23, 2020

I think this should be fine for 7.3. It changes the exit code, but only in an error case, where execution of the program is aborted. As such, I don't see how this could cause a practical backwards compatibility issue. Does anyone have concerns about this?

chamajcpradel
chamajcpradel approved these changes Jan 24, 2020
View reviewed changes
Copy link

@chamajcpradel chamajcpradel left a comment

Choose a reason for hiding this comment

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

Sirs. I will have to adjust my local scripts. LGTM.

@nikic
Copy link
Member

nikic commented Jan 27, 2020

Merged as fd08f06 into 7.3. Thanks!

@nikic nikic closed this Jan 27, 2020
@pierredup pierredup mentioned this pull request Jun 20, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nikic nikic nikic approved these changes

+1 more reviewer

@chamajcpradel chamajcpradel chamajcpradel approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@diafour @nikic @chamajcpradel