Skip to content
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

Support reading the RAW POST body #1443

Closed
wants to merge 1 commit into from
Closed

Support reading the RAW POST body #1443

wants to merge 1 commit into from

Conversation

itamar82
Copy link

Added method getPostBody() to CHttpRequest in order to support reading the entire POST body contents.

The method is using file_get_contents("php://input") rather than $HTTP_RAW_POST_DATA for efficiency.

Using file_get_contents("php://input") rather than $HTTP_RAW_POST_DATA
*/
public function getPostBody()
{
return file_get_contents("php://input");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

http://php.net/manual/en/wrappers.php.php

Note: A stream opened with php://input can only be read once; the stream does not support seek operations. However, depending on the SAPI implementation, it may be possible to open another php://input stream and restart reading. This is only possible if the request body data has been saved. Typically, this is the case for POST requests, but not other request methods, such as PUT or PROPFIND.

Your current code won't work properly.

@resurtm
Copy link
Contributor

resurtm commented Sep 21, 2012

CHANGELOG line is also needed.

@@ -186,6 +186,15 @@ public function getPost($name,$defaultValue=null)
{
return isset($_POST[$name]) ? $_POST[$name] : $defaultValue;
}

/**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, try to keep consistency. You have space indented code and there are no trailing dots (look at the other methods of CHttpRequest).

@resurtm
Copy link
Contributor

resurtm commented Sep 21, 2012

Btw, this is a BC breaking PR. Suppose we have the following code:

public function actionIndex()
{
    // imagine that this line located somewhere inside CHttpRequest and has been appeared in 1.1.13
    CVarDumper::dump(file_get_contents("php://input"));

    // imagine that this line was added by an end user before 1.1.13 were released
    CVarDumper::dump(file_get_contents("php://input"));
}

We decided to perform PUT request after upgrading our project to 1.1.13:

curl -H "Content-Type: text/plain" -X PUT -d 'test' "http://127.0.0.1:8000/?r=site/index"

Result ('test' string and '' empty string):

'test'''

@mdomba
Copy link
Member

mdomba commented Sep 21, 2012

I don't see why would someone call
Yii::app()->request->getPostBody()
instead of
file_get_contents("php://input")

@resurtm
Copy link
Contributor

resurtm commented Sep 21, 2012

@mdomba, use case: assume you have to retrieve raw POST body in two independent places in your project. As i pointed above you can't just call file_get_contents("php://input") twice or more times (this is by design in PHP). So how would you solve this task? Store request body inside application parameters? Global variable? Create your own version of the HttpRequest component?

With implemented CHttpRequest::getRawBody() this problem simply disappears.

My suggestion on how should be implemented this feature (just a draft to show my idea—not tested code):

public function getRawBody()
{
    static $rawBody;
    if($rawBody===null)
        $rawBody=file_get_contents('php://input');
    return $rawBody;
}

Of course we should mention this change in UPGRADE file. I'm voting for merging this enhancement, but only when it becomes better.

@creocoder
Copy link
Contributor

I propose CHttpRequest::getPostRawBody() name.

@resurtm
Copy link
Contributor

resurtm commented Sep 21, 2012

I propose CHttpRequest::getPostRawBody() name.

@creocoder, not only POST request can have body, as i pointed above. It's very strange to invoke getPostRawBody() when serving PUT-request, isn't it?

@creocoder
Copy link
Contributor

@resurtm Yes, you are right! :)

@mdomba
Copy link
Member

mdomba commented Sep 21, 2012

@resurtm I see your point... but how many times will someone need that.

In your example if someone needs this just once... than we are storing all the data and occupying the memory.

POST data can be really big

@itamar82
Copy link
Author

This change is for the use case of RESTful APIs where either XML or JSON data is put directly in the request body.

@resurtm I understand the issue you brought up about calling file_get_contents("php://input") further upstream. I tried to look up some ways of perhaps intercepting that stream wrapper or replacing it simply for "php://input" but it doesn't seem possible.

So the plan is to rename the method to getRawBody(), align the code to match the code conventions and implement the method with a static store.

@samdark
Copy link
Member

samdark commented Sep 21, 2012

I think we already have it. It's named getRestParams. I think we should rename this method leaving deprecated getRestParams as alias for BC.

@itamar82
Copy link
Author

ahhhh, thanks @samdark, this does look like it would work, but the method is marked as protected.

@itamar82 itamar82 closed this Sep 21, 2012
@itamar82 itamar82 reopened this Sep 21, 2012
@ghost ghost assigned samdark Sep 21, 2012
@samdark
Copy link
Member

samdark commented Sep 21, 2012

Will handle this one by adding better named public alias to existing protected method and deprecating protected method.

@itamar82 you can speed it up a bit by updating pull-request with this solution.

@samdark samdark closed this in c8aa982 Sep 24, 2012
@samdark
Copy link
Member

samdark commented Sep 24, 2012

Thanks for ideas and code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants