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

REST: should process_post flow include content_types_accepted? #356

Closed
jbothma opened this issue Jan 2, 2013 · 10 comments
Closed

REST: should process_post flow include content_types_accepted? #356

jbothma opened this issue Jan 2, 2013 · 10 comments

Comments

@jbothma
Copy link

jbothma commented Jan 2, 2013

I think process_post should happen after the appropriate content_types_accepted callback has been called. In 0.6.1 it seems that content_types_accepted is ignored when post_is_create returns false and it doesn't seem like this changed up to master.

The point of this change is that even when I'm doing a "process sort of" POST, I still want to do different decoding given application/json or application/x-www-form-urlencoded. I can either do my processing in my provided callback or set the data in my state and do the processing in process_post as per the current API.

Am I missing something or does this sound sensible enough for a pull request?

@jbothma
Copy link
Author

jbothma commented Jan 2, 2013

This change might have to break stuff for rest handlers that conditionally return true from post_is_create and don't expect their content_types_accepted callbacks to be called when they returned false.

@essen
Copy link
Member

essen commented Jan 2, 2013

Yeah that's planned.

@teburd
Copy link
Contributor

teburd commented Jan 2, 2013

I have a PR/patch for this if your interested check it out.

@jbothma
Copy link
Author

jbothma commented Jan 2, 2013

@BFrog Are you referring to #296 ? I've already cherry-picked that for the moment as a workaround :). I think it's similar but not quite the same. Or is there another patch somewhere for this issue?

@essen
Copy link
Member

essen commented Jan 2, 2013

Hey @BFrog I was hoping I could see with you for these PRs at some point. I'm sick right now so not exactly in the mood for it but perhaps next week?

@teburd
Copy link
Contributor

teburd commented Jan 3, 2013

Sure that sounds good

@ivlis
Copy link
Contributor

ivlis commented Jan 7, 2013

#296 would be a great improvement! Have to fork and cherry pick it. Can't wait to see it in vanilla cowboy.

@essen
Copy link
Member

essen commented Jan 22, 2013

@BFrog I need a few changes to the existing PRs, and if you already have done POST can you open a PR for it too? Thanks.

@essen
Copy link
Member

essen commented Jan 30, 2013

Patches welcome.

@essen
Copy link
Member

essen commented Apr 11, 2013

Done in 5a171d0. Please open new tickets if there's issues. Thanks!

@essen essen closed this as completed Apr 11, 2013
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 a pull request may close this issue.

4 participants