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

When print failed, exit with 255 #3306

Closed
wants to merge 1 commit into from

Conversation

robberphex
Copy link
Contributor

@@ -317,10 +313,9 @@ static size_t sapi_cli_ub_write(const char *str, size_t str_length) /* {{{ */
while (remaining > 0)
{
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 ...

{
#ifdef PHP_WRITE_STDOUT
zend_long ret;
#else
size_t ret;
ssize_t ret;
#endif

Copy link
Contributor

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
Contributor

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
Contributor

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.

None yet

6 participants