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

Rails 4 upgrade notes about clashing named route selection changes #9690

Merged
merged 1 commit into from Mar 13, 2013
Merged

Rails 4 upgrade notes about clashing named route selection changes #9690

merged 1 commit into from Mar 13, 2013

Conversation

trevorturk
Copy link
Contributor

Document change to clashing named route selection. Rails 4 correctly prefers the first named route. (Likely from rails/journey@98a9802)

@steveklabnik
Copy link
Member

/cc @fxn

@carlosantoniodasilva
Copy link
Member

👍

@trevorturk
Copy link
Contributor Author

I'd like to log a warning when a route is skipped here:

https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/journey/routes.rb#L61

I'm not sure how best to do that, though -- @steveklabnik or @carlosantoniodasilva have any ideas?

@carlosantoniodasilva
Copy link
Member

@trevorturk this is what comes up to my mind right now:

if name
  if named_routes[name]
    ActiveSupport::Deprecation.warn("BOOM")
  else
    named_routes[name] = route
  end
end

It could be moved to a private method like add_named_route or something like that if necessary, to be called in that place, wdyt?

@trevorturk
Copy link
Contributor Author

Yeah, I was thinking using ActiveSupport::Deprecation would be frowned upon, but it'd work!

Want to work something up, or shall I?

@carlosantoniodasilva
Copy link
Member

Hm right, I guess we should not use AS::Deprecation, it's probably better to use ActionController::Base.logger.warn right?

@trevorturk
Copy link
Contributor Author

@carlosantoniodasilva took a stab at it in #9704 -- worked nicely in my local testing. If we go ahead with that, I'd like to update this PR to mention the logging -- much easier than trying to read through the rake routes output.

@schneems
Copy link
Member

👍 on this, any other concerns we need to address before merging in this doc addition?

@trevorturk
Copy link
Contributor Author

IMO it's good to go -- I made a separate PR #9704 where we can discuss logging or any other helpful stuff we might consider doing. If we go ahead with that, I'll update this section as well.

rafaelfranca added a commit that referenced this pull request Mar 13, 2013
Rails 4 upgrade notes about clashing named route selection changes
@rafaelfranca rafaelfranca merged commit a0920bc into rails:master Mar 13, 2013
@rafaelfranca
Copy link
Member

Thank you

@carlosantoniodasilva
Copy link
Member

❤️

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

Successfully merging this pull request may close these issues.

None yet

5 participants