From dd9991bac598bb5da312278a749cf85e19b027cc Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Tue, 17 Mar 2020 18:36:41 -0700 Subject: [PATCH] Deprecate rendering templates with . in the name Allowing templates with "." introduces some ambiguity. Is index.html.erb a template named "index" with format "html", or is it a template named "index.html" without a format? We know it's probably the former, but if we asked ActionView to render "index.html" we would currently get some combination of the two: a Template with index.html as the name and virtual path, but with html as the format. This deprecates having "." anywhere in the template's name, we should reserve this character for specifying formats. I think in 99% of cases this will be people specifying `index.html` instead of simply `index`. This was actually once deprecated in the 3.x series (removed in 6c57177f2c7f4f934716d588545902d5fc00fa99) but I don't think we can rely on nobody having introduced this in the past 8 years. --- actionpack/test/controller/render_test.rb | 4 +++- actionpack/test/controller/renderer_test.rb | 2 +- actionview/lib/action_view/template/resolver.rb | 9 +++++++++ .../fallback_file_system_resolver_test.rb | 2 +- actionview/test/template/render_test.rb | 16 ++++++++++++---- .../test/template/resolver_shared_tests.rb | 2 +- 6 files changed, 27 insertions(+), 8 deletions(-) diff --git a/actionpack/test/controller/render_test.rb b/actionpack/test/controller/render_test.rb index c94e60be0db9e..474abff234190 100644 --- a/actionpack/test/controller/render_test.rb +++ b/actionpack/test/controller/render_test.rb @@ -365,7 +365,9 @@ def test_dynamic_render_with_absolute_path def test_dynamic_render assert File.exist?(File.expand_path("../../test/abstract_unit.rb", __dir__)) assert_raises ActionView::MissingTemplate do - get :dynamic_render, params: { id: '../\\../test/abstract_unit.rb' } + assert_deprecated do + get :dynamic_render, params: { id: '../\\../test/abstract_unit.rb' } + end end end diff --git a/actionpack/test/controller/renderer_test.rb b/actionpack/test/controller/renderer_test.rb index 5dfebc0f37628..4d45c082d7cbe 100644 --- a/actionpack/test/controller/renderer_test.rb +++ b/actionpack/test/controller/renderer_test.rb @@ -109,7 +109,7 @@ def test_render_component xml = "

Hello world!

\n" assert_equal html, render["respond_to/using_defaults"] - assert_equal xml, render["respond_to/using_defaults.xml.builder"] + assert_equal xml, assert_deprecated { render["respond_to/using_defaults.xml.builder"] } assert_equal xml, render["respond_to/using_defaults", formats: :xml] end diff --git a/actionview/lib/action_view/template/resolver.rb b/actionview/lib/action_view/template/resolver.rb index cc3d8bb0724c7..0f79f6febe995 100644 --- a/actionview/lib/action_view/template/resolver.rb +++ b/actionview/lib/action_view/template/resolver.rb @@ -227,6 +227,10 @@ def reject_files_external_to_app(files) end def find_template_paths_from_details(path, details) + if path.name.include?(".") + ActiveSupport::Deprecation.warn("Rendering actions with '.' in the name is deprecated: #{path}") + end + query = build_query(path, details) find_template_paths(query) end @@ -331,6 +335,11 @@ def find_candidate_template_paths(path) end def find_template_paths_from_details(path, details) + if path.name.include?(".") + # Fall back to the unoptimized resolver, which will warn + return super + end + candidates = find_candidate_template_paths(path) regex = build_regex(path, details) diff --git a/actionview/test/template/fallback_file_system_resolver_test.rb b/actionview/test/template/fallback_file_system_resolver_test.rb index fa770f3a15803..d5290b5fe742d 100644 --- a/actionview/test/template/fallback_file_system_resolver_test.rb +++ b/actionview/test/template/fallback_file_system_resolver_test.rb @@ -8,7 +8,7 @@ def setup end def test_should_have_no_virtual_path - templates = @root_resolver.find_all("hello_world.erb", "#{FIXTURE_LOAD_PATH}/test", false, locale: [], formats: [:html], variants: [], handlers: [:erb]) + templates = @root_resolver.find_all("hello_world", "#{FIXTURE_LOAD_PATH}/test", false, locale: [], formats: [:html], variants: [], handlers: [:erb]) assert_equal 1, templates.size assert_equal "Hello world!", templates[0].source assert_nil templates[0].virtual_path diff --git a/actionview/test/template/render_test.rb b/actionview/test/template/render_test.rb index ea4a3b643e6a6..e315d6e4e67e1 100644 --- a/actionview/test/template/render_test.rb +++ b/actionview/test/template/render_test.rb @@ -200,7 +200,9 @@ def test_render_partial_from_default def test_render_outside_path assert File.exist?(File.expand_path("../../test/abstract_unit.rb", __dir__)) assert_raises ActionView::MissingTemplate do - @view.render(template: "../\\../test/abstract_unit.rb") + assert_deprecated do + @view.render(template: "../\\../test/abstract_unit.rb") + end end end @@ -338,8 +340,10 @@ def test_render_partial_collection end def test_render_partial_collection_with_partial_name_containing_dot - assert_equal "Hello: davidHello: mary", - @view.render(partial: "test/customer.mobile", collection: [ Customer.new("david"), Customer.new("mary") ]) + assert_deprecated do + assert_equal "Hello: davidHello: mary", + @view.render(partial: "test/customer.mobile", collection: [ Customer.new("david"), Customer.new("mary") ]) + end end def test_render_partial_collection_as_by_string @@ -579,7 +583,11 @@ def test_render_does_not_use_unregistered_extension_and_template_handler def test_render_ignores_templates_with_malformed_template_handlers %w(malformed malformed.erb malformed.html.erb malformed.en.html.erb).each do |name| assert File.exist?(File.expand_path("#{FIXTURE_LOAD_PATH}/test/malformed/#{name}~")), "Malformed file (#{name}~) which should be ignored does not exists" - assert_raises(ActionView::MissingTemplate) { @view.render(template: "test/malformed/#{name}") } + assert_raises(ActionView::MissingTemplate) do + ActiveSupport::Deprecation.silence do + @view.render(template: "test/malformed/#{name}") + end + end end end diff --git a/actionview/test/template/resolver_shared_tests.rb b/actionview/test/template/resolver_shared_tests.rb index 470628af11159..332eb30a58f0a 100644 --- a/actionview/test/template/resolver_shared_tests.rb +++ b/actionview/test/template/resolver_shared_tests.rb @@ -169,7 +169,7 @@ def test_templates_sort_by_formats_html_first def test_virtual_path_is_preserved_with_dot with_file "test/hello_world.html.erb", "Hello html!" - template = context.find("hello_world.html", "test", false, [], {}) + template = assert_deprecated { context.find("hello_world.html", "test", false, [], {}) } assert_equal "test/hello_world.html", template.virtual_path template = context.find("hello_world", "test", false, [], {})