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

Try to catch controller action match errors and 400 #1225

Closed
chrismccord opened this issue Sep 19, 2015 · 4 comments
Closed

Try to catch controller action match errors and 400 #1225

chrismccord opened this issue Sep 19, 2015 · 4 comments

Comments

@chrismccord
Copy link
Member

No description provided.

@stevegraham
Copy link
Contributor

👍

@chrismccord
Copy link
Member Author

My new plan to solve this is to rely on action/3 clauses instead of mucking with stacktraces, for example this works today, and is clearer than our current approach:

defmodule MyController do

  def action(conn, _) do
    apply(__MODULE__, :action, [action_name(conn), conn, conn.params])
  end

  def action(:show, conn, params) do
    ...
  end

  def action(:index, conn, params) do
     ...
  end

  def action(_, conn, params), do: send_resp(conn, :bad_request, "")
end

Whatever we do here must be backwards compatible now that 1.0 has shipped, so this would have to be further off as a default if we went this direction. We could release a Phoenix.ActionController that injects this behavior and leave Phoenix.Controller untouched, or if we're feeling adventurous, we could detect if any :action clauses have been defined and use the new behavior. The latter is likely too much though. Thoughts?

@josevalim
Copy link
Member

Nah, I am 👎. We already have one way of doing things, we shouldn't have two unless we have very good reasons. This can either be a separate package or people can easily implement it in their apps.

@chrismccord
Copy link
Member Author

@josevalim good points. I'm definitely on board with the one way to do things, especially for learning material that's out there. I'll shelve this idea for now, with the flip side that folks can use that snippet exactly as shown today if they really want catch-all handling.

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

No branches or pull requests

3 participants