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

Adds head/2 and head/3 to PhoenixController. #818

Conversation

SteveAquino
Copy link

This adds head/2 and head/3 convenience functions for returning empty responses with just a status code and optionally a specific content type. Per this thread on phoenix-talk: https://groups.google.com/forum/#!topic/phoenix-talk/110izD_r-Zw

Examples:

  iex> head conn, 200
  iex> head conn, :ok
  iex> head conn, :ok, "application/json"

This adds head/2 and head/3 convenience
functions for returning empty responses
with just a status code and optionally
a specific content type.

Examples:

  iex> head conn, 200
  iex> head conn, :ok
  iex> head conn, :ok, "application/json"
@josevalim
Copy link
Member

I am a bit unsure about this addition:

  • We have no higher-level API that accept the status code. Only plug's put_status/2.
  • We have no higher-level API that accept the content-type code. Only plug's put_resp_content_type/2.

In other words, this doesn't look like it fits the rest of the controller API. Also, head/2 should really not set the content type. It may make sense for your application but this is not something we should promote framework wise.

I would propose head/1, that just calls send_resp/3 but if there is a strong preference I am ok with head/2 too. It just feels it would be a bit inconsistent with other APIs.

Thank you @SteveAquino!

@chrismccord
Copy link
Member

:+1 to what @josevalim said. head/1 is nice because sometimes people get tripped up when they want to "send no response", but they have to send an empty string. @SteveAquino can you make the changes to just support head/1? Please also include in the function docs an example of pipeline put_status and put_response_content_type to give a complete picture. Thanks!

@scrogson
Copy link
Contributor

Does head/1 even make sense? A head response status is generally 204 No Content, not 200 OK. I think head/2 makes more sense, but head/3 does not.

Am I missing something?

@chrismccord
Copy link
Member

head/1 would just be send_resp(conn, ""). Not sure how often it would be used, but we have function to compose the status and resp_content_type, so that's all it would do

@SteveAquino
Copy link
Author

The idea of this function is to, as chris pointed out, simplify an empty response. Sometimes that isn't necessarily a 204 (for example, a 401 unauthorized), and I imagine this is fairly common in API based applications. Also, this was intended to be inline with text/2, json/2, and html/2 in that a single function can handle common responses, so I don't see how this isn't inline with the current API.

Finally, and maybe this isn't a good reason, but the inspiration came from rails land were you can call head :ok and return an empty response with a specific status code (http://guides.rubyonrails.org/layouts_and_rendering.html#using-head-to-build-header-only-responses).

Given that, I would recommend at least head/2 and see no benefit otherwise. The reason for head/3 to specify content type is that the previous mentioned functions (text/2, json/2, and html/2) all set a content type, but I can see that being unnecessary.

@SteveAquino
Copy link
Author

@josevalim Just realized I did not address one of your comments, but head/2 does NOT set a content type, only head/3. See my last paragraph on the previous comment, but I'm fine removing head/3 given that it doesn't seem necessary. I'm also really curious on your thoughts about why this doesn't follow the existing API since my intention was to follow the convention set by text/2, html/2, json/2, and redirect/2.

Thanks for all the feedback!

@josevalim
Copy link
Member

I'm also really curious on your thoughts about why this doesn't follow the existing API since my intention was to follow the convention set by text/2, html/2, json/2, and redirect/2.

It doesn't just in the fact it receives the status code while the functions above do not.

Thanks for taking the time to improve Phoenix and participate in the discussion! :)

@djm
Copy link
Contributor

djm commented Apr 27, 2015

A few questions I have:

I don't come from a Rails background so while the name may seem obvious to most - it leaves me quite baffled. head refers to two things in my mind:

  • The first element of a list, this is confusing to me as Elixir already has an hd function which will be pronounced in exactly the same way by most people.
  • HEAD requests (not responses).

Is there a reason for the name other than "Rails did it"?

My 2nd question, and I wish I could check the codebase for the answer now but the wifi in this hotel is absolutely terrible: why can't we just have send_resp set the content/body arg to empty string by default?

It seems to me that this adds a level of indirection for doing a really simple thing. To me send_resp, conn, 200 is much clearer than head conn, 200.

@SteveAquino
Copy link
Author

@djm The name is entirely inspired by Rails, but I see your point about this being confusing, especially given the Erlang/Elixir terminology. Also I see @josevalim's point about sending the status code being inconsistent with the other APIs.

The reason I suggested this function was because it took me a long time to figure out how to send an empty response, so maybe the documentation needs improving more than the code. Also I feel like send_resp/3 is a lower level API since this function is never mentioned in the docs, which is why I went the route of adding a semantically named convenience function that wraps that function.

I'm not sure the correct approach here, but it seems like SOMETHING is missing, perhaps we just need to add an example to the docs and call it a day, because really that's what tripped me up.

@josevalim
Copy link
Member

Maybe we should clarify that the functions in Plug.Conn are essential for using Phoenix.Controller effectively?

@SteveAquino
Copy link
Author

Personally I feel like I understood the relationship between Plug and Phoenix.Controller, but I think that what really tripped me up is that I was looking in the guide for an example and couldn't find one. Eventually I was able to find it in the hex docs, but I think that maybe a simple example would really help folks.

@2kodes
Copy link
Contributor

2kodes commented Apr 27, 2015

Just adding my two cents. but I believe there should really be a strong link or pointing to the fact that Phoenix uses Plugs underneath and that sometimes the functions you may not see in the Phoenix documentation will be available in the Plug documentation. Usually that seems to be what trips people up; mostly we all seem to forget about checking the Plug docs ..
👍 on clarifying that point..

@lancehalvorsen
Copy link
Member

@SteveAquino, I would love to make this more clear for you and others. If I understand correctly, you're asking for examples of send_resp/3 in the Controller Guide. Is that accurate? If not, please open an issue or submit a PR to the guides repo to help me better understand what you're looking for.
https://github.com/lancehalvorsen/phoenix-guides

@2kodes, I would love to clarify the issue you brought up as well. We do have this guide for Plug:
https://github.com/lancehalvorsen/phoenix-guides/blob/master/DD_Plug.md
http://www.phoenixframework.org/v0.11.0/docs/understanding-plug

Are you suggesting we just add a bit suggesting folks remember to look in the Plug documentation as well if they can't find what they are looking for in the Phoenix docs? Again, if I'm getting that wrong, please open an issue or submit a PR to the guides repo.

Thanks!

@SteveAquino
Copy link
Author

@lancehalvorsen Yes, I think an example of using send_resp/3 would be great, specifically the case of sending an empty response. To answer your final question, and to @2kodes point, maybe a sentence/link at the beginning of the controller docs would help point folks in the right direction.

@lancehalvorsen
Copy link
Member

Ok, I created 2 issues in the guides repo (as we can see above).

@chrismccord
Copy link
Member

Thanks @lancehalvorsen. I think we can shelve this and focus on handling it on the guides side. send_resp is somewhat obscure today doc wise. @SteveAquino thanks for the effort. We can revisit later if it becomes needed.

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

7 participants