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

Response should accept request query parameters #160

Open
logistiker opened this issue Jul 17, 2013 · 18 comments
Open

Response should accept request query parameters #160

logistiker opened this issue Jul 17, 2013 · 18 comments

Comments

@logistiker
Copy link

It would be good if the Response class could accept an array for query parameters. This would allow application wide manipulation of the response class based on $_GET, $_POST, etc params (e.g. ?plain_text=1 to override the mimetype and send plain/text instead -- useful for development of things like application/hal+json within Firefox).

@peej
Copy link
Owner

peej commented Jul 17, 2013

What advantage would this give over simply using the $_GET and $_POST superglobals?

@logistiker
Copy link
Author

The response class has no knowledge of the request method. So you would pass in the superglobal that's available within the resource through the request to the response.

@peej
Copy link
Owner

peej commented Jul 17, 2013

True but it seems like an unlikely scenario that you'd want to do anything to the response based on the data within a POST request since POST data is supposed to change resource state not resource responses.

@logistiker
Copy link
Author

Yes you are correct. It wouldn't make sense to pass $_POST in but most definitely it would make sense to pass in $_GET.

@logistiker
Copy link
Author

Of course you would only pass in $_GET for @method get methods as well.

@peej
Copy link
Owner

peej commented Jul 18, 2013

So then you could just use the $_GET superglobal.

If we did want to provide a processed querystring to the response then the best way I think would be to add this information into the Request object and then make the request available to the response. That way all the request stuff is held together and the response would get access to the whole request.

Since PHP already handles parsing the querystring and providing it in the $_GET superglobal, it doesn't seem to make much sense to then copy this into the Request object except for completeness of the Request object data.

@logistiker
Copy link
Author

I think $_GET should really just live in a resource though. You shouldn't have to set $_GET just to do unit tests on your end for Response since $_GET isn't set in a unit test environment. This is why I was suggesting to allow a generic array to be passed in where you would usually just pass it $_GET within the resource. In my case, I extended Response, added a generic private queryParams property, and overwrote __set and then checked the existence of the key to change the mimetype just as ContentType is set. I don't want $_GET to be in there on my side because the class really shouldn't have any knowledge of what that means because it's not set in a unit testing context. I know I could set $_GET in the unit test but it just seems wrong.

@logistiker
Copy link
Author

The other thing about this is strictly speaking how does the Response class even know if $_GET exists? It potentially could be called outside of a Resource context albeit it may not be.

@logistiker
Copy link
Author

Seems other people agree with me that it should be limited in scope where it's used:

http://stackoverflow.com/questions/132342/testing-form-inputs-in-phpunit

@drkibitz
Copy link
Contributor

This feature makes sense to be implemented in the the request object like described, but I honestly don't like the idea of modifying the $_GET super global for everything (though many times this is the best solution). I know REST is for HTTP, but tonic does not need to be limited to a webserver, it could also be used for PHP cli, where we could just pass a url as a cli argument or option. This could actually be a cli dispatcher, where it is this dispatcher's job to parse the url and pass the appropriate info to the request object.

@tvandame
Copy link

Tonic The Right Way Rocks!

@logistiker
Copy link
Author

I like the idea of passing the request object into the response. If $_GET parameters were attached to the request, then it would be trivial to manipulate your response using query params. Tonic used to pass the request object into the response object a while ago but for some reason now no longer accepts the request object.

@peej
Copy link
Owner

peej commented Aug 9, 2013

Taking the $_GET array and pushing it into the Request object makes sense from a completeness perspective. The querystring is part of the request so it should be in the Request object.

The problem is that a lot of other stuff that isn't in the Request object is also part of the request, so do we add all of that too? A line has to be drawn somewhere between what is useful and what makes sense conceptionally.

I would err on the side of caution and add less stuff rather than more. The querystring is already made available by PHP in the $_GET superglobal and people know to look for it there, why complicate things by duplicating it somewhere else?

I'm still on the fence. Someone needs to convince me.

It would be as simple as doing:

$this->query = $_GET;

in the Request constructor. The Request object could then be attached to the Response object (or passed to it's output() method) in the dispatcher if you want to use it inside your response.

@logistiker
Copy link
Author

I'm not entirely sure what you're on the fence about. It makes sense to make the request object available to the response so you manipulate the response based on what happened in the request. If you needed to know the mimetype of the request for example to manipulate the response based on that, you don't have any way to get that data other than getting it from the request object that is passed in through the construct. This worked well in an old version of Tonic and I'm not sure why it was taken out.

You're correct that $_GET is available anywhere since it's a superglobal but conceptually speaking, the response class really should not know what $_GET since it originates from a request.

@logistiker
Copy link
Author

I don't think your suggestion to use an IoC helps. You're simply setting a public property in that case on the response which I would not advise due to the __set() method in that class.

@peej
Copy link
Owner

peej commented Aug 11, 2013

For me, the Response should be dumb. If you want a different response based upon something (the request or the resource) then your Resource should be returning a different Response object.

@logistiker
Copy link
Author

I guess you do have a point. If the base response has no need for the base request, why pass it in? It just seems natural to me that the response is going to be generated by at least the mime type requested in the request object or $_GET parameters but perhaps the default case is people will not write responses based on request mime types and will just serve whatever they want to serve.

1 similar comment
@logistiker
Copy link
Author

I guess you do have a point. If the base response has no need for the base request, why pass it in? It just seems natural to me that the response is going to be generated by at least the mime type requested in the request object or $_GET parameters but perhaps the default case is people will not write responses based on request mime types and will just serve whatever they want to serve.

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

No branches or pull requests

4 participants