Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

UrlHelpers should generate path/url methods only when resource not skipped in devise_for #1288

Closed
dmitry opened this Issue Aug 27, 2011 · 10 comments

Comments

Projects
None yet
3 participants

dmitry commented Aug 27, 2011

For example, I want to use my own registrations controller with resource :registration in routes. In that case I will catch an exception with "ArgumentError: wrong number of arguments (0 for 1)" when I try to use registration_path anywhere in the controller. That's because path's where generated in the UrlHelpers devise module (and only on controller scope).

It will be nice, if generation of the device's resource paths can depend on :skip options from the devise_for method (from the routing).

https://github.com/plataformatec/devise/blob/master/lib/devise/controllers/url_helpers.rb

If you have any suggestions, how I can get around it, let me know please.

Thanks!

Owner

josevalim commented Aug 27, 2011

Please do provide a pull request! You can store the :skip options in the Devise::Mapping object (if it isn't stored yet) and skip it as appropriate in the url_helpers.rb file you linked. If you have any questions, I'd be glad to help.

dmitry commented Aug 27, 2011

Looked and tried to fix it...

UrlHelpers loads before the devise_for execution, so it's impossible to detect is there are any resource skipped or no.

Any suggestions where to dig?

Owner

josevalim commented Aug 27, 2011

Hrm, I had forgotten that (Rails 3 initialization process is a bit chaotic).

One option is to remove the defined methods at some point. For example,
before we include the UrlHelpers to controller, we could check all mappings
and see if all of them skip the same configuration. For instance, if all
mappings skip registerable, we could safely unregister the method. The
unregistering could be done here:

https://github.com/plataformatec/devise/blob/master/lib/devise/rails.rb#L17

Do you think this could work?

@josevalim josevalim closed this in f21d05a Aug 29, 2011

dmitry commented Aug 29, 2011

Thanks!

This has broken my Rails app... I understand why this was changed, but it's not intuitive with how the README / standard config. I have various :skips along with specifying the controllers directly as well, and now I don't have the routes that point to those controllers. As an example "new_session_path" doesn't exist anymore. Thoughts?

My route.rb:

devise_for(:users, :skip => [ :sessions, :registrations, :confirmations, :passwords, :omniauth_callbacks ],
  :controllers => {
     :sessions => 'users/sessions',
     :registrations => 'users/registrations',
     :confirmations => 'users/confirmations',
     :passwords => 'users/passwords',
     :omniauth_callbacks => 'users/omniauth_callbacks'
  }) do
Owner

josevalim commented Aug 31, 2011

@Brady8, there is already an issue open for this. But if you are passing the controllers, you don't need to pass skip.

Wow you're fast Jose, thanks! I'll give it a go, much appreciated.

Owner

josevalim commented Aug 31, 2011

@Brady8 I have fixed this in Devise master as well, could you please test and confirm it works? The original issue is #1304

@josevalim Fixed on master for me, although I've confirmed that I do indeed need the :skip alongside :controller, otherwise my route:

get '/users/:username', :to => 'users/registrations#show', :as => :user_registration

overrides the sessions#new route and I get:

Processing by Users::RegistrationsController#show as HTML
Parameters: {"username"=>"sign_in"}

when I click follow a link such as:

<%= link_to('sign in', new_session_path(:user)) %>

which is obviously not what I want :) I think that's particular to my app, and a separate issue, but just thought I'd let you know in case it's relevant.

My routes.rb:

devise_for(:users, :skip => [ :sessions, :registrations, :confirmations, :passwords, :omniauth_callbacks ],
  #devise_for(:users,
  :controllers => {
     :sessions => 'users/sessions',
     :registrations => 'users/registrations',
     :confirmations => 'users/confirmations',
     :passwords => 'users/passwords',
     :omniauth_callbacks => 'users/omniauth_callbacks'
  }) do
    delete '/users/:username', :to => 'users/registrations#destroy', :as => :user_registration
    put '/users/:username', :to => 'users/registrations#update', :as => :user_registration
    get '/signup', :to => 'users/registrations#new', :as => :new_user_registration
    post '/signup', :to => 'users/registrations#create', :as => :new_user_registration
    get '/users/:username/edit', :to => 'users/registrations#edit', :as => :edit_user_registration
    get '/users/:username', :to => 'users/registrations#show', :as => :user_registration

    get '/signin', :to => 'users/sessions#new', :as => :new_user_session
    post '/signin', :to => 'users/sessions#create', :as => :user_session
    get '/signout', :to => 'users/sessions#destroy', :as => :destroy_user_session

    get '/confirm', :to => 'users/confirmations#new', :as => :new_user_confirmation
    post '/confirm', :to => 'users/confirmations#create', :as => :user_confirmation
    get '/activate/:confirmation_token', :to => 'users/confirmations#show', :as => :user_activation

    get '/password', :to => 'users/passwords#new', :as => :new_user_password
    post '/password', :to => 'users/passwords#create', :as => :user_password
    put '/password', :to => 'users/passwords#update', :as => :user_password
    get '/password/reset/:reset_password_token', :to => 'users/passwords#edit', :as => :edit_user_password
  end
Owner

josevalim commented Aug 31, 2011

Ok, thanks for pinging back then. I will release a new version soon.

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