HTTP client: Add last effective URI of redirected requests to response

[Manuelacosta98]

Hello, i am trying to get the last URI in a redirect sequence, this uri is available in the request object but there is no way to getting out at the moment in a easy way.

To give context i will try to point the times that this problem has been mentioned:

• clue/reactphp-buzz explainig the problem https://github.com/clue/reactphp-buzz/issues/129

• guzzle fix of the problem https://github.com/guzzle/guzzle/issues/1166

I wonder what approach to take in this library, i dont think is clever to modify the Psr ResponseInterface, so i am open to suggestions, thanks great code :+1:

[Manuelacosta98]

Well i mannage to find a way to get the headers on redirection in the request object making changes in react/http/src/Io/Transaction.php

private function onResponseRedirect(ResponseInterface $response, RequestInterface $request, Deferred $deferred)
{
    // resolve location relative to last request URI
    $location = $this->messageFactory->uriRelative($request->getUri(), $response->getHeaderLine('Location'));

    // CHANGE Add redirect info headers
    $request = $request
        ->withHeader("Redirect-Count", $deferred->numRequests)
        ->withAddedHeader("Redirect-Urls", (string)$location);

    $request = $this->makeRedirectRequest($request, $location);
    $this->progress('redirect', array($request));

    if ($deferred->numRequests >= $this->maxRedirects) {
        throw new \RuntimeException('Maximum number of redirects (' . $this->maxRedirects . ') exceeded');
    }

    return $this->next($request, $deferred);
}`

The problem that i am facing now is that the headers are not pass in the response, maybe it has something to do with Expose-Headers but not quite sure if someone got any ideas would be great. I am facing the solution of the added redirect info headers first instead of the middleware or the "on_redirect" event like is discuss in the guzzel tread with the same problem with psr-7 .

Thanks love to turn this into a PR but not quite sure the approach is right

[clue]

@Manuelacosta98 Thanks for reporting, I agree that this feature makes sense :+1:

I wonder what a decent API to expose this could look like.

I think exposing this as part of a response header is probably easiest from an API perspective. We need to make sure this doesn't interfere with actual response headers tho. This requires a number of test cases to deal with duplicate headers, multiple redirects etc.

The code snippet you've posted only adds an outgoing request header, but it's pretty closer otherwise. The $next() calls returns a Promise<ResponseInterface>, so you can also attach incoming response headers here.

Does anybody feel like looking into this and filing a PR? :+1:

[olegbaturin]

Hi. This is my issue added some years ago and a fast durty solution is to add the latest request object to response: look here.

[boenrobot]

I think exposing this as part of a response header is probably easiest from an API perspective. We need to make sure this doesn't interfere with actual response headers tho. This requires a number of test cases to deal with duplicate headers, multiple redirects etc.

Chrome, in its dev tools, uses "headers" that have a name starting with ":". Since normally ":" is a separator between name and value, there's 0 possibility of a name clash.

That said, I think it will be better to provide the entire last request object rather than a few headers with information from the last request.

Maybe add a second interface with a new method (e.g. getRequest()), and make the default implementation implement it? Custom response implementations that need this feature can implement this other interface in addition to ResponseInterface.

On a somewhat related note, IMHO, it would be even better if the request featured a nullable getPreviousResponse(), and thus allow backtracking of the entire redirect chain.

[clue]

I agree that a custom interface would be best to also provide additional guarantees for type safety, documentation and IDE autocompletion. The problem with implementing a custom interface isn't really implementing the interface itself, but rather how to guarantee type safety. Take a look at following example, how would we ensure that we get a RedirectedResponseInterface (name subject to change) in one case or the other?

$browser->get($url)->then(function (ResponseInterface $response) {
    // …
});

$browser = $browser->withRememberRedirects(true);
$browser->get($url)->then(function (RedirectedResponseInterface $response) {
    // …
});

We could of course enforce always returning this interface, but I wonder what BC and performance implications this has. As an alternative, we may as well resort to duck typing and/or using explicit instanceof checks.

Realistically, and while I see a number of perfectly valid use cases, it's also not the most requested feature. This brings up the question whether this is the only custom interface we expose and how we would potentially expose more details for other use cases (by implementing, exposing and documenting multiple independent interfaces?).

Would love to hear more feedback about this feature :+1:

[boenrobot]

There are several ways to deal with this problem, and which one a user picks will depend on their project requirements.

You could type hint the actual class to get it 100% type safe, but that's not a good idea if you are making a reusable component that may be used with different implementations. And if you are not doing just that, maybe there is a bigger problem in the application architecture.

You could use intersection type at the promise callback declaration, but that requires PHP 8.1, and your callback would only work for versions of the implementation where both interfaces are implemented.

You could use instanceof checks on the otherwise standard response interface argument if you need to both have safety and conditionally work based on inplementation and PHP version... That's the approach I would hope most users take. Certainly one I would take. I don't think it's too much to ask for such an extra check, especially if the approach is documented in an example use for the interface's methods. With even more extra interfaces in place, a single if could as well be used to ensure the entire shape of extra features required.

[clue]

@boenrobot I think we're talking about something along the lines of this?

$browser = $browser->withRememberRedirects(true);
$browser->get($url)->then(function (ResponseInterface $response) {
    echo 'Got a response: ' . $response->getStatusCode();

    while ($response instanceof RedirectedResponseInterface) {
        $response = $response->getRedirect();
    }

    echo 'First status: '  . $response->getStatusCode();
});

I can definitely see some value in having this around. But then again, afaict this doesn't solve the original problem of exposing the effective URL, so this might be a bit off-topic at this point?

The linked ticket https://github.com/guzzle/guzzle/issues/1166 suggests a similar approach but ultimately implemented a redirect callback and flags to enable redirect tracking in HTTP response headers. I still wonder what would be the best solution here.

[olegbaturin]

I still wonder what would be the best solution here.

https://github.com/amphp/http-client/blob/master/src/Response.php