Merge pull request #35688 from jhawthorn/render_file_rfc

RFC: Introduce Template::File

13 files changed, 131 insertions, 40 deletions

diff --git a/actionpack/test/controller/new_base/render_file_test.rb b/actionpack/test/controller/new_base/render_file_test.rb
index de8af029e0..82325c5bb2 100644
--- a/actionpack/test/controller/new_base/render_file_test.rb
+++ b/actionpack/test/controller/new_base/render_file_test.rb
@@ -40,32 +40,44 @@ module RenderFile
     testing RenderFile::BasicController
 
     test "rendering simple template" do
-      get :index
+      assert_deprecated do
+        get :index
+      end
       assert_response "Hello world!"
     end
 
     test "rendering template with ivar" do
-      get :with_instance_variables
+      assert_deprecated do
+        get :with_instance_variables
+      end
       assert_response "The secret is in the sauce\n"
     end
 
     test "rendering a relative path" do
-      get :relative_path
+      assert_deprecated do
+        get :relative_path
+      end
       assert_response "The secret is in the sauce\n"
     end
 
     test "rendering a relative path with dot" do
-      get :relative_path_with_dot
+      assert_deprecated do
+        get :relative_path_with_dot
+      end
       assert_response "The secret is in the sauce\n"
     end
 
     test "rendering a Pathname" do
-      get :pathname
+      assert_deprecated do
+        get :pathname
+      end
       assert_response "The secret is in the sauce\n"
     end
 
     test "rendering file with locals" do
-      get :with_locals
+      assert_deprecated do
+        get :with_locals
+      end
       assert_response "The secret is in the sauce\n"
     end
   end
diff --git a/actionpack/test/controller/render_test.rb b/actionpack/test/controller/render_test.rb
index 4750093c5c..6d198ca42f 100644
--- a/actionpack/test/controller/render_test.rb
+++ b/actionpack/test/controller/render_test.rb
@@ -325,7 +325,7 @@ class ExpiresInRenderTest < ActionController::TestCase
   def test_dynamic_render_with_file
     # This is extremely bad, but should be possible to do.
     assert File.exist?(File.expand_path("../../test/abstract_unit.rb", __dir__))
-    response = get :dynamic_render_with_file, params: { id: '../\\../test/abstract_unit.rb' }
+    response = assert_deprecated { get :dynamic_render_with_file, params: { id: '../\\../test/abstract_unit.rb' } }
     assert_equal File.read(File.expand_path("../../test/abstract_unit.rb", __dir__)),
       response.body
   end
@@ -351,7 +351,7 @@ class ExpiresInRenderTest < ActionController::TestCase
 
   def test_permitted_dynamic_render_file_hash
     assert File.exist?(File.expand_path("../../test/abstract_unit.rb", __dir__))
-    response = get :dynamic_render_permit, params: { id: { file: '../\\../test/abstract_unit.rb' } }
+    response = assert_deprecated { get :dynamic_render_permit, params: { id: { file: '../\\../test/abstract_unit.rb' } } }
     assert_equal File.read(File.expand_path("../../test/abstract_unit.rb", __dir__)),
       response.body
   end
diff --git a/actionpack/test/controller/renderer_test.rb b/actionpack/test/controller/renderer_test.rb
index ae8330e029..3d5161f207 100644
--- a/actionpack/test/controller/renderer_test.rb
+++ b/actionpack/test/controller/renderer_test.rb
@@ -40,7 +40,7 @@ class RendererTest < ActiveSupport::TestCase
 
   test "rendering with an instance renderer" do
     renderer = ApplicationController.renderer.new
-    content  = renderer.render file: "test/hello_world"
+    content  = assert_deprecated { renderer.render file: "test/hello_world" }
 
     assert_equal "Hello world!", content
   end
diff --git a/actionview/lib/action_view/file_template.rb b/actionview/lib/action_view/file_template.rb
index dea02176eb..e0dc7da3b6 100644
--- a/actionview/lib/action_view/file_template.rb
+++ b/actionview/lib/action_view/file_template.rb
@@ -11,7 +11,7 @@ module ActionView
     end
 
     def source
-      File.binread @filename
+      ::File.binread @filename
     end
 
     def refresh(_)
diff --git a/actionview/lib/action_view/renderer/template_renderer.rb b/actionview/lib/action_view/renderer/template_renderer.rb
index 9548fe12c4..60cb2ceafa 100644
--- a/actionview/lib/action_view/renderer/template_renderer.rb
+++ b/actionview/lib/action_view/renderer/template_renderer.rb
@@ -26,7 +26,12 @@ module ActionView
         elsif options.key?(:html)
           Template::HTML.new(options[:html], formats.first)
         elsif options.key?(:file)
