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

Implement custom url helpers and polymorphic mapping #23138

Merged
merged 16 commits into from Feb 21, 2017

Conversation

pixeltrix
Copy link
Contributor

@pixeltrix pixeltrix commented Jan 20, 2016

Implement the features outlined in #22512.

  • Add support for defining custom url helpers in routes.rb
  • Add support for defining custom polymorphic mappings in routes.rb

@pixeltrix
Copy link
Contributor Author

@rails/core pushing up a WIP for some feedback on method name and implementation.

def call(t, args, options, outer_options = {})
url_options = eval_block(t, args, options)

case url_options
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is not this duplicated somewhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's similar to the behaviour in ActionDispatch::Routing::UrlFor but not exactly the same, mainly because of the options being needed to be combined into the arguments for url_for.

#
# NOTE: It is the url helper's responsibility to return the correct
# set of options to be passed to the `url_for` call.
def url_helper(name, options = {}, &block)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not liking the word url_helper for this. It doesn't fit well with the rest of the metaphor we're using with drawing etc. What didn't you like about direct from the original pitch?

direct :homepage do
  "http://www.rubyonrails.org"
end

direct :commentable do |model|
  [ model, anchor: model.dom_id ]
end

direct :main do
  { controller: 'pages', action: 'index', subdomain: 'www' }
end

Looks and sounds great to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pixeltrix Did you have any reservations about using direct? Would love to get this merged before rc1. Also, we can use lookup for the link_to @model change.

@pixeltrix pixeltrix force-pushed the custom-url-helpers-and-polymorphic-urls branch from 3c0ff72 to 419c715 Compare February 23, 2016 09:54
@pixeltrix pixeltrix modified the milestones: 5.0.0 [temp], 5.0.0 Feb 23, 2016
@rafaelfranca rafaelfranca removed this from the 5.0.0 [temp] milestone Apr 5, 2016
@pixeltrix pixeltrix force-pushed the custom-url-helpers-and-polymorphic-urls branch 4 times, most recently from 6499cdd to 03890f3 Compare February 20, 2017 13:02
@pixeltrix pixeltrix changed the title [WIP] Implement custom url helpers and polymorphic mapping Implement custom url helpers and polymorphic mapping Feb 20, 2017
@pixeltrix pixeltrix added this to the 5.1.0 milestone Feb 20, 2017
#
# NOTE: The `direct` method doesn't observe the current scope in routes.rb
# and because of this it's recommended to define them outside of any blocks
# such as `namespace` or `scope`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we just make this raise?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably - I'm sure we can detect that

@@ -156,6 +164,11 @@ def polymorphic_path_for_action(action, record_or_hash, options)
polymorphic_path(record_or_hash, options.merge(action: action))
end

def polymorphic_mapping(record)
return false unless record.respond_to?(:to_model)
_routes.polymorphic_mappings[record.to_model.model_name.name]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider allowing non-models to map through by falling back on record.class.name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

url_name = :"#{helper}_url"

if @path_helpers_module.method_defined?(path_name)
@path_helpers_module.send :undef_method, path_name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this (and the existing call above) be using remove_method instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsure - need to check why we used undef_method in the first place

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was changed in 0088b08 - @tenderlove any reason undef_method was used instead of removed_method ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to assume that undef_method was used because remove_possible_method uses that internally for the reason outlined in 0244c0d

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the fact that we should allow people to access inherited methods if they include url helpers and then change a conflicting route name then probably best to switch to remove_method

def direct(name_or_hash, options = nil, &block)
case name_or_hash
when Hash
@set.add_polymorphic_mapping(name_or_hash, &block)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure options is nil, as we're ignoring it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And what if it isn't - raise or just merge?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say raise ArgumentError

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should always use a hash?, e.g:

direct(method: :rubyonrails) { "http://rubyonrails.org" }
direct(class: "Basket") { "/basket" }

Helps to clear up some naming confusion - @dhh wdyt?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a fan of that. Then I'd rather we use two separate method names. We could use direct for the URL declaration and resolve for polymorphics. So it's:

direct(:rubyonrails) { "http://rubyonrails.org" }
resolve("Basket") { "/basket" }


@path_helpers_module.module_eval do
define_method(:"#{name}_path") do |*args|
options = args.extract_options!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any benefit to doing this extraction here? Seems like that could be call's problem, where it already does so for nested arrays.

if url_options.permitted?
t.url_for(url_options.to_h.merge(outer_options))
else
raise ArgumentError, "Generating an URL from non sanitized request parameters is insecure!"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a URL

def call(t, args, options, outer_options = {})
url_options = eval_block(t, args, options)

