Rails 4.2 compatibility

[lucasmazza]

This Pull Request aims to bring back compatibility with Rails 4.2/master since rails/rails@dc3f25c has changed the @scope data structure from the router, without losing compatibility with existing versions of Rails where this object is a Hash.

In the matter of Rails 4.2, we also have this deprecation warning:

DEPRECATION WARNING: defining a route where `to` is a controller without an action is deprecated.  Please change "to: :users/omniauth_callbacks" to "controller: :users/omniauth_callbacks". (called from devise_omniauth_callback at /Users/lucas/code/ptec/devise/lib/devise/rails/routes.rb:436)

Changing the router code to use controller: controllers[:omniauth_callbacks] leads to the following error:

/opt/rubies/2.1.2/lib/ruby/gems/2.1.0/bundler/gems/rails-d5be08347fb7/actionpack/lib/action_dispatch/routing/mapper.rb:263:in `block (2 levels) in check_controller_and_action': 'users/auth/:action' is not a supported controller name. This can lead to potential routing problems. See http://guides.rubyonrails.org/routing.html#specifying-a-controller-to-use (ArgumentError)

• Fix router monkeypatch.
• Remove OmniAuth route warning.
• Deal with respond_with removal.
• Reorganize the project Gemfiles.
• 

[carlosantoniodasilva]

Will you still need this constant, then?

[lucasmazza]

🏃

[carlosantoniodasilva]

Seems good to me 👍

[Fudoshiki]

Merge this with master branch

[lucasmazza]

I would really like to remove the warning before merging this (@josevalim, any ideas?), and probably deal with #3157 before releasing a 4.2.0 compatible version.

Once we are good to go this will be merged, don't worry :).

[lucasmazza]

@carlosantoniodasilva @josevalim another detail that we need to address: Devise does a hard use of respond_with, so this means that we now depend on the responders. Should we have a more specialized error message or let rails raise the "The controller-level 'respond_to' feature has been extracted..." one?

[lucasmazza]

Updated the whole thing, testing against the released 4.2.0.beta1 gem. I've noticed several intermittent errors when running the test suite locally multiple times and we need to take a look on that.

[josevalim]

Those changes look great. Will Devise depend just on Rails 4.2 then? It kinda sucks that Rails is basically forcing us to do backwards incompatible changes.

I am also fine with depending on responders, at least for now, because most Rails devs will still expect this kind of stuff to just work.

[carlosantoniodasilva]

We could try to move on to respond_to block rather than respond_with in most cases, just not sure it's totally worth it anyway. The other changes doesn't seem to be totally back imcompatible, they should work the same way on Rails 4.x apparently, but I might be wrong. Looking good 👍

[josevalim]

Isn't it better to just replace to: by controller:? Would that get rid of the warning?

[lucasmazza]

Nope, it crashes the app with the second message I've posted on this pull request description.

[lucasmazza]

I don' think that rewriting the respond_with is a good path, and having a fixed dependency on responders will force us to drop support for Rails 4.0/4.1.

I'm thinking of going with an exception for now (something like 'Devise depends on the respond_to/respond_with API that was extracted to the responders gem. Please add gem "responders", "~> 2.0"' to your Gemfile') so we can support multiple versions with Devise 3.x and then look for a major release that drops this backwards support - something like Devise 4 is only for Rails 4.2+.

Does it sounds too complicated?

[trinode]

@lucasmazza this sounds good to me, I've been limited before by gem dependencies of libraries, when a perfectly good API compatible alternative exists (eg cancan dependency and I wanted to use cancancan), this approach will allow us to use super_awesome_mega_reponders.gem (which I made up) if it provided all the necessary functions I presume?

[lucasmazza]

@trinode sort of. Once we add responders as a fixed dependency on Devise this won't be so easy, but I don't know if there any responders alternative out there. The main goal is to keep compatibility between Devise and multiple Rails versions.

[trinode]

@lucasmazza If it achieves the goal of older version compatibility it can only be a good thing, and like mentioned before, dropping 4.1 and below (or perhaps anything below 5 if there are features for Devise to take advantage of in Rails 5) from the next major version. Perhaps update the docs to mention to include it since it doesn't appear to be a default include in a new app, so long as it's clear in the exception output I don't think it'd cause too much friction.

[rafaelfranca]

I'd go with the path of avoiding using responders, unless we have behaviour coupled in the gem.

[lucasmazza]

@rafaelfranca I think we are too coupled to drop it right now.

[rafaelfranca]

I see. Can't we just say we depends on responders without specifying any version and let bundler resolve it correctly to different Rails versions? responders 2.0 requires 4.2 so bundler will never let 1.x be installed. Same when you are using 4.1.

[lucasmazza]

👍 for that If we are ok with shipping responders for free for everyone 😄

[rafaelfranca]

Yes, there is no problem on shipping responders for free. It doesn't inject code automatically anyway.

[lucasmazza]

So I'll cut a 3.3.1 release for #3157 and 3.4.0.beta for Rails 4.2, and look through the test failures later.

[zenspider]

Fan-fucking-tastic!

[deepj]

I've been trying to migrate an application (which I haven't developed before) to Rails 4.2 (mainly due to ActiveJob) and I'm using this branch due to incompatibility with Rails 4.2 instead of Devise 3.3.0 (where everything works fine) . Unfortunately, I haven't worked with Devise for a while (I was stuck in 2.x). So I'm not sure how to deal with this issue.

I'm getting the following issue when I run it in production environment (no problem in development or test environment).

 app/controllers/application_controller.rb:6:in `alias_method': undefined method `current_api_user' for class `ApplicationController' (NameError)

My app/controllers/application_controller.rb looks something like

class ApplicationController < ActionController::Base
  # Prevent CSRF attacks by raising an exception.
  # For APIs, you may want to use :null_session instead.
  protect_from_forgery with: :exception

  alias_method :current_user, :current_api_user
  alias_method :user_signed_in?, :api_user_signed_in?
end

In config/routes.rb

namespace :api, defaults: { format: :json } do
  devise_for :users, skip: :registrations, controllers: { sessions: 'api/auth', omniauth_callbacks: 'api/omniauth_callbacks' }

  devise_scope :api_user do
    post :logout, to: 'auth#destroy', as: :destroy_user_session
    post :login,  to: 'auth#create',  as: :create_user_session

    get 'auth/facebook/callback', to: 'api/omniauth_callbacks#facebook'
    get 'auth/google/callback',   to: 'api/omniauth_callbacks#google'
  end
end

I'm not sure if the customized Devise configuration is used properly. The same problem I'm getting with Rails 4.1.6 when I use this branch. There is no problem with Devise 3.3.0. I use Ruby 2.1.3.

[zenspider]

@deepj please don't piggy back on other issues. File a new issue.

[deepj]

@zenspider I don't think that belongs to somewhere else. I just describe my issues and experiences related to this pull request.

[lucasmazza]

@deepj can you replicate this in a fresh app and push it to GitHub? Then I take a look and check what is really going wrong with this branch.

[deepj]

@lucasmazza Here is the repository for replication of the problem

https://github.com/deepj/devise-rails42

Just run it like bundle exec rails s -e production

[lucasmazza]

@deepj thanks! I took a look on your app and seems to me that, when the Rails config eager_load is true, the application will load the ApplicationController before loading the routes file, so in that moment the Devise helpers won't be available. I'll recheck this loading order in different Rails versions.

[lucasmazza]

Dug some more, the culprit is #3195, not the 4.2 compatibility code.

[lucasmazza]

@deepj can you test this branch one more time? ❤️ 💚 💙 💛 💜

[deepj]

@lucasmazza It seems everything is green! 👍

[lucasmazza]

Awesome!

[rubytastic]

Having an initialiser for splitting up routes like so:

class ActionDispatch::Routing::Mapper
def draw(routes_name)
instance_eval(File.read(Rails.root.join("config/routes#{@scope[:path]}", "#{routes_name}.rb")))
end
end

Still breaks rails 4.2beta2

[mmahalwy]

undefined local variable or method `mimes_for_respond_to' for DeviseController:Class

Still happening in 4.2.0.rc1

[promiseverma]

replace
gem 'devise'
to
gem 'devise', :git => 'https://github.com/plataformatec/devise.git'

[joshdance]

I was still having errors. This SO answer fixed it for me - http://stackoverflow.com/questions/27611947/devise-raises-error-with-rails-4-2-upgrade

[amritdeep]

Use devise 3.4.1.