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

Namespaced resource route generates incorrect aliases #26

Closed
chrismccord opened this issue Feb 4, 2014 · 18 comments
Closed

Namespaced resource route generates incorrect aliases #26

chrismccord opened this issue Feb 4, 2014 · 18 comments
Assignees

Comments

@chrismccord
Copy link
Member

defmodule Router do
  use Phoenix.Router, port: 4000

  resources "admin/users", Phoenix.Controllers.Users do
    resources "comments", Phoenix.Controllers.Comments
  end
end

mix phoenix.routes Router

       admin/users  GET     admin/users                Users#index
   edit_admin/user  GET     admin/users/:id/edit       Users#edit
        admin/user  GET     admin/users/:id            Users#show
    new_admin/user  GET     admin/users/new            Users#new

We need to detect these scenarios and convert to underscores

@chrismccord
Copy link
Member Author

Actually, the above convention doesn't exist in rails and actually raises an error. Given this we can:

  1. Support arbitrary namespaces and use the last chunk as the resource name
  2. Only support a single "name" for resources and use another scope or namespace mechanism.

Would like to hear everyones thought's on this one. I believe @darkofabijan has previously suggested a scope option such as resources "users", Controllers.Users, scope: "admin"

@nurugger07
Copy link

We could use the following:

defmodule Router do
  use Phoenix.Router, port: 4000

  scope "admin" do
    resources "users", Phoenix.Controllers.Users do
      resources "comments", Phoenix.Controllers.Comments
    end
  end
end

Rails has this same syntax so it should feel the same. I don't mind having the option to inline :scope but I also think that having multiple resources within a scope is common enough to warrant grouping. Using the scope to group the resources makes it easier to discern that they are scoped together.

@nurugger07
Copy link

Also, the scope could be defined with any path like so:

defmodule Router do
  use Phoenix.Router, port: 4000

  scope "/admin/hidden" do
    resources "users", Phoenix.Controllers.Users do
      resources "comments", Phoenix.Controllers.Comments
    end
  end
end

@darkofabijan
Copy link
Contributor

@nurugger07 I would prefer that style too. In that case we could also s/scope/namespace/. Thinking just from end user perspective I wouldn't mind having DSL very similar to one Rails has. It's very clean and over the years many hours invested in making it powerful as it is now. Also I don't recall that any part of it felt unintuitive or that I ever felt that something could be done simpler. My idea was in the spirit of discussion to we initially had - to add all features that current router is missing without making routing macros too complicated and possibly hard to maintain.

Thinking that it's more important that routing DSL is clean and pleasant to use over being simple to maintain is valid I think. The question is just how should we approach development of this or any other component. There is a perfect solution for router, there is one good enough for now and there is one that can be done in two hours.

In the end purpose of these discussion is to find middle ground. I just felt it would be easier for me to explain my proposition by open-sourcing my thinking process. :)

@nurugger07
Copy link

@darkofabijan I go back and forth on how closely to build things in a similar manner to Rails. There are pros and cons, but it is a DSL people are familiar with. I do prefer scope over namespace. I think the word namespace should be retained for other uses by either Phoenix or Elixir itself.

I think the discussion is good as well :)

@chrismccord
Copy link
Member Author

@darkofabijan Go ahead on this one. Here's my thoughts on the proposed approaches. Feel free to try both out and see what feels right Mapper wise. My vote is for 1 since it accommodates most use-cases, but we have to implement it cleanly.

  1. The scope "admin" do .. end is nice, but will require an additional macro as well as nesting considerations. The bulk of the work though should be able to be pushed down to Router.Context. We will need to consider pushing/popping a more complex context vs the string approach right now since will have a couple states to keep track of (resources, scopes).
  2. Use @darkofabijan's original proposal and provided scope as a resources options:
resources "user", Users, scope: "private/admin" do
...
end

This approach doesn't not require additional macros, but will still involve Router.Context, perhaps to a lesser degree of refactoring. What this approach lacks is the ability to scope entire sections of non resource based router. So if you wanted to scope a series of gets/posts/puts, you'd have to repeat the scope, which is undesirable.

@josevalim
Copy link
Member

I don't like scope in that example because it doesn't tell me what is actually doing. Is it modifying a path? I think scope is suitable as a general thing.

scope path: "private/admin" do
  ...
end
scope alias: Admin do
  ...
end

I liked namespace in Rails 2 router because it meant scoping many things at once: the path, the alias and the named helpers.

@chrismccord
Copy link
Member Author

I like the idea of providing both a path and alias option as it combines a few active issues we've been discussing. Because we require explicit controllers right now, alias: would free up all the repetitive controller aliases or controller prefixes. I'd like to give @josevalim's suggestion a go.

@nurugger07
Copy link

+1 on @josevalim's proposal. I like the idea of being explicit with the intent.

@darkofabijan
Copy link
Contributor

@josevalim 👍 I agree about Rails router comments. That's what would people expect from scoping, and is probably the one that makes most sense. But it's the same in Rails 3 isn't it? https://gist.github.com/darkofabijan/8833685 path, controller and helpers are scoped.

I am not sure what would I gain by just scoping path or alias? In any of the Rails routing files that I am checking namespaces are used to scope all three, and it's good. Ok since we specify controller explicitly so that would not be an issue, but named helpers would be. E.g.

resources "users", Controllers.Users

scope path: "/admin", alias: Controllers.Admin do
   resources "users", Users
end

In the example above we would be still stuck with same named helpers. And since we are explicitly specifying controllers we should probably also explicitly specify helpers scoping.

scope path: "/admin", alias: Controllers.Admin, helper: "admin" do
   resources "users", Users
end

Do you guys see anything wrong with last example? As @josevalim stated it is really desired behaviour. Since we are not using implicit DSL I guess that being explicit about everything is I guess the price we have pay. :) It gives user much freedom, maybe unneeded.

P.S. I think I might have secret desire for implicit DSL :D

@chrismccord
Copy link
Member Author

Good point about not knowing what to make the named route helper functions. I agree helper: makes since in this case then. Explicit behavior is better than implicity trying to make a decision based on the path name, which could potentially have multiple segments. The nice thing about our explicit options is, once implemented, adding implicit behavior is just a matter of a Keyword.get/3 with default implicit values. For now making the implicit, explicit is the way to go.

@nurugger07
Copy link

I think I'm agreeing with @darkofabijan that the arguments should act more like overrides. If I define a named scope, it's easy to assume a convention that names path. alias, helper, and controller. In the adverse, if all the names match up, I don't want to have to explicitly name them.

@chrismccord
Copy link
Member Author

Once we have all the functionality in place for routing, I plan to open a RFC on convention over configuration. My main focus at the moment is two fold:

  1. Have all Router functionality in place, focusing on the internals. If we get caught up now on conventions before anyone has built out a couple prototypes, the dev time won't be productive.
  2. I want to make sure our conventions map well for a non-monolithic setup. Phoenix will be designed to run on multiple nodes, potentially with multiple umbrella apps, so we need to keep this context in mind when in comes to conventions.

Things are progressing very nicely with the router and we should be able to start playing with apps and seeing what feels right as far as conventions go, keeping 1 & 2 above in mind.

@nurugger07
Copy link

+1 @chrismccord I totally agree. It's best to have a solid routing tool first. The only argument I would have for implied behavior in this situation is to reduce redundancy in defining scopes.

@darkofabijan
Copy link
Contributor

I agree on all points and I am especially happy about the following:

Things are progressing very nicely with the router and we should be able to start playing with apps and seeing what feels right as far as conventions go, keeping 1 & 2 above in mind.

There is nothing better than using, even early prototype, in the wild. It makes things clear without much thinking, sets the roadmap and is also great motivator.

@chrismccord
Copy link
Member Author

@darkofabijan's PR has been merged and I'm very happy with the results. There is some redundancy of scope: 'admin', helper: 'admin', and both options are likely be the same in most cases. Soon I'll open up a RFC on conventions and we can discuss sane defaults, such as helper being based on the path.

@josevalim
Copy link
Member

Maybe you could support scope "admin", Adminas it is symmetric with resources?

@darkofabijan
Copy link
Contributor

@josevalim I like the symmetry that you are suggesting.

In resources example resources "users", Users, "users" implicitly define url path and path helper. Same would be the case with scope "admin", Admin. We can discuss it further in RFC that @chrismccord is mentioning.

Gazler pushed a commit to Gazler/phoenix that referenced this issue Sep 17, 2015
…ction-to-controller-guide

add redirection to controller guide, including redirect/2 and redirect/3
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

No branches or pull requests

4 participants