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

Get json encoded data from POST and PUT requests #2059

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@radzserg

radzserg commented Jan 31, 2013

Added ability to get json encoded data from POST and PUT requests. I check $_SERVER['CONTENT_TYPE'] and then getRawBody() and decode it by CJSON. Also made some properties protected. Cause if you want to extend base CHttpRequest by your class you can't have ability to work with private properties.

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Jan 31, 2013

Member

I don't think it should be in since sometimes you want to deal with raw data. Additionally it can be easily done with just

CJSON::decode(Yii::app()->request->getRawBody());
Member

samdark commented Jan 31, 2013

I don't think it should be in since sometimes you want to deal with raw data. Additionally it can be easily done with just

CJSON::decode(Yii::app()->request->getRawBody());

@samdark samdark closed this Jan 31, 2013

@radzserg

This comment has been minimized.

Show comment
Hide comment
@radzserg

radzserg Jan 31, 2013

One of the reasons that made me think about such update was Backbone.js that send encoded data as JSON in "Request Payload" header.
But imagine any external REST server that send data in encoded JSON. Of course I can define some custom function and use it. But why should I do that if we can do it via framework. I don't want to worry about that I want to delegate it to framework.
And ok you're the boss here so you decide what's good.

But why did you do $_restParams as private. For instance I want to use my MyHttpRequest (setup correct app config for request component) that will extends CHttpRequest. BUT I haven't ability to use _restParams and other similar properties. Make sense?

radzserg commented Jan 31, 2013

One of the reasons that made me think about such update was Backbone.js that send encoded data as JSON in "Request Payload" header.
But imagine any external REST server that send data in encoded JSON. Of course I can define some custom function and use it. But why should I do that if we can do it via framework. I don't want to worry about that I want to delegate it to framework.
And ok you're the boss here so you decide what's good.

But why did you do $_restParams as private. For instance I want to use my MyHttpRequest (setup correct app config for request component) that will extends CHttpRequest. BUT I haven't ability to use _restParams and other similar properties. Make sense?

@radzserg

This comment has been minimized.

Show comment
Hide comment
@radzserg

radzserg Jan 31, 2013

And yes if I want to deal with raw data I can use Yii::app()->request->getRawBody() ;)

radzserg commented Jan 31, 2013

And yes if I want to deal with raw data I can use Yii::app()->request->getRawBody() ;)

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Jan 31, 2013

Member

You have getRestParams method so there should be no problem to implement what you suggest in your own custom class.

Member

samdark commented Jan 31, 2013

You have getRestParams method so there should be no problem to implement what you suggest in your own custom class.

@radzserg

This comment has been minimized.

Show comment
Hide comment
@radzserg

radzserg Jan 31, 2013

Right, but look at getPut

   public function getPut($name,$defaultValue=null)
{
    if($this->getIsPutViaPostRequest())
        return $this->getPost($name, $defaultValue);

    if($this->getIsPutRequest())
    {
        $this->getRestParams();
        return isset($this->_restParams[$name]) ? $this->_restParams[$name] : $defaultValue;
    }
    else
        return $defaultValue;
}

Even if I override getRestParams I need to override getPut and getDelete - _restParams is private it won't be accesible in parent class. (I described this problem here http://www.yiiframework.com/forum/index.php/topic/39867-how-to-get-json-decoded-postdelete-data )
$this->getRestParams();
return isset($this->_restParams[$name]) ? $this->_restParams[$name] : $defaultValue;

radzserg commented Jan 31, 2013

Right, but look at getPut

   public function getPut($name,$defaultValue=null)
{
    if($this->getIsPutViaPostRequest())
        return $this->getPost($name, $defaultValue);

    if($this->getIsPutRequest())
    {
        $this->getRestParams();
        return isset($this->_restParams[$name]) ? $this->_restParams[$name] : $defaultValue;
    }
    else
        return $defaultValue;
}

