Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Deprecate `*_path` methods in mailers

Email does not support relative links since there is no implicit host. Therefore all links inside of emails must be fully qualified URLs. All path helpers are now deprecated. When removed, the error will give early indication to developers to use `*_url` methods instead.

Currently if a developer uses a `*_path` helper, their tests and `mail_view` will not catch the mistake. The only way to see the error is by sending emails in production. Preventing sending out emails with non-working path's is the desired end goal of this PR.

Currently path helpers are mixed-in to controllers (the ActionMailer::Base acts as a controller). All `*_url` and `*_path` helpers are made available through the same module. This PR separates this behavior into two modules so we can extend the `*_path` methods to add a Deprecation to them. Once deprecated we can use this same area to raise a NoMethodError and add an informative message directing the developer to use `*_url` instead.

The module with warnings is only mixed in when a controller returns false from the newly added `supports_relative_path?`.

Paired @sgrif & @schneems
  • Loading branch information...
commit 2bbcca004cc232cef868cd0e301f274ce5638df0 1 parent 4efb36e
Sean Griffin sgrif authored schneems committed
6 actionmailer/CHANGELOG.md
View
@@ -1,3 +1,9 @@
+* Deprecate `*_path` helpers in email views. When used they generate
+ non-working links and are not the intention of most developers. Instead
+ we recommend to use `*_url` helper.
+
+ *Richard Schneeman*
+
* Raise an exception when attachments are added after `mail` was called.
This is a safeguard to prevent invalid emails.
5 actionmailer/lib/action_mailer/base.rb
View
@@ -897,6 +897,11 @@ def insert_part(container, response, charset) #:nodoc:
container.add_part(part)
end
+ # Emails do not support relative path links.
+ def self.supports_path?
+ false
+ end
+
ActiveSupport.run_load_hooks(:action_mailer, self)
end
end
2  actionmailer/lib/action_mailer/railtie.rb
View
@@ -30,7 +30,7 @@ class Railtie < Rails::Railtie # :nodoc:
ActiveSupport.on_load(:action_mailer) do
include AbstractController::UrlFor
- extend ::AbstractController::Railties::RoutesHelpers.with(app.routes)
+ extend ::AbstractController::Railties::RoutesHelpers.with(app.routes, false)
include app.routes.mounted_helpers
register_interceptors(options.delete(:interceptors))
8 actionpack/lib/abstract_controller/base.rb
View
@@ -164,6 +164,14 @@ def available_action?(action_name)
_find_action_name(action_name).present?
end
+ # Returns true if the given controller is capable of rendering
+ # a path. A subclass of +AbstractController::Base+
+ # may return false. An Email controller for example does not
+ # support paths, only full URLs.
+ def self.supports_path?
+ true
+ end
+
private
# Returns true if the name can be considered an action because
6 actionpack/lib/abstract_controller/railties/routes_helpers.rb
View
@@ -1,14 +1,14 @@
module AbstractController
module Railties
module RoutesHelpers
- def self.with(routes)
+ def self.with(routes, include_path_helpers = true)
Module.new do
define_method(:inherited) do |klass|
super(klass)
if namespace = klass.parents.detect { |m| m.respond_to?(:railtie_routes_url_helpers) }
- klass.send(:include, namespace.railtie_routes_url_helpers)
+ klass.send(:include, namespace.railtie_routes_url_helpers(include_path_helpers))
else
- klass.send(:include, routes.url_helpers)
+ klass.send(:include, routes.url_helpers(include_path_helpers))
end
end
end
120 actionpack/lib/action_dispatch/routing/route_set.rb
View
@@ -86,12 +86,13 @@ def merge_default_action!(params)
# named routes.
class NamedRouteCollection #:nodoc:
include Enumerable
- attr_reader :routes, :helpers, :module
+ attr_reader :routes, :helpers, :url_helpers_module
def initialize
@routes = {}
@helpers = Set.new
- @module = Module.new
+ @url_helpers_module = Module.new
+ @path_helpers_module = Module.new
end
def route_defined?(name)
@@ -104,7 +105,11 @@ def helper_names
def clear!
@helpers.each do |helper|
- @module.send :undef_method, helper
+ if helper =~ /_path$/
+ @path_helpers_module.send :undef_method, helper
+ else
+ @url_helpers_module.send :undef_method, helper
+ end
end
@routes.clear
@@ -114,10 +119,12 @@ def clear!
def add(name, route)
key = name.to_sym
if routes.key? key
- undef_named_route_methods @module, name
+ @path_helpers_module.send :undef_method, :"#{name}_path"
+ @url_helpers_module.send :undef_method, :"#{name}_url"
end
routes[key] = route
- define_named_route_methods(@module, name, route)
+ define_url_helper @path_helpers_module, route, :"#{name}_path", route.defaults, name, PATH
+ define_url_helper @url_helpers_module, route, :"#{name}_url", route.defaults, name, FULL
end
def get(name)
@@ -141,6 +148,26 @@ def length
routes.length
end
+ def path_helpers_module(warn = false)
+ if warn
+ mod = @path_helpers_module
+ Module.new do
+ include mod
+
+ mod.instance_methods(false).each do |meth|
+ define_method("#{meth}_with_warning") do |*args, &block|
+ ActiveSupport::Deprecation.warn("The method `#{meth}` cannot be used here as a full URL is required. Use `#{meth.to_s.sub(/_path$/, '_url')}` instead")
+ send("#{meth}_without_warning", *args, &block)
+ end
+
+ alias_method_chain meth, :warning
+ end
+ end
+ else
+ @path_helpers_module
+ end
+ end
+
class UrlHelper # :nodoc:
def self.create(route, options, route_name, url_strategy)
if optimize_helper?(route)
@@ -263,7 +290,7 @@ def handle_positional_args(controller_options, inner_options, args, result, path
#
def define_url_helper(mod, route, name, opts, route_key, url_strategy)
helper = UrlHelper.create(route, opts, route_key, url_strategy)
-
+ mod.remove_possible_method name
mod.module_eval do
define_method(name) do |*args|
options = nil
@@ -274,16 +301,6 @@ def define_url_helper(mod, route, name, opts, route_key, url_strategy)
helpers << name
end
-
- def define_named_route_methods(mod, name, route)
- define_url_helper mod, route, :"#{name}_path", route.defaults, name, PATH
- define_url_helper mod, route, :"#{name}_url", route.defaults, name, FULL
- end
-
- def undef_named_route_methods(mod, name)
- mod.send :undef_method, :"#{name}_path"
- mod.send :undef_method, :"#{name}_url"
- end
end
# :stopdoc:
@@ -396,44 +413,51 @@ def #{name}
RUBY
end
- def url_helpers
- @url_helpers ||= begin
- routes = self
-
- Module.new do
- extend ActiveSupport::Concern
- include UrlFor
-
- # Define url_for in the singleton level so one can do:
- # Rails.application.routes.url_helpers.url_for(args)
- @_routes = routes
- class << self
- delegate :url_for, :optimize_routes_generation?, :to => '@_routes'
- attr_reader :_routes
- def url_options; {}; end
- end
+ def url_helpers(include_path_helpers = true)
+ routes = self
- route_methods = routes.named_routes.module
+ Module.new do
+ extend ActiveSupport::Concern
+ include UrlFor
+
+ # Define url_for in the singleton level so one can do:
+ # Rails.application.routes.url_helpers.url_for(args)
+ @_routes = routes
+ class << self
+ delegate :url_for, :optimize_routes_generation?, to: '@_routes'
+ attr_reader :_routes
+ def url_options; {}; end
+ end
- # Make named_routes available in the module singleton
- # as well, so one can do:
- # Rails.application.routes.url_helpers.posts_path
- extend route_methods
+ route_methods = routes.named_routes.url_helpers_module
- # Any class that includes this module will get all
- # named routes...
- include route_methods
+ # Make named_routes available in the module singleton
+ # as well, so one can do:
+ # Rails.application.routes.url_helpers.posts_path
+ extend route_methods
- # plus a singleton class method called _routes ...
- included do
- singleton_class.send(:redefine_method, :_routes) { routes }
- end
+ # Any class that includes this module will get all
+ # named routes...
+ include route_methods
- # And an instance method _routes. Note that
- # UrlFor (included in this module) add extra
- # conveniences for working with @_routes.
- define_method(:_routes) { @_routes || routes }
+ if include_path_helpers
+ path_helpers = routes.named_routes.path_helpers_module
+ else
+ path_helpers = routes.named_routes.path_helpers_module(true)
end
+
+ include path_helpers
+ extend path_helpers
+
+ # plus a singleton class method called _routes ...
+ included do
+ singleton_class.send(:redefine_method, :_routes) { routes }
+ end
+
+ # And an instance method _routes. Note that
+ # UrlFor (included in this module) add extra
+ # conveniences for working with @_routes.
+ define_method(:_routes) { @_routes || routes }
end
end
14 actionpack/test/routing/helper_test.rb
View
@@ -26,6 +26,20 @@ def test_exception
x.new.pond_duck_path Duck.new
end
end
+
+ def test_path_deprecation
+ rs = ::ActionDispatch::Routing::RouteSet.new
+ rs.draw do
+ resources :ducks
+ end
+
+ x = Class.new {
+ include rs.url_helpers(false)
+ }
+ assert_deprecated do
+ assert_equal '/ducks', x.new.ducks_path
+ end
+ end
end
end
end
5 actionview/lib/action_view/rendering.rb
View
@@ -35,12 +35,13 @@ def process(*) #:nodoc:
module ClassMethods
def view_context_class
@view_context_class ||= begin
- routes = respond_to?(:_routes) && _routes
+ include_path_helpers = supports_path?
+ routes = respond_to?(:_routes) && _routes
helpers = respond_to?(:_helpers) && _helpers
Class.new(ActionView::Base) do
if routes
- include routes.url_helpers
+ include routes.url_helpers(include_path_helpers)
include routes.mounted_helpers
end
16 guides/source/action_mailer_basics.md
View
@@ -414,6 +414,22 @@ globally in `config/application.rb`:
config.action_mailer.default_url_options = { host: 'example.com' }
```
+Because of this behavior you cannot use any of the `*_path` helpers inside of
+an email. Instead you will need to use the associated `*_url` helper. For example
+instead of using
+
+```
+<%= link_to 'welcome', welcome_path %>
+```
+
+You will need to use:
+
+```
+<%= link_to 'welcome', welcome_url %>
+```
+
+By using the full URL, your links will now work in your emails.
+
#### generating URLs with `url_for`
You need to pass the `only_path: false` option when using `url_for`. This will
2  railties/lib/rails/engine.rb
View
@@ -395,7 +395,7 @@ def isolate_namespace(mod)
end
unless mod.respond_to?(:railtie_routes_url_helpers)
- define_method(:railtie_routes_url_helpers) { railtie.routes.url_helpers }
+ define_method(:railtie_routes_url_helpers) {|include_path_helpers = true| railtie.routes.url_helpers(include_path_helpers) }
end
end
end
4 railties/test/application/initializers/frameworks_test.rb
View
@@ -50,7 +50,7 @@ def teardown
assert_equal "test.rails", ActionMailer::Base.default_url_options[:host]
end
- test "does not include url helpers as action methods" do
+ test "includes url helpers as action methods" do
app_file "config/routes.rb", <<-RUBY
Rails.application.routes.draw do
get "/foo", :to => lambda { |env| [200, {}, []] }, :as => :foo
@@ -66,8 +66,8 @@ def notify
require "#{app_path}/config/environment"
assert Foo.method_defined?(:foo_path)
+ assert Foo.method_defined?(:foo_url)
assert Foo.method_defined?(:main_app)
- assert_equal Set.new(["notify"]), Foo.action_methods
end
test "allows to not load all helpers for controllers" do
52 railties/test/application/mailer_previews_test.rb
View
@@ -417,6 +417,58 @@ def foo
assert_match '<option selected value="?part=text%2Fplain">View as plain-text email</option>', last_response.body
end
+ test "*_path helpers emit a deprecation" do
+
+ app_file "config/routes.rb", <<-RUBY
+ Rails.application.routes.draw do
+ get 'foo', to: 'foo#index'
+ end
+ RUBY
+
+ mailer 'notifier', <<-RUBY
+ class Notifier < ActionMailer::Base
+ default from: "from@example.com"
+
+ def path_in_view
+ mail to: "to@example.org"
+ end
+
+ def path_in_mailer
+ @url = foo_path
+ mail to: "to@example.org"
+ end
+ end
+ RUBY
+
+ html_template 'notifier/path_in_view', "<%= link_to 'foo', foo_path %>"
+
+ mailer_preview 'notifier', <<-RUBY
+ class NotifierPreview < ActionMailer::Preview
+ def path_in_view
+ Notifier.path_in_view
+ end
+
+ def path_in_mailer
+ Notifier.path_in_mailer
+ end
+ end
+ RUBY
+
+ app('development')
+
+ assert_deprecated do
+ get "/rails/mailers/notifier/path_in_view.html"
+ assert_equal 200, last_response.status
+ end
+
+ html_template 'notifier/path_in_mailer', "No ERB in here"
+
+ assert_deprecated do
+ get "/rails/mailers/notifier/path_in_mailer.html"
+ assert_equal 200, last_response.status
+ end
+ end
+
private
def build_app
super
Please sign in to comment.
Something went wrong with that request. Please try again.