case url_options
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we push this whole case back into t (that's the "real" route thingy, right?), by just adding a new url_for-like method that takes an explicit path_only parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't there a path_for we could use?


direct(class: "Basket") { |basket| [:basket] }
direct(class: "User", anchor: "details") { |user, options| [:profile, options] }
direct(class: "Video") { |video| [:media, { id: video.id }] }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given a Model#to_param, this would work as [:media, video], right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that would map to media_video_path(@video) which doesn't exist

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah... ouch.

So there's no way to really delegate to another route helper directly.

Could we make media_path(video) work, and have that magically behave like media_url or media_path depending on whether we're doing video_url or video_path, a bit like ActionMailer already does by forcing a full URL even when you ask for just the path?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we make media_path(video) work, and have that magically behave like media_url or media_path depending on whether we're doing video_url or video_path, a bit like ActionMailer already does by forcing a full URL even when you ask for just the path?

Possibly, url_for takes a strategy argument but we'd have to intercept the call somehow

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The easy option would seem to be to have two different objects we can do the instance_exec on.. I don't remember how AM achieves its version, though.

```

This generates the correct singular URL for the form instead of the default
resources member url, e.g. `/basket` vs. `/basket/:id`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we should come up with a different example here -- something that uses a parent route, perhaps -- both because it's desirable to show an argument-using block, and because I think resource should be registering the singular URL automatically (with some conflict resolution to avoid an incompatible change if you define both singular and plural resources for the same class).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, not entirely sure what you mean by this - can you illustrate with an example? Do you mean that calling resource :basket should automatically do direct(class: 'Basket') { [:basket] } behind the scenes?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean that calling resource :basket should automatically do direct(class: 'Basket') { [:basket] } behind the scenes?

Yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this would be a good idea - if we add it via resource then we'd need to add an option to configure/disable it and the thought of touching that code again makes me 😱

pixeltrix and others added 9 commits February 21, 2017 15:30
The singleton url_for on Rails.application.routes.url_helpers isn't the
same as the url_for you get when you include the module in your class as
the latter has support for polymorphic style routes, etc. whereas the
former accepts only a hash and is the underlying implementation defined
on ActionDispatch::Routing::RouteSet.

This commit changes the singleton method to call through a proxy instance
so that it gets the full range of features specified in the documentation
for url_for.
Allow the definition of custom url helpers that will be available
automatically wherever standard url helpers are available. The
current solution is to create helper methods in ApplicationHelper
or some other helper module and this isn't a great solution since
the url helper module can be called directly or included in another
class which doesn't include the normal helper modules.

Reference #22512.
Allow the use of `direct` to specify custom mappings for polymorphic_url, e.g:

  resource :basket
  direct(class: "Basket") { [:basket] }

This will then generate the following:

  >> link_to "Basket", @Basket
  => <a href="/basket">Basket</a>

More importantly it will generate the correct url when used with `form_for`.

Fixes #1769.
Using `undef_method` means that when a route is removed any other
implementations of that method in the ancestor chain are inaccessible
so instead use `remove_method` which restores access to the ancestor.
Use a separate method called `resolve` for the custom polymorphic
mapping to clarify the API.
@pixeltrix pixeltrix force-pushed the custom-url-helpers-and-polymorphic-urls branch from 8d5b711 to d7c1e62 Compare February 21, 2017 15:31
The column information for the testings table was being cached
so clear the cache in the test teardown.
@pixeltrix pixeltrix merged commit f3d729f into master Feb 21, 2017
@pixeltrix pixeltrix deleted the custom-url-helpers-and-polymorphic-urls branch February 21, 2017 20:00
``` ruby
resource :basket
direct(class: "Basket") { [:basket] }
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a typo, it says "Add the resolve" method, but then the example uses direct not resolve. Not sure if the example is valid as it is and is demonstrating direct, or if it's a typo and the example is demonstrating resolve. Trying to figure out these new features.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s corrected in master.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was almost fixed in master - it is now 😄

@sunnyrjuneja
Copy link

@pixeltrix Hey, great work on this PR. Was just reviewing the RC to see what's new in Rails 5.1. Personally, I found your direct example to be very easy to understand and illustrative. However, I wasn't able to totally grok why and when for resolve. Just some feedback. Thanks again for your hard work.

claudiob added a commit to claudiob/rails that referenced this pull request Apr 7, 2017
@matthewd matthewd mentioned this pull request Jan 22, 2018
@adammiribyan
Copy link

@sunnyrjuneja the tests contain a quite illustrative example for resolve.

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

Successfully merging this pull request may close these issues.

None yet

9 participants