-          @lookup_context.with_fallbacks.find_file(options[:file], nil, false, keys, @details)
+          if File.exist?(options[:file])
+            Template::File.new(options[:file])
+          else
+            ActiveSupport::Deprecation.warn "render file: should be given the absolute path to a file"
+            @lookup_context.with_fallbacks.find_file(options[:file], nil, false, keys, @details)
+          end
         elsif options.key?(:inline)
           handler = Template.handler_for_extension(options[:type] || "erb")
           format = if handler.respond_to?(:default_format)
diff --git a/actionview/lib/action_view/template.rb b/actionview/lib/action_view/template.rb
index 7bc5e86edb..ebe52532d6 100644
--- a/actionview/lib/action_view/template.rb
+++ b/actionview/lib/action_view/template.rb
@@ -113,6 +113,7 @@ module ActionView
 
     eager_autoload do
       autoload :Error
+      autoload :File
       autoload :Handlers
       autoload :HTML
       autoload :Inline
@@ -139,7 +140,7 @@ module ActionView
       @virtual_path      = virtual_path
 
       @variable = if @virtual_path
-        base = @virtual_path[-1] == "/" ? "" : File.basename(@virtual_path)
+        base = @virtual_path[-1] == "/" ? "" : ::File.basename(@virtual_path)
         base =~ /\A_?(.*?)(?:\.\w+)*\z/
         $1.to_sym
       end
diff --git a/actionview/lib/action_view/template/error.rb b/actionview/lib/action_view/template/error.rb
index d0ea03e228..aace0be04d 100644
--- a/actionview/lib/action_view/template/error.rb
+++ b/actionview/lib/action_view/template/error.rb
@@ -104,7 +104,7 @@ module ActionView
       def line_number
         @line_number ||=
           if file_name
-            regexp = /#{Regexp.escape File.basename(file_name)}:(\d+)/
+            regexp = /#{Regexp.escape ::File.basename(file_name)}:(\d+)/
             $1 if message =~ regexp || backtrace.find { |line| line =~ regexp }
           end
       end
diff --git a/actionview/lib/action_view/template/file.rb b/actionview/lib/action_view/template/file.rb
new file mode 100644
index 0000000000..487e5735cf
--- /dev/null
+++ b/actionview/lib/action_view/template/file.rb
@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+module ActionView #:nodoc:
+  # = Action View File Template
+  class Template #:nodoc:
+    class File #:nodoc:
+      attr_accessor :type, :format
+
+      def initialize(filename)
+        @filename = filename.to_s
+        extname = ::File.extname(filename).delete(".")
+        @type = Template::Types[extname] || Template::Types[:text]
+        @format = @type.symbol
+      end
+
+      def identifier
+        @filename
+      end
+
+      def render(*args)
+        ::File.read(@filename)
+      end
+
+      def formats; Array(format); end
+      deprecate :formats
+    end
+  end
+end
diff --git a/actionview/test/actionpack/abstract/render_test.rb b/actionview/test/actionpack/abstract/render_test.rb
index d863548a5c..e4e8ac93b2 100644
--- a/actionview/test/actionpack/abstract/render_test.rb
+++ b/actionview/test/actionpack/abstract/render_test.rb
@@ -26,7 +26,7 @@ module AbstractController
       end
 
       def file
-        render file: "some/file"
+        ActiveSupport::Deprecation.silence { render file: "some/file" }
       end
 
       def inline
diff --git a/actionview/test/actionpack/controller/render_test.rb b/actionview/test/actionpack/controller/render_test.rb
index 52c3c54d96..c8ce7366d1 100644
--- a/actionview/test/actionpack/controller/render_test.rb
+++ b/actionview/test/actionpack/controller/render_test.rb
@@ -872,48 +872,64 @@ class RenderTest < ActionController::TestCase
 
   # :ported:
   def test_render_file_with_instance_variables
-    get :render_file_with_instance_variables
+    assert_deprecated do
+      get :render_file_with_instance_variables
+    end
     assert_equal "The secret is in the sauce\n", @response.body
   end
 
   def test_render_file
-    get :hello_world_file
+    assert_deprecated do
+      get :hello_world_file
+    end
     assert_equal "Hello world!", @response.body
   end
 
   # :ported:
   def test_render_file_not_using_full_path
-    get :render_file_not_using_full_path
+    assert_deprecated do
+      get :render_file_not_using_full_path
+    end
     assert_equal "The secret is in the sauce\n", @response.body
   end
 
   # :ported:
   def test_render_file_not_using_full_path_with_dot_in_path
-    get :render_file_not_using_full_path_with_dot_in_path
+    assert_deprecated do
+      get :render_file_not_using_full_path_with_dot_in_path
+    end
     assert_equal "The secret is in the sauce\n", @response.body
   end
 
   # :ported:
   def test_render_file_using_pathname
-    get :render_file_using_pathname
+    assert_deprecated do
+      get :render_file_using_pathname
+    end
     assert_equal "The secret is in the sauce\n", @response.body
   end
 
   # :ported:
   def test_render_file_with_locals
-    get :render_file_with_locals
+    assert_deprecated do
+      get :render_file_with_locals
+    end
     assert_equal "The secret is in the sauce\n", @response.body
   end
 
   # :ported:
   def test_render_file_as_string_with_locals
-    get :render_file_as_string_with_locals
+    assert_deprecated do
+      get :render_file_as_string_with_locals
+    end
     assert_equal "The secret is in the sauce\n", @response.body
   end
 
   # :assessed:
   def test_render_file_from_template
-    get :render_file_from_template
+    assert_deprecated do
+      get :render_file_from_template
+    end
     assert_equal "The secret is in the sauce\n", @response.body
   end
 
@@ -1133,11 +1149,19 @@ class RenderTest < ActionController::TestCase
   end
 
   def test_bad_render_to_string_still_throws_exception
-    assert_raise(ActionView::MissingTemplate) { get :render_to_string_with_exception }
+    assert_deprecated do
+      assert_raise(ActionView::MissingTemplate) do
+        get :render_to_string_with_exception
+      end
+    end
   end
 
   def test_render_to_string_that_throws_caught_exception_doesnt_break_assigns
-    assert_nothing_raised { get :render_to_string_with_caught_exception }
+    assert_deprecated do
+      assert_nothing_raised do
+        get :render_to_string_with_caught_exception
+      end
+    end
     assert_equal "i'm before the render", @controller.instance_variable_get(:@before)
     assert_equal "i'm after the render", @controller.instance_variable_get(:@after)
   end
diff --git a/actionview/test/template/log_subscriber_test.rb b/actionview/test/template/log_subscriber_test.rb
index 85735139c1..8b160a7336 100644
--- a/actionview/test/template/log_subscriber_test.rb
+++ b/actionview/test/template/log_subscriber_test.rb
@@ -51,9 +51,20 @@ class AVLogSubscriberTest < ActiveSupport::TestCase
     def @view.combined_fragment_cache_key(*); "ahoy `controller` dependency"; end
   end
 
+  def test_render_template_template
+    Rails.stub(:root, File.expand_path(FIXTURE_LOAD_PATH)) do
+      @view.render(template: "test/hello_world")
+      wait
+
+      assert_equal 2, @logger.logged(:info).size
+      assert_match(/Rendering test\/hello_world\.erb/, @logger.logged(:info).first)
+      assert_match(/Rendered test\/hello_world\.erb/, @logger.logged(:info).last)
+    end
+  end
+
   def test_render_file_template
     Rails.stub(:root, File.expand_path(FIXTURE_LOAD_PATH)) do
-      @view.render(file: "test/hello_world")
+      @view.render(file: "#{FIXTURE_LOAD_PATH}/test/hello_world.erb")
       wait
 
       assert_equal 2, @logger.logged(:info).size
diff --git a/actionview/test/template/render_test.rb b/actionview/test/template/render_test.rb
index baff7cd83f..15c41051de 100644
--- a/actionview/test/template/render_test.rb
+++ b/actionview/test/template/render_test.rb
@@ -53,15 +53,20 @@ module RenderTestCases
     assert_match(/You invoked render but did not give any of (.+) option\./, e.message)
   end
 
+  def test_render_template
+    assert_equal "Hello world!", @view.render(template: "test/hello_world")
+  end
+
+
   def test_render_file
-    assert_equal "Hello world!", @view.render(file: "test/hello_world")
+    assert_equal "Hello world!", assert_deprecated { @view.render(file: "test/hello_world") }
   end
 
   # Test if :formats, :locale etc. options are passed correctly to the resolvers.
   def test_render_file_with_format
-    assert_match "<h1>No Comment</h1>", @view.render(file: "comments/empty", formats: [:html])
-    assert_match "<error>No Comment</error>", @view.render(file: "comments/empty", formats: [:xml])
-    assert_match "<error>No Comment</error>", @view.render(file: "comments/empty", formats: :xml)
+    assert_match "<h1>No Comment</h1>", assert_deprecated { @view.render(file: "comments/empty", formats: [:html]) }
+    assert_match "<error>No Comment</error>", assert_deprecated { @view.render(file: "comments/empty", formats: [:xml]) }
+    assert_match "<error>No Comment</error>", assert_deprecated { @view.render(file: "comments/empty", formats: :xml) }
   end
 
   def test_render_template_with_format
