Set response correctly for json API responses #3545
Conversation
|
The errors being given by the Static Analysis Check are not very descriptive. If anyone can help with that it would be appreciated. |
|
@caswell-wc you need to fix travis build first https://travis-ci.org/github/codeigniter4/CodeIgniter4/jobs/721516405 because your change made different tests expectation. That's the first priority. For phpstan, it seems your change cause ignored error can be removed:
|
|
I created PR #3547 for phpstan notice. You can focus on tests. |
|
please rebase latest develop |
|
@jim-parry 's original intent was for this to be handled by a Format Filter, see #2185. I will leave it up to the team if that still seems worth doing, but I'm afraid this PR would preclude that functionality for the |
|
Actually looking at this change I'm afraid this would constitute a breaking change. I realize there is some expectation that |
|
@MGatner I think I understand what you are saying about the breaking change. The real issue that I am trying to resolve here, as explained in #3079 is that if the body is formatted as json, it never actually sets Response::bodyFormat to 'json'. It stays as html. Because it never gets changed it ends up json_encoding the body of the response twice before the assertJSONFragment can json_decode it. I researched this when I first created the issue and found that if the bodyFormat gets updated correctly, it solves the problem which is why I used the setJSON function because it also sets the bodyFormat. The other option that I thought of was to create a setter on the Response object for bodyFormat and then call that in the ResponseTrait::format method when it is setting the content type on the response. Is that a more viable solution? |
|
You need rebase instead of merge. Your commits history seems already ruined now, you can do like the following: git checkout -b temp
git checkout json-api-response
git reset --hard HEAD~52
git checkout develop
# pull latest develop via git url
git pull [email protected]:codeigniter4/CodeIgniter4.git develop
# or pull latest develop via https url
git pull https://github.com/codeigniter4/CodeIgniter4.git develop
git checkout json-api-response
git cherry-pick 8b4e5dcbf4028f0e07d2eafe7ace92d97cd943a1
git cherry-pick d4e307cff53eb81fc887e9761c6c61585c15713e
git cherry-pick 3ea8a08de3e599dee38b54ea35627a85d0a39b3d
git cherry-pick 811c17d86ee30fcf36e12a1488a4f6960ff31fd4
# finally, force push
git push origin json-api-responseNext time, you need to rebase instead, I created a tutorial for clean rebase at https://samsonasik.wordpress.com/2015/09/16/practical-git-4-rebasing-conflicted-task-branch-against-primary-branch/ |
|
Definitely implement the rebase samsonasik outlined above. I think I see your point now, the issue is not the formatting of the body but rather notifying the response of the format type. I will have to look at this a bit more, or hear feedback from others. Jim had this all figured out but I'm not nearly as adept at RESTful implementations as he was. I think his goal was to have the actual Controller to be "format agnostic" and have the Filter handle the formatting (unless overridden). This does seem to be in tension with the current |
|
I actually thought I had rebased it but I ran into issues along the way and when I look back at it, I think I know where I went wrong and it ended up merging instead. Once we figure out the best path to accomplish what we are trying to do, I will either fix that or I may just close this PR and create a new one. |
|
Sounds good! Thank you for bringing this up - we'll get a little more discussion going and figure it out. |
|
I originally had a vision of being able to have the controller negotiate the correct content type with the client in a little utopian vision once myself. IIRC, playing with some API generation proved it didn't work out as well as hoped, as utopias often don't. :) I prefer to allow the developer to explicitly declare the output format. The ResponseTrait seems like a good place to do that. However, we would have to provide for all supported output formats (which is only XML and JSON at this time) to be set, not just JSON. My other concern with the filter Jim recommended was the possibility of conversion errors across the different formats and back. It might not be a big deal but I'm concerned the number of edge cases would be a pain. Especially for a feature where 90% of the use case is going to be to spit out JSON anyway lol. |
|
@lonnieezell So are you thinking that we stick with the change in this PR but also add logic to use the Response::setXML when the format is 'xml'? |
|
@lonnieezell @MGatner Do either of you have anymore thoughts about this. I can see 2 options.
I like the second option better because it is allowing the ResponseTrait to format the response instead of needlessly sending the formatted response through another format method which of course gets ignored because it's already formatted. The only downside I see to the second option is that it does expose to the developer the ability to set the bodyFormat. I am not aware of a problem with this but you guys know the system better then I do. |
|
@caswell-wc Could you rebase first? I'm trying to look back at my notes on this and the proposed changes but they are hard to see with all the intermediary cruft. Let me know if you need help with that command sequence - I think @samsonasik also has a handy blog on the subject. |
|
Bump! |
We were getting an error when running running setStatusCode on the response directly. This puts it back where it was before.
…tting it as json. Modified the format method so it sets the format to hmtl if the data is a string.
|
@MGatner Sorry about the confusion on this. I have been having a lot of issues with this rebase stuff. I'm pretty new to it. Anyway, I think I got it now. Please let me know if I still need to do something with it. |
|
Yes looks much better! Thank you. I think with 4.1 around the corner we do take the easy route to try to get this done.
Let's do this for now (add XML support) and then if we need another pass at the handling later we can revisit. |
…rmat is xml and added tests for the setJSON functionality.
There was a problem hiding this comment.
Yay! Looks good. Thanks so much. Anyone else? @samsonasik ?
fixes #3079
Description
In the CodeIgniter\API\ResponseTrait it defaults the format to json and everything is treated as a json but when it goes to send the response, it uses the setBody function in the response object. The problem with this is that it never sets the bodyFormat correctly in the response object and that causes issues when running tests and trying to assert different things about the json because the response json is not encoded correctly. This change makes sure that the bodyFormat will be set correctly by using setJSON function when the format is json.