Skip to content

At error_cb, set status header before send header #3244

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

Closed
wants to merge 1 commit into from

Conversation

robberphex
Copy link
Contributor

@nikic
Copy link
Member

nikic commented May 19, 2018

Looks reasonable to me, there are some tests that need to be adjusted though.

The bug report also mentions IE displaying a custom error page on ISE. Is this something it still does or have newer versions fixed that?

@robberphex robberphex force-pushed the issue50921 branch 5 times, most recently from 92ad6c7 to 5651148 Compare May 21, 2018 05:11
@robberphex
Copy link
Contributor Author

All test passed. (and sapi/cli/tests/php_cli_server_015.phpt is modified)


For IE user/developer, they can go to Internet Options -> Advanced, and uncheck show friendly HTTP error messages, to see page with 500 code.

@nikic
Copy link
Member

nikic commented May 25, 2018

@weltling What do you think about this change?

@weltling
Copy link
Contributor

weltling commented Jun 2, 2018

@nikic I finally came to check it with various browsers. IE 11, the latest one, shows the same behavior with pretty error messages, which was kinda expected. Otherwise, besides FF and Chromium, even Edge shows the error message as is with 500 status.

The change seems to make sense as it aligns the returned HTTP status for more low level errors like undefined functions and syntax errors. Though, i'd be reluctant on rushing with it after reading the comments from Derick and Ferenc in the ticket. On the one hand, ofc it's logic and there are some more advantages - fe. misconfigured servers returting 500 are unlikely to be indexed by search engines. On the other hand - there can be non obvious side effects with existing apps and server configurations, having less to do with the actual browser issue but impacting the general operation.

With IE itself - for sure it's going to be supported for a while and some dedicated apps out there do exist. Still, the issue almost only development relevant. Neither a properly configured server should show these errors, nor the regular IE users would normally want to see them. The usage of the dev server fe seems to be fine as is, as it shows all the errors on stderr.

My suggestion would be to discuss on internals and go through RFC targeting PHP 7.4. For 7.3 it seems a bit too short cut, as for me, otherwise one should talk to the 7.3 RMs. At least Xdebug will need to catch up, other modules might be affected, too. The change itself is not big, but has a potential to affect beyond areas. OFC it'd need UPGRADING notes and possibly some documentation patch. Also perhaps more tests with display_errors=on/off and some fatal/syntax/etc. errors would not harm.

Thanks.

@krakjoe krakjoe added the RFC label Jun 14, 2018
@robberphex
Copy link
Contributor Author

Is there anything I can do? (subscribe mail list? write an RFC?)

@cmb69
Copy link
Member

cmb69 commented Apr 26, 2019

@robberphex See https://wiki.php.net/rfc/howto. And sorry for the late reply, but there's still time to walk through the RFC process for PHP 7.4.

@cmb69
Copy link
Member

cmb69 commented Dec 28, 2021

I'm closing this PR due to inactivity. @robberphex, feel free to fix the merge conflicts, and re-open.

Thanks for your work, anyway! :)

@cmb69 cmb69 closed this Dec 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants