diff options
| -rw-r--r-- | actionmailer/CHANGELOG.md | 6 | ||||
| -rw-r--r-- | actionmailer/lib/action_mailer/base.rb | 5 | ||||
| -rw-r--r-- | actionmailer/lib/action_mailer/railtie.rb | 2 | ||||
| -rw-r--r-- | actionpack/lib/abstract_controller/base.rb | 8 | ||||
| -rw-r--r-- | actionpack/lib/abstract_controller/railties/routes_helpers.rb | 6 | ||||
| -rw-r--r-- | actionpack/lib/action_dispatch/routing/route_set.rb | 120 | ||||
| -rw-r--r-- | actionpack/test/routing/helper_test.rb | 14 | ||||
| -rw-r--r-- | actionview/lib/action_view/rendering.rb | 5 | ||||
| -rw-r--r-- | guides/source/action_mailer_basics.md | 16 | ||||
| -rw-r--r-- | railties/lib/rails/engine.rb | 2 | ||||
| -rw-r--r-- | railties/test/application/initializers/frameworks_test.rb | 4 | ||||
| -rw-r--r-- | railties/test/application/mailer_previews_test.rb | 52 |
12 files changed, 183 insertions, 57 deletions
diff --git a/actionmailer/CHANGELOG.md b/actionmailer/CHANGELOG.md index bcfde6d96f..ab93745f60 100644 --- a/actionmailer/CHANGELOG.md +++ b/actionmailer/CHANGELOG.md @@ -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. diff --git a/actionmailer/lib/action_mailer/base.rb b/actionmailer/lib/action_mailer/base.rb index d9b88ec608..bc540aece0 100644 --- a/actionmailer/lib/action_mailer/base.rb +++ b/actionmailer/lib/action_mailer/base.rb @@ -897,6 +897,11 @@ module ActionMailer 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 diff --git a/actionmailer/lib/action_mailer/railtie.rb b/actionmailer/lib/action_mailer/railtie.rb index 6f760732e2..c62d4b5082 100644 --- a/actionmailer/lib/action_mailer/railtie.rb +++ b/actionmailer/lib/action_mailer/railtie.rb @@ -30,7 +30,7 @@ module ActionMailer 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)) diff --git a/actionpack/lib/abstract_controller/base.rb b/actionpack/lib/abstract_controller/base.rb index 15faabf977..4026dab2ce 100644 --- a/actionpack/lib/abstract_controller/base.rb +++ b/actionpack/lib/abstract_controller/base.rb @@ -164,6 +164,14 @@ module AbstractController _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 diff --git a/actionpack/lib/abstract_controller/railties/routes_helpers.rb b/actionpack/lib/abstract_controller/railties/routes_helpers.rb index 6684f46f64..568c47e43a 100644 --- a/actionpack/lib/abstract_controller/railties/routes_helpers.rb +++ b/actionpack/lib/abstract_controller/railties/routes_helpers.rb @@ -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 diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb index d869b62398..0906c67719 100644 --- a/actionpack/lib/action_dispatch/routing/route_set.rb +++ b/actionpack/lib/action_dispatch/routing/route_set.rb @@ -86,12 +86,13 @@ module ActionDispatch # 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 @@ module ActionDispatch 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 @@ module ActionDispatch 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 @@ module ActionDispatch 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 @@ module ActionDispatch # 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 @@ module ActionDispatch 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 @@ module ActionDispatch 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 diff --git a/actionpack/test/routing/helper_test.rb b/actionpack/test/routing/helper_test.rb index 0028aaa629..09ca7ff73b 100644 --- a/actionpack/test/routing/helper_test.rb +++ b/actionpack/test/routing/helper_test.rb @@ -26,6 +26,20 @@ module ActionDispatch 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 diff --git a/actionview/lib/action_view/rendering.rb b/actionview/lib/action_view/rendering.rb index c92d090cce..81d5836a8c 100644 --- a/actionview/lib/action_view/rendering.rb +++ b/actionview/lib/action_view/rendering.rb @@ -35,12 +35,13 @@ module ActionView 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 diff --git a/guides/source/action_mailer_basics.md b/guides/source/action_mailer_basics.md index cb1c1c653d..9ad9319255 100644 --- a/guides/source/action_mailer_basics.md +++ b/guides/source/action_mailer_basics.md @@ -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 diff --git a/railties/lib/rails/engine.rb b/railties/lib/rails/engine.rb index b36ab3d0d5..aa4f94ef1b 100644 --- a/railties/lib/rails/engine.rb +++ b/railties/lib/rails/engine.rb @@ -395,7 +395,7 @@ module Rails 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 diff --git a/railties/test/application/initializers/frameworks_test.rb b/railties/test/application/initializers/frameworks_test.rb index 8e76bf27f3..ae550331bd 100644 --- a/railties/test/application/initializers/frameworks_test.rb +++ b/railties/test/application/initializers/frameworks_test.rb @@ -50,7 +50,7 @@ module ApplicationTests 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 @@ module ApplicationTests 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 diff --git a/railties/test/application/mailer_previews_test.rb b/railties/test/application/mailer_previews_test.rb index 8b91a1171f..55e917c3ec 100644 --- a/railties/test/application/mailer_previews_test.rb +++ b/railties/test/application/mailer_previews_test.rb @@ -417,6 +417,58 @@ module ApplicationTests 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 |