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

Fixes for #77841 #4131

Closed
wants to merge 1 commit into from

Conversation

6 participants
@peter279k
Copy link
Contributor

commented May 8, 2019

Changed log

  • Resolves #77841
  • I use the sapi_module.name to detect whether the environment is CLI.

If it's CLI environment, it will not set any response code for SG(sapi_headers).http_response_code.

  • I think it's a quick way to fix this, feel free to add the comment on this.
@KalleZ

This comment has been minimized.

Copy link
Member

commented May 8, 2019

This should also account for phpdbg which won't have a status code. I think its better to just have an early return for these SAPIs

@cmb69

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

I wonder whether it would be better to leave http_response_code() alone for BC reasons, and to recommend to the bug reporter to use php_sapi_name() in the first place.

@peter279k

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

I wonder whether it would be better to leave http_response_code() alone for BC reasons, and to recommend to the bug reporter to use php_sapi_name() in the first place.

@cmb69. Thank you for your reply.

Could you explain leave http_response_code alone description?

What's BC reasons?

If we use the php_sapi_name function in the first place of code that bug reporter provides, I think this will solve this issue.

@KalleZ

This comment has been minimized.

Copy link
Member

commented May 8, 2019

The BC that @cmb69 mentions is that you are effectively changing the return value from an integer to a boolean

@cmb69

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

The BC that @cmb69 mentions is that you are effectively changing the return value from an integer to a boolean

Actually, I hadn't even noticed that. :)

What I've meant is that it might be best to stick with the current behavior, i.e. what has been reported in https://bugs.php.net/77841 as actual result. Some existing code may rely on this (for whatever reason).

So instead of changing the behavior, we may consider to fix the documentation, and to recommend to do something $is_cli = in_array(php_sapi_name(), ['cgi', 'phpdbg']) instead of $is_cli = http_response_code();

@nikic

This comment has been minimized.

Copy link
Member

commented May 13, 2019

I tend to agree with @cmb69. Using http_response_code() to check whether you're running on CLI is super fishy and not something we would want to endorse or support. I expect changing this will also break someones tests (running on CLI) for HTTP response code functionality...

@petk petk added the Bugfix label May 16, 2019

@krakjoe

This comment has been minimized.

Copy link
Member

commented May 24, 2019

This would not appear to be the correct solution to the problem, so closing this.

@krakjoe krakjoe closed this May 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.