http_response_code should warn if headers already sent

[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

[iluuu1994]

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";

[NattyNarwhal]

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.

[iluuu1994]

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.

[NattyNarwhal]

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.

[iluuu1994]

Which function are you referring to? php_output_get_start_lineno also returns int.

[NattyNarwhal]

Augh, I think I mixed it up with zend_get_executed_lineno. Which, they should be consistent if they're both about line numbers?

[iluuu1994]

@NattyNarwhal Ah yes, zend_op.lineno and zend_ast.lineno are both uint32_t, so converting this should be unproblematic.

[NattyNarwhal]

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.

[iluuu1994]

LGTM otherwise

[bukka]

Just one or two minor things otherwise it makes sense and looks reasonable.

[NattyNarwhal]

Test failure seems spurious, can't reproduce on amd64/arm64 Linux/debug/NTS here?

[nielsdos]

Test failure seems spurious, can't reproduce on amd64/arm64 Linux/debug/NTS here?

I agree, it looks spurious.

[iluuu1994]

Thanks @NattyNarwhal!

[NattyNarwhal]

Pinging to see if this is still mergeable.

[iluuu1994]

@NattyNarwhal Oh, I thought you have merge access. I can merge it tomorrow. 🙂

[iluuu1994]

Thank you @NattyNarwhal!