Share HTTP status codes map between SAPIs #892
Conversation
hikari-no-yume
changed the title
| { 502, "Bad Gateway" }, | ||
| { 503, "Service Unavailable" }, | ||
| { 504, "Gateway Timeout" }, | ||
| { 505, "HTTP Version Not Supported" }, |
There was a problem hiding this comment.
506, 507, 508 and 510 could be added as well. What would be nice is to offer a userland function to expose a map of HTTP status codes as well.
P.S.: nice refactoring!
There was a problem hiding this comment.
👎 Those are not part of RFC, I think only those specified in RFC 7231 should be explicitly set.
👍 for some sort of getter.
|
Any reason why these code messages do not match RFC 7231 (https://tools.ietf.org/html/rfc7231#section-6.1)? |
| { 426, "Upgrade Required" }, | ||
| { 428, "Precondition Required" }, | ||
| { 429, "Too Many Requests" }, | ||
| { 431, "Request Header Fields Too Large" }, |
There was a problem hiding this comment.
These three above are not specified in RFC 7231.
There was a problem hiding this comment.
They are in RFC 6585, however.
There was a problem hiding this comment.
Which is an extension of obsoslete 2616, right?
There was a problem hiding this comment.
That doesn't obsolete 6585.
|
You will find an exhaustive list here: Hypertext Transfer Protocol (HTTP) Status Code Registry (IANA). Just copy/paste ;-). |
|
@Hywan That's handy, thanks. Though I don't think any of the ones in that which we don't have are worth adding, unless they're actually used much. |
hikari-no-yume
force-pushed
the
unify_http
branch
from
5ad0a5f
to
b3dec6d
Compare
|
@TazeTSchnitzel Used or not, if you would like to be exhaustive, you have to follow this list :-). Especially if you expose this API to the userland. |
|
I suggest not adding strings to our binary if we dont use them. |
| @@ -449,8 +401,8 @@ static int sapi_cgi_send_headers(sapi_headers_struct *sapi_headers TSRMLS_DC) | |||
| } | |||
| err++; | |||
| } | |||
| if (err->msg) { | |||
| len = slprintf(buf, sizeof(buf), "Status: %d %s\r\n", SG(sapi_headers).http_response_code, err->msg); | |||
| if (err->str) { | |||
There was a problem hiding this comment.
@TazeTSchnitzel This condition can be omitted, because current shared map no longer contains code-text pair {0, NULL} as did previous map. According to this case, the else-block will never get run.
There was a problem hiding this comment.
Oh god, how does it know it reached the end, then? I need to add {0, NULL} back D:
There was a problem hiding this comment.
Aha. But the header should not really depend on other layer implementation, should it?
There was a problem hiding this comment.
I guess so, but terminating with {0, NULL} isn't really a bad idea.
|
@jpauli It's a header, no strings are added unless it's used somewhere. |
| @@ -457,8 +408,8 @@ static int sapi_cgi_send_headers(sapi_headers_struct *sapi_headers TSRMLS_DC) | |||
| } | |||
| err++; | |||
| } | |||
| if (err->msg) { | |||
| len = slprintf(buf, sizeof(buf), "Status: %d %s\r\n", SG(sapi_headers).http_response_code, err->msg); | |||
| if (err->str) { | |||
There was a problem hiding this comment.
This one is also able to be omitted (reason).
There was a problem hiding this comment.
Oh god, I'm surprised this isn't segfaulting, oh god...
hikari-no-yume
force-pushed
the
unify_http
branch
from
b3dec6d
to
65768ed
Compare
|
Well, it's been a while with no real complaints, so I'll merge this. |
|
I screwed up when I merged it and I reverted it, new pull request here: #957 |
There's some other stuff (header parsing etc.) which also ought to be shared, but this is, at least, a start.
This also means other code in PHP can benefit from this info.