Even if I override getRestParams I need to override getPut and getDelete - _restParams is private it won't be accesible in parent class. (I described this problem here http://www.yiiframework.com/forum/index.php/topic/39867-how-to-get-json-decoded-postdelete-data )
$this->getRestParams();
return isset($this->_restParams[$name]) ? $this->_restParams[$name] : $defaultValue;

@samdark samdark reopened this Jan 31, 2013

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Jan 31, 2013

Member

That is a valid one but not the PR itself.

Member

samdark commented Jan 31, 2013

That is a valid one but not the PR itself.

@ghost ghost assigned samdark Jan 31, 2013

@radzserg

This comment has been minimized.

Show comment
Hide comment
@radzserg

radzserg Feb 1, 2013

Well I got some new thought about that issue. You are definitely right about BC. Of course somebody already uses this component and expects certain behavior. And automatic JSON decode could break the app.

But I'll tell you where you are wrong. Take a look again at
public function getPut($name,$defaultValue=null)
we are expecting to get some variables from PUT by its $name
$this->getRestParams();
return isset($this->_restParams[$name]) ? $this->_restParams[$name] : $defaultValue;
If we don't decode JSON data we will get null. Already it's not very good for me. Ok I assume that you'll suggest to use getRawData and then decode and parse it manually. BUT what about csrf verification ? It won't work automatically.
public function validateCsrfToken($event)
...
$userToken=$this->getPut($this->csrfTokenName);
We will get null here and fail verification.

So I propose to add additional public property $autoDecodeJson = false and then use it during fetching data. This won't break BC and add some new functionality to framework.

What do you think about that note?

radzserg commented Feb 1, 2013

Well I got some new thought about that issue. You are definitely right about BC. Of course somebody already uses this component and expects certain behavior. And automatic JSON decode could break the app.

But I'll tell you where you are wrong. Take a look again at
public function getPut($name,$defaultValue=null)
we are expecting to get some variables from PUT by its $name
$this->getRestParams();
return isset($this->_restParams[$name]) ? $this->_restParams[$name] : $defaultValue;
If we don't decode JSON data we will get null. Already it's not very good for me. Ok I assume that you'll suggest to use getRawData and then decode and parse it manually. BUT what about csrf verification ? It won't work automatically.
public function validateCsrfToken($event)
...
$userToken=$this->getPut($this->csrfTokenName);
We will get null here and fail verification.

So I propose to add additional public property $autoDecodeJson = false and then use it during fetching data. This won't break BC and add some new functionality to framework.

What do you think about that note?

@radzserg

This comment has been minimized.

Show comment
Hide comment
@radzserg

radzserg commented Mar 1, 2013

Added new commit radzserg@6398086

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Mar 5, 2013

Member

As I've mentioned before, automatic JSON decode will not be accepted. Still I've refactored CHttpRequest a bit: e37a9fe Thanks for pointing it out.

Member

samdark commented Mar 5, 2013

As I've mentioned before, automatic JSON decode will not be accepted. Still I've refactored CHttpRequest a bit: e37a9fe Thanks for pointing it out.

@samdark samdark closed this Mar 5, 2013

@radzserg

This comment has been minimized.

Show comment
Hide comment
@radzserg

radzserg May 15, 2013

Agreed with @samdark - this should be closed. You can add this functionality to custom Request class. But it's quite difficult to add this to yii thinking about BC.
Maybe yii team will add such functionality in Yii2 👍

radzserg commented May 15, 2013

Agreed with @samdark - this should be closed. You can add this functionality to custom Request class. But it's quite difficult to add this to yii thinking about BC.
Maybe yii team will add such functionality in Yii2 👍

@cebe

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe May 15, 2013

Member

@radzserg feel free to create an issue: https://github.com/yiisoft/yii2/issues

Member

cebe commented May 15, 2013

@radzserg feel free to create an issue: https://github.com/yiisoft/yii2/issues

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