-
Notifications
You must be signed in to change notification settings - Fork 8k
http_response_code should warn if headers already sent
#10744
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
Conversation
This would fail silently otherwise. The warning should be similar to the one that header emits (the code is some copy and paste from main/SAPI.c, to match). It'll also return false in that case. Fixes phpGH-10742
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, other than that the failing test needs to be fixed.
diff --git a/sapi/fpm/tests/log-suppress-output.phpt b/sapi/fpm/tests/log-suppress-output.phpt index 5a5e7bb954..a507180e99 100644 --- a/sapi/fpm/tests/log-suppress-output.phpt +++ b/sapi/fpm/tests/log-suppress-output.phpt @@ -38,7 +38,7 @@ function doTestCalls(FPM\Tester &$tester, bool $expectSuppressableEntries) $tester->request(query: 'test=output', uri: '/ping')->expectBody('pong', 'text/plain'); $tester->expectAccessLog("'GET /ping?test=output' 200", suppressable: false); - $tester->request(headers: ['X_ERROR' => 1])->expectBody('Not OK'); + $tester->request(headers: ['X_ERROR' => 1])->expectStatus('500 Internal Server Error')->expectBody('Not OK'); $tester->expectAccessLog("'GET /log-suppress-output.src.php' 500", suppressable: false); $tester->request()->expectBody('OK'); @@ -54,8 +54,8 @@ function doTestCalls(FPM\Tester &$tester, bool $expectSuppressableEntries) $src = <<<EOT <?php if (isset(\$_SERVER['X_ERROR'])) { - echo "Not OK"; http_response_code(500); + echo "Not OK"; exit; } echo \$_REQUEST['test'] ?? "OK";
FWIW while it doesn't affect this PR, there are a lot of callers of php_output_get_start_lineno that mix up signedness. I fixed it for this instance, but it should probably be for the others as well.
FWIW while it doesn't affect this PR, there are a lot of callers of php_output_get_start_lineno that mix up signedness. I fixed it for this instance, but it should probably be for the others as well.
Well, output_start_lineno is declared as int so signed is probably correct. I'm not sure if -1 is a possible value. If not, we could convert it to uint32_t but that should be done separately.
Obviously not in this PR, but yeah. If it is actually signed, that's a bit confusing because of the function's signature, which is what I was basing it off of.
Which function are you referring to? php_output_get_start_lineno also returns int.
Augh, I think I mixed it up with zend_get_executed_lineno. Which, they should be consistent if they're both about line numbers?
@NattyNarwhal Ah yes, zend_op.lineno and zend_ast.lineno are both uint32_t, so converting this should be unproblematic.
I changed it back to int because that's what the API is defined as for now; if that API does get changed to be uint32_t, it'll have to be a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM otherwise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one or two minor things otherwise it makes sense and looks reasonable.
Test failure seems spurious, can't reproduce on amd64/arm64 Linux/debug/NTS here?
Test failure seems spurious, can't reproduce on amd64/arm64 Linux/debug/NTS here?
I agree, it looks spurious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @NattyNarwhal!
Pinging to see if this is still mergeable.
@NattyNarwhal Oh, I thought you have merge access. I can merge it tomorrow. 🙂
Thank you @NattyNarwhal!
This would fail silently otherwise. The warning should be similar to the one that header emits (the code is some copy and paste from
main/SAPI.c, to match). It'll also return false in that case.Fixes GH-10742