Skip to content

Conversation

@robberphex
Copy link
Contributor

{
ret = sapi_cli_single_write(ptr, remaining);
if (!ret) {
#ifndef PHP_CLI_WIN32_NO_CONSOLE
Copy link
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.

Copy link
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?

Copy link
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
Copy link
Member

krakjoe commented Jun 15, 2018

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

size_t ret;
ssize_t ret;
#endif

Copy link
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
Copy link
Member

cmb69 commented Jun 15, 2018

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
Copy link
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
Copy link
Member

cmb69 commented Jun 16, 2018

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
Copy link

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants