file_get_contents should probably clear the global state of last headers

[Seldaek]

Description

The following code:

<?php

file_get_contents(__FILE__);
var_dump(http_get_last_response_headers());

file_get_contents('https://php.net/');
var_dump(http_get_last_response_headers());

file_get_contents(__FILE__);
var_dump(http_get_last_response_headers());

Resulted in this output:

NULL
array( headers from php.net )
array( headers from php.net )

But I expected this output instead:

NULL
array( headers from php.net )
NULL // as reading a file clearly outputs no headers

IMO this is very surprising, and can lead to bugs (see jsonrainbow/json-schema#843 for example) as people are now nudged to switch to http_get_last_response_headers() by the deprecations.

The problem is that $http_response_headers, while a very ugly API, was a local variable.. and http_get_last_response_headers() while appearing cleaner stores global state and can thus leak data across boundaries if you do not call http_clear_last_response_headers() after fetching the headers.

Thus my suggestion would be that file_get_contents first clears the headers and then populates them if the stream wrapper in use has anything to populate.

/cc @Girgias as I know you were involved in the deprecation. And apologies if this was already discussed somewhere I didn't find.

PHP Version

All versions where `http_get_last_response_headers()` is available

Operating System

No response

[Girgias]

No, I think that makes sense and wasn't discussed before. It should be relatively easy to reset the variable in file_get_contents(), but is this something that also needs to be dealt with in other file functions?

[Seldaek]

I guess anywhere that populates this should populate it always, either with null or with headers.. I am not sure what the complete list is tho.

[krakjoe]

I guess anywhere that populates this should populate it always, either with null or with headers.. I am not sure what the complete list is tho.

This is already true.

The misalignment between behavior and expectation is because the file wrapper doesn't populate this array, it is http specific. I'm not certain the file wrapper should destroy this array, technically the last http response headers are returned correctly in your example code.

The expected behavior might seem a bit magical, and might also exhibit unwanted side effects; It would essentially mean you cannot delay the call to fetch headers if subsequent non-http operations destroy them, making what would seem like acceptable code behave surprisingly:

$result = file_get_contents("http://my.http.resource");

if ($result) {
   $confile = file_get_contents("/my/data/configuration.json");

   /* do things on disk */

  /* uh-oh */
  $headers = http_get_last_response_headers();
}

[Seldaek]

Yeah, there is the potential for surprises both ways I can see that. But currently the deprecation message nudges you to simply replace the variable with a global-state function, which seems innocuous but actually carries a lot of WTF potential.

I think one option is to make it always clear headers, the other is to maybe update the deprecation message to suggest clearing headers before the call, and then using the function to retrieve them. That'd at least raise awareness but IMO given this is only available since PHP 8.4 changing the behavior now to the safer one is the better option (I'd argue you'll notice missing headers faster when testing than potentially incorrect headers globally-injected by some other code anywhere else in the application).

[krakjoe]

I think one option is to make it always clear headers

Well, we could only change internally defined stream wrappers to conform to this new behavior. Any third party cannot be forced to behave this way (that would require that we wrap wrappers).

If we were to specify that externally registered stream wrappers must clear http wrapper globals (they're actually basic globals, but this is an implementation detail) before opening any stream, that would be a pretty strange looking specification.

This starts to look like a documentation (starting with warning message) problem to me.

EDIT: maybe I was too hasty to suggest it would be impractical, we could maybe clear them before invoking any wrappers open method, which means we don't have to make this the responsibility of the wrapper ... but the behavior still looks questionable to me, however it's implemented; Whether we implicitly clear the headers or force wrappers to do it explicitly, we have effectively changed the specification of stream wrappers - any open wrapper method would destroy http wrapper globals.

[krakjoe]

Probably the best route forward is starting a conversation on internals to try to gather a consensus about what to do.

[github-actions]

No feedback was provided. The issue is being suspended because we assume that you are no longer experiencing the problem. If this is not the case and you are able to provide the information that was requested earlier, please do so. Thank you.