Skip to content

When print failed, exit with 255 - #3306

Closed
robberphex wants to merge 1 commit into
php:masterfrom
robberphex:print-failed
Closed

When print failed, exit with 255#3306
robberphex wants to merge 1 commit into
php:masterfrom
robberphex:print-failed

Conversation

@robberphex

@robberphex robberphex commented Jun 15, 2018

Copy link
Copy Markdown
Contributor

weltling
weltling reviewed Jun 15, 2018
View reviewed changes
Comment thread sapi/cli/php_cli.c Outdated

@weltling weltling Jun 15, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The removal of this macro seems wrong.

@robberphex robberphex Jun 15, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I look php_handle_aborted_connection through, it works with PHP_CLI_WIN32_NO_CONSOLE.

When at Win32, why not call php_handle_aborted_connection?

@weltling weltling Jun 15, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In addition to the usual php.exe there is a binary variant php-win.exe dedicated to specifically have no console I/O.

Thanks.

@krakjoe

krakjoe commented Jun 15, 2018

Copy link
Copy Markdown
Member

@cmb69 you'll want to look at this one ...

cmb69
cmb69 reviewed Jun 15, 2018
View reviewed changes
Comment thread sapi/cli/php_cli.c Outdated

@cmb69 cmb69 Jun 15, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It seems to me that we should declare ssize_t ret; unconditionally.

@cmb69

cmb69 commented Jun 15, 2018

Copy link
Copy Markdown
Member

In my opinion, we should:

  • not refer to bug 44217 since this has been closed as not-a-bug.
  • have a test case for the new behavior, if possible
  • prominently document the subtle internal API break of sapi_cli_single_write in UPGRADING.INTERNALS

Otherwise, looks good to me.

@robberphex
robberphex force-pushed the print-failed branch 2 times, most recently from a86f58d to 0abc7af Compare June 16, 2018 05:32
@robberphex

robberphex commented Jun 16, 2018

Copy link
Copy Markdown
Contributor Author

@cmb69

  • In my opintion, bug 44217 is a real bug.
  • phpt cannot test the exit code, and this pr just to modify exit code
  • added to UPGRADING.INTERNALS

@cmb69

cmb69 commented Jun 16, 2018

Copy link
Copy Markdown
Member

Thanks!

In my opintion, bug 44217 is a real bug.

Well, the ticket claims that “Output after stdout/stderr closed cause immediate exit”, what would not be fixed by this pull request. The fix here is merely that an appropriate exit code code is set (which is good in my opinion).

Anyway, if there are no objections, I'm going to apply this fix before 7.3.0alpha2 is tagged (presumably next Tuesday).

@smalyshev smalyshev assigned smalyshev and unassigned smalyshev Jun 17, 2018
@php-pulls

php-pulls commented Jun 19, 2018

Copy link
Copy Markdown

Comment on behalf of cmb at php.net:

Thanks! Applied via ecc1a7c.

@php-pulls php-pulls closed this Jun 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cmb69 cmb69 cmb69 left review comments
+1 more reviewer
@weltling weltling weltling left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Quickfix Waiting on RM

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@robberphex @krakjoe @cmb69 @php-pulls @weltling @smalyshev