Support for LINK/UNLINK http methods into http parser #1842
Conversation
| @@ -621,6 +623,8 @@ size_t php_http_parser_execute (php_http_parser *parser, | |||
| } else { | |||
| parser->method = PHP_HTTP_NOT_IMPLEMENTED; | |||
| } | |||
| } else if (index=1 && parser->method == PHP_HTTP_LOCK && ch == 'I') { | |||
|
We can note that even if it's still a draft, the last 2015 sept. revision (12) make the server implementation requirements simpler. I admit there's some debate about supporting LINK/UNLINK methods. Its not implemented in python & ruby for example but in nodejs. I personally have tests in many webservice apps that need to fire some fastcgi http server with php-fpm just because neither of the 2 methods are supported, that's a pity. |
shouze
changed the title
| @@ -79,6 +79,9 @@ enum php_http_method | |||
| , PHP_HTTP_POST | |||
| , PHP_HTTP_PUT | |||
| , PHP_HTTP_PATCH | |||
| /* RFC-2068, section 19.6.1.2 */ | |||
There was a problem hiding this comment.
You're referring to RFC 2068 which has been obsoleted by RFC 2616 which has been obsoleted by RFC 7230ff.
There was a problem hiding this comment.
Yes but RFC-2068, section 19.6.1.2 explains pretty well the 2 methods. There's 8 RFCs about HTTP/1.1, without counting the ones about HTTP/2.0 and so on ;)
|
Seems like there is no consensus yet on merge or not merge. |
|
I'm not sure if this is a good idea, regardless of the status of this particular HTTP verb: The HTTP spec allows for extension with any verb, it would appear that what we really want is for a way to support any extension, rather than trying to decide which specific ones to add ? |
|
Yeah, I think everyone agrees that we should allow any request method. Someone just has to implement it. |
|
indeed we should embrace HTTP extensibility |
|
Okay then, since we and the author of this PR agree this is not the right solution to the problem, closing. |
|
We are using the node.js HTTP parser (https://github.com/nodejs/http-parser). Unfortunately, they have already rejected a number of PRs adding support for non-standard HTTP methods, so there's little hope we'll see an upstream solution for this. |
|
We are already pulling in upstream changes, I think we'll have to modify it and just deal with the more difficult merges ... modify it with the least changes possible to minimise pain ? |
AFAIK, our version has never been synched with upstream, as the different copyright notes show: nodejs/http-parser@8dabce6 vs. https://github.com/php/php-src/blob/master/sapi/cli/php_http_parser.c. |
|
Oh, that's surprising ... thanks for checking :) It's either a big issue that we are not in sync, or it doesn't matter if we change the parser then ? I'm not sure which. |
I don't think that it's an issue to be not in sync. Even if there would be security issues fixed in "upstream", these won't really affect us, as the built-in webserver is not meant for production usage. So we're free to make whatever changes we like without bothering about "upstream" merges. |
|
I wonder if we maybe should sync the parser with upstream once before implementing any major deviations? I think all changes we applied to the parser were related to request methods and those should also have happened upstream as well, so we don't lose anything there. |
That seems sensible to me. |
Fixes #68875.