[RFC] Add http_(get|clear)_last_request

A.L.E.C

2 years ago

On 3.01.2024 14:41, Gina P. Banyard wrote:

> Link: https://wiki.php.net/rfc/http-last-response-headers

Wrong function name in the subject (should be "response" not "request")

I don't think we need the clearing function. Do we?

I don't like that this is HTTP specific feature while we have other
protocol wrappers. Here's a different approach. Use stream context with
extended context parameters feature. Something like:

$context = stream_context_create();

$file = file_get_contents('http://www.example.com/', false, $context);

$headers = stream_context_get_params($context)['response_headers'];

Or something like that. I don't know.

While on this I found out that we already have stream_get_meta_data()
and `wrapper_data` there. So, maybe we should/could make it more unified.

Maybe it should be mentioned in the RFC.

--
Aleksander Machniak
Kolab Groupware Developer        [https://kolab.org]
Roundcube Webmail Developer  [https://roundcube.net]
----------------------------------------------------
PGP: 19359DC1 # Blog: https://kolabian.wordpress.com

Gina P. Banyard

2 years ago

On Wednesday, 3 January 2024 at 14:34, Aleksander Machniak <alec@alec.pl> wrote:

> On 3.01.2024 14:41, Gina P. Banyard wrote:
>
> > Link: https://wiki.php.net/rfc/http-last-response-headers
>
>
> Wrong function name in the subject (should be "response" not "request")

Ah yes indeed, the RFC title is also incorrect.

> I don't think we need the clearing function. Do we?

Yes we do, because this is effectively global state whereas the variable is created in the *local* scope.

> I don't like that this is HTTP specific feature while we have other
> protocol wrappers. Here's a different approach. Use stream context with
> extended context parameters feature. Something like:
>
> $context = stream_context_create();
>
> $file = file_get_contents('http://www.example.com/', false, $context);
>
> $headers = stream_context_get_params($context)['response_headers'];
>
> Or something like that. I don't know.
>
> While on this I found out that we already have stream_get_meta_data()
> and `wrapper_data` there. So, maybe we should/could make it more unified.
>
> Maybe it should be mentioned in the RFC.

The whole point of this RFC is to be able to remove the local variable, requiring to drop to the stream layer, is the reason this got removed from the 8.1 mass deprecation RFC.
That RFC also shows how this can be achieved currently, but it is way more cumbersome.

You cannot use any of the stream related functions if the request fails unless one ignores errors, which requires parsing every single response to know if it is valid or not instead of just checking for false.

Moreover, this means one cannot use file_get_contents().

As such, I am not going to change anything in the RFC as my objective is to get rid of the local variable creation in the long term.

Best regards,

Gina P. Banyard

Michał Brzuchalski

2 years ago

Hi Gina,

śr., 3 sty 2024 o 14:41 Gina P. Banyard <internals@gpb.moe> napisał(a):

> Hello internals,
>
> I would like to propose an RFC to add the functions
> http_get_last_request_headers() and http_clear_last_request_headers() to
> PHP to replace the magic variable $http_response_header.
>
> Link: https://wiki.php.net/rfc/http-last-response-headers

I was on the specific documentation page describing this feature Today and
was thinking that it is inappropriate as well.
But my thinking was whether it shouldn't be a part of a stream context
instead, something like this:

$response_headers;
$context = stream_context_create([
'http' => ['response_headers'  => &$response_headers]
]);
$result = file_get_contents('http://example.com/submit.php', false,
$context);

This way by ref you get a specific HTTP wrapper running under the hood
response headers instead of just the last one.
Any thoughts about that?

Cheers,
Michał Marcin Brzuchalski

Gina P. Banyard

2 years ago

On Wednesday, 3 January 2024 at 14:38, Michał Marcin Brzuchalski <michal.brzuchalski@gmail.com> wrote:

> Hi Gina,

> śr., 3 sty 2024 o 14:41 Gina P. Banyard <internals@gpb.moe> napisał(a):
>
>> Hello internals,
>>
>> I would like to propose an RFC to add the functions http_get_last_request_headers() and http_clear_last_request_headers() to PHP to replace the magic variable $http_response_header.
>>
>> Link: https://wiki.php.net/rfc/http-last-response-headers
>
> I was on the specific documentation page describing this feature Today and was thinking that it is inappropriate as well.
> But my thinking was whether it shouldn't be a part of a stream context instead, something like this:
>
> $response_headers;
> $context = stream_context_create([
> 'http' => ['response_headers' => &$response_headers]
> ]);
> $result = file_get_contents('http://example.com/submit.php', false, $context);
>
> This way by ref you get a specific HTTP wrapper running under the hood response headers instead of just the last one.
> Any thoughts about that?

I don't understand why the response should be part of the context of a stream.
Especially as I'm not aware of anything within the context that changes values after a request?

Best regards,
Gina P. Banyard

Michał Brzuchalski

2 years ago

śr., 3 sty 2024 o 15:57 Gina P. Banyard <internals@gpb.moe> napisał(a):

> On Wednesday, 3 January 2024 at 14:38, Michał Marcin Brzuchalski <
> michal.brzuchalski@gmail.com> wrote:
>
> Hi Gina,
>
> śr., 3 sty 2024 o 14:41 Gina P. Banyard <internals@gpb.moe> napisał(a):
>
> Hello internals,
>
> I would like to propose an RFC to add the functions
> http_get_last_request_headers() and http_clear_last_request_headers() to
> PHP to replace the magic variable $http_response_header.
>
> Link: https://wiki.php.net/rfc/http-last-response-headers
>
>
> I was on the specific documentation page describing this feature Today and
> was thinking that it is inappropriate as well.
> But my thinking was whether it shouldn't be a part of a stream context
> instead, something like this:
>
> $response_headers;
> $context = stream_context_create([
> 'http' => ['response_headers' => &$response_headers]
> ]);
> $result = file_get_contents('http://example.com/submit.php', false,
> $context);
>
> This way by ref you get a specific HTTP wrapper running under the hood
> response headers instead of just the last one.
> Any thoughts about that?
>
>
> I don't understand why the response should be part of the context of a
> stream.
> Especially as I'm not aware of anything within the context that changes
> values after a request?
>

My thinking was like passing as part of the context a placeholder by ref
where the headers of requests done in this context are to be populated.
This means instead of calling http_get_last_request_headers() as you
propose just read them from $response_headers array which got populated
during the HTTP call.
In general the same to what Aleksander asked.

Cheers

Gina P. Banyard

2 years ago

Hello internals,

I have updated the RFC to include a motivation and rejected ideas section:
https://wiki.php.net/rfc/http-last-response-headers

Unless there is further discussion, I intend to open the vote for the RFC on Wednesday the 24th of January.

Best regards,

Gina P. Banyard

>

Larry Garfield

2 years ago

On Mon, Jan 22, 2024, at 1:48 AM, Gina P. Banyard wrote:

> Hello internals,
>
> I have updated the RFC to include a motivation and rejected ideas section:
> https://wiki.php.net/rfc/http-last-response-headers
>
> Unless there is further discussion, I intend to open the vote for the
> RFC on Wednesday the 24th of January.
>
> Best regards,
>
> Gina P. Banyard

The change seems reasonable, but I am still nervous about removing anything without a deprecation.  We need to be better about that for user-space.  Per my previous message we *probably* are looking at one year from 8.4 to deprecation removal in 9.0; For a change this obscure I think that timeframe is acceptable, as its impact is much smaller.

--Larry Garfield

Nicolas Grekas

2 years ago

Hi Gina,

I would like to propose an RFC to add the functions

> http_get_last_request_headers() and http_clear_last_request_headers() to
> PHP to replace the magic variable $http_response_header.
>
> Link: https://wiki.php.net/rfc/http-last-response-headers
>

Sorry I missed this RFC.

In Symfony's HttpClient, we use stream_get_meta_data($h)['wrapper_data'] to
access headers, so there is already a way to get them using nothing but
local scope.

This makes me wonder why, in "rejected ideas", you wrote this?

> One suggested idea was to provide those headers via a by-reference entry
> to the stream context.

Maybe we don't need anything but to promote what works already (this
"wrapper_data" entry)?

Nicolas

Gina P. Banyard

2 years ago

On Tuesday, 30 January 2024 at 08:48, Nicolas Grekas <nicolas.grekas+php@gmail.com> wrote:

>
> In Symfony's HttpClient, we use stream_get_meta_data($h)['wrapper_data'] to
> access headers, so there is already a way to get them using nothing but
> local scope.
>
> This makes me wonder why, in "rejected ideas", you wrote this?
>
> > One suggested idea was to provide those headers via a by-reference entry
> > to the stream context.
>
>
>
> Maybe we don't need anything but to promote what works already (this
> "wrapper_data" entry)?

The issue is this requires you to have a stream in the first place.
file_get_contents() doesn't return a stream, and if the request fails fopen(), by default, returns false.

If you do have a stream, then yes stream_get_meta_data() is better, but the objective of this addition is to replace the $http_request_header variable so that it can be removed.
This is a rather fringe feature, and thus I expect these functions to also be a fringe feature.

Best regards,

Gina P. Banyard

[RFC] Add http_(get|clear)_last_request_headers() function

Gina P. Banyard

A.L.E.C

Gina P. Banyard

Michał Brzuchalski

Gina P. Banyard

Michał Brzuchalski

Gina P. Banyard

Larry Garfield

Nicolas Grekas

Gina P. Banyard