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

Add HTTP verb to VerifiedRoutes #5805

Closed

Conversation

Gladear
Copy link

@Gladear Gladear commented May 8, 2024

This PR is a follow-up of the proposal on the Elixir Forum: Proposal for Phoenix: add HTTP verb to VerifiedRoutes.

tldr; I propose to add a way for VerifiedRoutes to know if they’re used for get, post, etc to make them even more robust, and safer to refactor. This solution could be to add a “tag” to verified routes, like so: ~p"/user/#{user}"get. More context and details can be found on the original post.

Please tell me what you think about this proposition. The code in the PR is functional, although I'd rather have more people re-read the test cases carefully to ensure the behaviour is the one expected. Also, I didn't update the documentation yet.
Here's the behaviour I expected to introduce (this is a copy-paste of the "add tests explaining behaviour" commit description):

Add tests to VerifiedRoutes ensuring the correctness
of the introduced behaviour.
We want to ensure the following is respected:

  • runtime behaviour isn't altered at all: a ~p
    with http verb modifier emits the path as a
    string,
  • existing warning behaviour isn't altered:
    a ~p - with or without an http verb - not
    matching a route emits a warning,
  • (new) a ~p matching an existing route without
    a matching http verb emits a warning too,
  • (new) a ~p matching an existing route with
    a matching http verb does not emit a warning.

PS: I know the proposal wasn't validated or anything, but I figured this wouldn't take me too long to make a POC anyway, so I don't mind reworking the solution if this one isn't the one we ought to keep for any reason.

BREAKING CHANGE

Before, `url(~p"/path"foo)` was considered valid,
whilst `~p"/path"foo` prevented compilation.

After, `url(...)` has the same behaviour as
a lone `~p`.
Add tests to VerifiedRoutes ensuring the correctness
of the introduced behaviour.
We want to ensure the following is respected:
* runtime behaviour isn't altered at all: a ~p
  with http method modifier emits the path as a
  string,
* warning behaviour isn't altered: a ~p - with or
  without an http verb - not matching a route
  emits a warning,
* (new) a ~p matching an existing route without
  a matching http method emits a warning too,
* (new) a ~p matching an existing route with
  a matching http method does not emit a warning.
@Gladear Gladear force-pushed the add-method-to-verified-routes branch from c38f14b to d5048e9 Compare May 12, 2024 11:53
@Gladear
Copy link
Author

Gladear commented May 12, 2024

As proposed on the forum, I updated to PR to use a "prefix" syntax : ~p"get /posts/1".
The "flag syntax" is still in the commit history, but I'll remove the commits if the new syntax is kept

@chrismccord
Copy link
Member

Thanks for this, but I am not convinced it's what we want. There are many cases where use of a route is determined at runtime, such as a form's HTTP verb based on a new changest for record insert or existing changeset for a record update. Ultimately you should have a test hitting these verbs so I'm not convinced the value is worth complexity and extended usage. Thanks!

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

2 participants