Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Correct the use of to_model in polymorphic routing #6588

Merged
merged 1 commit into from

2 participants

@nbibler

This pull is against 3-2-stable, not master. If accepted, I'll quickly put together a pull for master, as well.

Polymorphic routes currently only use arg.to_model when generating the path directory or name portion of a route. They do not use the to_param value from the delegated model for the generated route IDs (since it currently passes the arguments directly to the named route functions without converting them to models).

Since to_model is supposed to act as the ActiveModel stand-in for an object, and the polymorphic routing mechanism already takes to_model in to account for a small bit of it, I think this is a bug in the router to not fully follow through with its usage. It also significantly limits the usefulness of creating to_model stand-ins.

So, this pull has a test to exploit the bug where I exercise that the delegate is not currently being used for to_param. And, then the fix simply passes the args collection after having converted it to models.

@josevalim josevalim merged commit 5b6b0df into rails:3-2-stable
@josevalim
Owner

Already ported to master, thanks!

@nbibler

Wow, great, thanks! I went off to port it to master and by the time I came back to make the pull request you'd already done it. Nice job. :+1:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
8 actionpack/lib/action_dispatch/routing/polymorphic_routes.rb
@@ -97,7 +97,7 @@ def polymorphic_url(record_or_hash_or_array, options = {})
end
record = extract_record(record_or_hash_or_array)
- record = record.to_model if record.respond_to?(:to_model)
+ record = convert_to_model(record)
args = Array === record_or_hash_or_array ?
record_or_hash_or_array.dup :
@@ -124,6 +124,8 @@ def polymorphic_url(record_or_hash_or_array, options = {})
args.last.kind_of?(Hash) ? args.last.merge!(url_options) : args << url_options
end
+ args.collect! { |a| convert_to_model(a) }
+
(proxy || self).send(named_route, *args)
end
@@ -154,6 +156,10 @@ def action_prefix(options)
options[:action] ? "#{options[:action]}_" : ''
end
+ def convert_to_model(object)
+ object.respond_to?(:to_model) ? object.to_model : object
+ end
+
def routing_type(options)
options[:routing_type] || :url
end
View
28 actionpack/test/activerecord/polymorphic_routes_test.rb
@@ -25,6 +25,24 @@ class Series < ActiveRecord::Base
self.table_name = 'projects'
end
+class ModelDelegator < ActiveRecord::Base
+ self.table_name = 'projects'
+
+ def to_model
+ ModelDelegate.new
+ end
+end
+
+class ModelDelegate
+ def self.model_name
+ ActiveModel::Name.new(self)
+ end
+
+ def to_param
+ 'overridden'
+ end
+end
+
module Blog
class Post < ActiveRecord::Base
self.table_name = 'projects'
@@ -50,6 +68,7 @@ def setup
@bid = Bid.new
@tax = Tax.new
@fax = Fax.new
+ @delegator = ModelDelegator.new
@series = Series.new
@blog_post = Blog::Post.new
@blog_blog = Blog::Blog.new
@@ -439,6 +458,13 @@ def test_uncountable_resource
end
end
+ def test_routing_a_to_model_delegate
+ with_test_routes do
+ @delegator.save
+ assert_equal "http://example.com/model_delegates/overridden", polymorphic_url(@delegator)
+ end
+ end
+
def with_namespaced_routes(name)
with_routing do |set|
set.draw do
@@ -469,6 +495,7 @@ def with_test_routes(options = {})
resource :bid
end
resources :series
+ resources :model_delegates
end
self.class.send(:include, @routes.url_helpers)
@@ -516,5 +543,4 @@ def with_admin_and_site_test_routes(options = {})
yield
end
end
-
end
Something went wrong with that request. Please try again.