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

Can't create 200 OK response with Location header #1730

Closed
adambro opened this Issue Jan 11, 2016 · 16 comments

Comments

Projects
None yet
7 participants
@adambro
Contributor

adambro commented Jan 11, 2016

I'd like to respond 200 OK with Location HTTP header, but what I'm getting is 302 Found instead:

    return $response
            ->withHeader('Location', '/upload/' .basename($filePath))
            ->withStatus(200)
            ;

The same happens when I try to use 204 No Content response.

Surprisingly 201 Created works as expected. Can we make it work for 200 as well for clients that do not know how to handle 201, please?

@ppetermann

This comment has been minimized.

Contributor

ppetermann commented Jan 11, 2016

HTTP Location is valid with:
201 - to give the location of a newly created resource
202 - accepted, when its still being processed
3xx - for redirection purposes.

it shouldn't be send with 200.

@AndrewCarterUK

This comment has been minimized.

Contributor

AndrewCarterUK commented Jan 11, 2016

It might be PHP adding that status code once it detects the location header - I tried looking in the source but couldn't see anywhere obvious where the status code would be changed.

@adambro

This comment has been minimized.

Contributor

adambro commented Jan 11, 2016

@ppetermann I know, that's why I've mentioned the case for client that does not recognize 201 response properly.

@AndrewCarterUK I also haven't found it in Slim code.

@ppetermann

This comment has been minimized.

Contributor

ppetermann commented Jan 11, 2016

@adambro: use 3xx status code then.

@adambro

This comment has been minimized.

Contributor

adambro commented Jan 11, 2016

@ppetermann thanks for hint, but 3xx is not really an option - it's interpreted as error by the client. Unfortunately I can't fix that. I'd rather get rid of Location header instead.

@AndrewCarterUK

This comment has been minimized.

Contributor

AndrewCarterUK commented Jan 11, 2016

@adambro can you dump the response object before it gets sent?

You should be able to run Slim in silent mode to prevent it actually sending the response:

$response = $app->run(true);
var_dump($response);
@ppetermann

This comment has been minimized.

Contributor

ppetermann commented Jan 11, 2016

what sort of client is that? the 300's are pretty common status codes

@adambro

This comment has been minimized.

Contributor

adambro commented Jan 11, 2016

@ppetermann it's an embedded software system.

@AndrewCarterUK here's the dump of response:

object(Slim\Http\Response)#51 (5) {
  ["status":protected]=>
  int(200)
  ["reasonPhrase":protected]=>
  string(2) "OK"
  ["protocolVersion":protected]=>
  string(3) "1.1"
  ["headers":protected]=>
  object(Slim\Http\Headers)#50 (1) {
    ["data":protected]=>
    array(3) {
      ["content-type"]=>
      array(2) {
        ["value"]=>
        array(1) {
          [0]=>
          string(24) "text/html; charset=UTF-8"
        }
        ["originalKey"]=>
        string(12) "Content-Type"
      }
      ["location"]=>
      array(2) {
        ["value"]=>
        array(1) {
          [0]=>
          string(13) "/upload/test2"
        }
        ["originalKey"]=>
        string(8) "Location"
      }
      ["content-length"]=>
      array(2) {
        ["value"]=>
        array(1) {
          [0]=>
          string(1) "0"
        }
        ["originalKey"]=>
        string(14) "Content-Length"
      }
    }
  }
  ["body":protected]=>
  object(Slim\Http\Body)#48 (6) {
    ["stream":protected]=>
    resource(61) of type (stream)
    ["meta":protected]=>
    NULL
    ["readable":protected]=>
    NULL
    ["writable":protected]=>
    NULL
    ["seekable":protected]=>
    NULL
    ["size":protected]=>
    int(0)
  }
}
@AndrewCarterUK

This comment has been minimized.

Contributor

AndrewCarterUK commented Jan 11, 2016

I don't think Slim is your problem here.

Maybe try running a file like this:

// test.php
header('HTTP/1.1 200 Ok');
header('Location: /my-redirect');

echo 'Hello, World!';

And check what status code you actually get. If you get a 302 from that, you should look into PHP and webserver configuration.

@adambro

This comment has been minimized.

Contributor

adambro commented Jan 11, 2016

Indeed, I was able to reproduce the issue using @AndrewCarterUK code above. I've got 302 Found and browser redirect afterwards.

I'm using Apache 2.4 from Docker image php:5.6-apache for the record. Closing as it's not Slim issue.

@adambro adambro closed this Jan 11, 2016

@akrabat

This comment has been minimized.

Member

akrabat commented Jan 11, 2016

This is a PHP feature. From the header manual page:

The second special case is the "Location:" header. Not only does it send this header back to the browser, but it also returns a REDIRECT (302) status code to the browser unless the 201 or a 3xx status code has already been set.

@iansltx

This comment has been minimized.

Contributor

iansltx commented Sep 28, 2017

Seems like we could work around that behavior though, based on:

https://github.com/php/php-src/blob/a51cb393b1accc29200e8f57ef867a6a47b2564f/main/SAPI.c#L814-L827

So the PSR-7 message format starts doing what you'd expect it to do.

Basically you'd have to treat the Location header uniquely, passing the response code along with the redirect.

Just checked RFC 7231: https://tools.ietf.org/html/rfc7231, and while it mentions 3xx and 201 explicitly, it also doesn't say that other status codes can't be used with the Location header. See: https://tools.ietf.org/html/rfc7231#section-7.1.2. So this is a case of the runtime being overly "helpful", removing fidelity from the PSR-7 representation.

@kelunik

This comment has been minimized.

Contributor

kelunik commented Sep 28, 2017

This is a PHP feature. From the header manual page:

It's rather a bug than a feature. ACME is one example that requires sending a location header for a 200 response, see https://tools.ietf.org/html/draft-ietf-acme-acme-07#section-7.3.1.

It should at least be ignored if an explicit status code is set, which seems to be handled when looking at the code @iansltx linked.

@iansltx

This comment has been minimized.

Contributor

iansltx commented Sep 28, 2017

FWIW this also works:

<?php header('Location: /foo'); http_response_code(202);

So revising App::respond() to include an explicit http_response_code() call at the end should clean this up, if we don't want to start handling individual headers differently.

kelunik added a commit to kelunik/Slim that referenced this issue Sep 28, 2017

Send correct status code regardless of location header
PHP thinks it's clever, but it isn't. Fixes slimphp#1730.
@kelunik

This comment has been minimized.

Contributor

kelunik commented Sep 28, 2017

See #2309 for a PR.

kelunik added a commit to kelunik/Slim that referenced this issue Sep 28, 2017

Send correct status code regardless of location header
PHP thinks it's clever, but it isn't. Fixes slimphp#1730.

akrabat added a commit that referenced this issue Nov 3, 2017

Send correct status code regardless of location header
PHP thinks it's clever, but it isn't. Fixes #1730.
@vlakoff

This comment has been minimized.

Contributor

vlakoff commented Nov 23, 2017

See the subsequent #2345.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment