Added put, patch and delete request_methods plus tests. #1519

kalaspuffar · 2015-09-14T21:16:27Z

Summary:

Added support for request methods with the smallest change possible.

History:

HTTP 1.0 had only support for GET, POST and HEAD.
In June of 1999 HTTP 1.1 added the verbs PUT and DELETE and later in 2010 RFC5789 (https://tools.ietf.org/html/rfc5789) added PATCH.

Pull request explaination:

In this pull request I try to add put, patch and delete with changing as few functions as possible. I simply handle the new verbs as POST. As defined in HTTP 1.1 they are similar in input data requirements and differ only in usage.

Known side effects

Because this PR tries to handle PUT, PATCH and DELETE exactly as POST the php://input stream will be read. So as POST one need to set the enable_post_data_reading to fetch the stream from these requests.

After reading discussion linked by Tyrael where the suggestion below already has been discussed in length I'll remove the suggestion so we could focus on the main part of the PR.
More work I could do is add new globals for $_PUT, $_PATCH and $_DELETE but I think this might be more confusing. Or we could move from $_POST to a new global not tied to a verb. Example $_PARSED_INPUT

Mentioned in bugs

https://bugs.php.net/bug.php?id=55815
https://bugs.php.net/bug.php?id=70331

Tyrael · 2015-09-14T21:34:30Z

just linking the last time we discussed this:
http://grokbase.com/t/php/php-internals/13a2kmd39x/http-supergloblas-and-request-body-query-was-parsing-put-data

krakjoe · 2017-01-01T16:34:25Z

@kalaspuffar fix conflicts please

staabm · 2017-01-01T17:08:48Z

main/SAPI.c

+			!strcmp(SG(request_info).request_method, "PUT") ||
+			!strcmp(SG(request_info).request_method, "PATCH") ||
+			!strcmp(SG(request_info).request_method, "DELETE"))
+		) {
 			/* HTTP POST may contain form data to be processed into variables


Comment needs minor update, as it only/explicitly mentions POST

kelunik · 2017-01-01T17:43:30Z

run-tests.php

+
+		save_text($tmp_post, $request);
+		$cmd = "$php $pass_options $ini_settings -f \"$test_file\" 2>&1 < \"$tmp_post\"";
+


Seems like a lot of duplicated code here.

kelunik · 2017-01-01T17:45:21Z

main/php_content_types.c

+        !strcmp(SG(request_info).request_method, "PUT") ||
+        !strcmp(SG(request_info).request_method, "PATCH") ||
+        !strcmp(SG(request_info).request_method, "DELETE")
+        ) {


Mixed tabs and spaces here.

kelunik · 2017-01-01T17:50:39Z

run-tests.php

-	/* For GET/POST/PUT tests, check if cgi sapi is available and if it is, use it. */
-	if (!empty($section_text['GET']) || !empty($section_text['POST']) || !empty($section_text['GZIP_POST']) || !empty($section_text['DEFLATE_POST']) || !empty($section_text['POST_RAW']) || !empty($section_text['PUT']) || !empty($section_text['COOKIE']) || !empty($section_text['EXPECTHEADERS'])) {
+	/* For GET/POST/PUT/PATCH/DELETE tests, check if cgi sapi is available and if it is, use it. */
+	if (!empty($section_text['GET']) || !empty($section_text['POST']) || !empty($section_text['GZIP_POST']) || !empty($section_text['DEFLATE_POST']) || !empty($section_text['POST_RAW']) || !empty($section_text['PUT']) || !empty($section_text['PATCH']) || !empty($section_text['DELETE']) || !empty($section_text['COOKIE']) || !empty($section_text['EXPECTHEADERS'])) {


We have POST, GZIP_POST, DEFLATE_POST, POST_RAW, why only PUT, PATCH and DELETE? I have no idea what this line does, it's just an inconsistency I saw.

Not sure we should add all this test facilities for all request-methods. I would add those only when we actually have tests which require them

staabm · 2017-01-01T18:09:24Z

main/php_content_types.c

@@ -37,7 +37,13 @@ static sapi_post_entry php_post_entries[] = {
 */
 SAPI_API SAPI_POST_READER_FUNC(php_default_post_reader)
 {
-	if (!strcmp(SG(request_info).request_method, "POST")) {
+	if (
+        !strcmp(SG(request_info).request_method, "POST") ||


Should this be && instead of ||?

staabm · 2017-01-01T18:16:58Z

run-tests.php

@@ -1770,6 +1770,10 @@ function run_test($php, $file, $env)
 			$request .= $line;
 		}

+		if (empty($env['CONTENT_TYPE'])) {


Looks like a unrelated change in old POST logic?

staabm · 2017-01-01T18:18:11Z

run-tests.php

+		save_text($tmp_post, $request);
+		$cmd = "$php $pass_options $ini_settings -f \"$test_file\" 2>&1 < \"$tmp_post\"";
+
+	} elseif (array_key_exists('DELETE', $section_text) && !empty($section_text['DELETE'])) {


Doesnt it require a similar branch for PUT?

staabm · 2017-01-01T18:31:47Z

I am not convinced this is a feature which is really required. Most frameworks abstract away the superglobals as those make testing hard and introduce global dependencies.

As this raw-data is also available via stdin [1], this kind of stuff is easy to handle in userland and should not require php-src level changes IMO.

[1] http://php.net/manual/en/features.file-upload.put-method.php

krakjoe · 2017-02-22T08:32:14Z

Having waited more than a month for feedback, and having received nothing, I'm closing this PR as it would appear abandoned.

kalaspuffar · 2017-02-22T08:35:49Z

@ALL @krakjoe because of all the activity since your post it wasn't clear who should do what or if the conflicts where solved. But if your and the community isn't interested then I'm not going to do the work.

smalyshev added the Feature label Sep 28, 2015

kalaspuffar force-pushed the add_patch_put_delete branch from a31821e to 54286af Compare October 26, 2015 19:28

kalaspuffar force-pushed the add_patch_put_delete branch from 54286af to fb0e240 Compare January 4, 2016 09:08

Added put, patch and delete request_methods plus tests.

fb0e240

staabm reviewed Jan 1, 2017

View reviewed changes

kelunik reviewed Jan 1, 2017

View reviewed changes

staabm reviewed Jan 1, 2017

View reviewed changes

krakjoe added the Waiting on Author label Jan 4, 2017

krakjoe closed this Feb 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added put, patch and delete request_methods plus tests. #1519

Added put, patch and delete request_methods plus tests. #1519

kalaspuffar commented Sep 14, 2015

Tyrael commented Sep 14, 2015

krakjoe commented Jan 1, 2017

staabm Jan 1, 2017

kelunik Jan 1, 2017

kelunik Jan 1, 2017

kelunik Jan 1, 2017

staabm Jan 1, 2017

staabm Jan 1, 2017

staabm Jan 1, 2017

staabm Jan 1, 2017

staabm commented Jan 1, 2017 •

edited

Loading

krakjoe commented Feb 22, 2017

kalaspuffar commented Feb 22, 2017


		save_text($tmp_post, $request);
		$cmd = "$php $pass_options $ini_settings -f \"$test_file\" 2>&1 < \"$tmp_post\"";

Added put, patch and delete request_methods plus tests. #1519

Added put, patch and delete request_methods plus tests. #1519

Conversation

kalaspuffar commented Sep 14, 2015

Summary:

History:

Pull request explaination:

Known side effects

Mentioned in bugs

Tyrael commented Sep 14, 2015

krakjoe commented Jan 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staabm commented Jan 1, 2017 • edited Loading

krakjoe commented Feb 22, 2017

kalaspuffar commented Feb 22, 2017

staabm commented Jan 1, 2017 •

edited

Loading