@@ -94,8 +99,8 @@ module RenderTestCases
   end
 
   def test_render_file_with_locale
-    assert_equal "<h1>Kein Kommentar</h1>", @view.render(file: "comments/empty", locale: [:de])
-    assert_equal "<h1>Kein Kommentar</h1>", @view.render(file: "comments/empty", locale: :de)
+    assert_equal "<h1>Kein Kommentar</h1>", assert_deprecated { @view.render(file: "comments/empty", locale: [:de]) }
+    assert_equal "<h1>Kein Kommentar</h1>", assert_deprecated { @view.render(file: "comments/empty", locale: :de) }
   end
 
   def test_render_template_with_locale
@@ -107,8 +112,8 @@ module RenderTestCases
   end
 
   def test_render_file_with_handlers
-    assert_equal "<h1>No Comment</h1>\n", @view.render(file: "comments/empty", handlers: [:builder])
-    assert_equal "<h1>No Comment</h1>\n", @view.render(file: "comments/empty", handlers: :builder)
+    assert_equal "<h1>No Comment</h1>\n", assert_deprecated { @view.render(file: "comments/empty", handlers: [:builder]) }
+    assert_equal "<h1>No Comment</h1>\n", assert_deprecated { @view.render(file: "comments/empty", handlers: :builder) }
   end
 
   def test_render_template_with_handlers
@@ -156,22 +161,27 @@ module RenderTestCases
     assert_equal "Elastica", @view.render(template: "/shared")
   end
 
-  def test_render_file_with_full_path
+  def test_render_file_with_full_path_no_extension
     template_path = File.expand_path("../fixtures/test/hello_world", __dir__)
+    assert_equal "Hello world!", assert_deprecated { @view.render(file: template_path) }
+  end
+
+  def test_render_file_with_full_path
+    template_path = File.expand_path("../fixtures/test/hello_world.erb", __dir__)
     assert_equal "Hello world!", @view.render(file: template_path)
   end
 
   def test_render_file_with_instance_variables
-    assert_equal "The secret is in the sauce\n", @view.render(file: "test/render_file_with_ivar")
+    assert_equal "The secret is in the sauce\n", assert_deprecated { @view.render(file: "test/render_file_with_ivar") }
   end
 
   def test_render_file_with_locals
     locals = { secret: "in the sauce" }
-    assert_equal "The secret is in the sauce\n", @view.render(file: "test/render_file_with_locals", locals: locals)
+    assert_equal "The secret is in the sauce\n", assert_deprecated { @view.render(file: "test/render_file_with_locals", locals: locals) }
   end
 
   def test_render_file_not_using_full_path_with_dot_in_path
-    assert_equal "The secret is in the sauce\n", @view.render(file: "test/dot.directory/render_file_with_ivar")
+    assert_equal "The secret is in the sauce\n", assert_deprecated { @view.render(file: "test/dot.directory/render_file_with_ivar") }
   end
 
   def test_render_partial_from_default
@@ -292,7 +302,7 @@ module RenderTestCases
   end
 
   def test_render_file_with_errors
-    e = assert_raises(ActionView::Template::Error) { @view.render(file: File.expand_path("test/_raise", FIXTURE_LOAD_PATH)) }
+    e = assert_raises(ActionView::Template::Error) { assert_deprecated { @view.render(file: File.expand_path("test/_raise", FIXTURE_LOAD_PATH)) } }
     assert_match %r!method.*doesnt_exist!, e.message
     assert_equal "", e.sub_template_message
     assert_equal "1", e.line_number
diff --git a/actionview/test/template/streaming_render_test.rb b/actionview/test/template/streaming_render_test.rb
index a5b59a700e..a5e673e71e 100644
--- a/actionview/test/template/streaming_render_test.rb
+++ b/actionview/test/template/streaming_render_test.rb
@@ -47,12 +47,12 @@ class FiberedTest < SetupFiberedBase
   end
 
   def test_render_file
-    assert_equal "Hello world!", buffered_render(file: "test/hello_world")
+    assert_equal "Hello world!", assert_deprecated { buffered_render(file: "test/hello_world") }
   end
 
   def test_render_file_with_locals
     locals = { secret: "in the sauce" }
-    assert_equal "The secret is in the sauce\n", buffered_render(file: "test/render_file_with_locals", locals: locals)
+    assert_equal "The secret is in the sauce\n", assert_deprecated { buffered_render(file: "test/render_file_with_locals", locals: locals) }
   end
 
   def test_render_partial