From 916b74d16a5ed369ba58c17630f593470ca502a8 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Thu, 31 Jan 2019 14:06:21 -0800 Subject: use the source returned from encode! --- actionview/lib/action_view/template.rb | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/actionview/lib/action_view/template.rb b/actionview/lib/action_view/template.rb index 8ce8053011..a283d2467c 100644 --- a/actionview/lib/action_view/template.rb +++ b/actionview/lib/action_view/template.rb @@ -202,7 +202,9 @@ module ActionView # before passing the source on to the template engine, leaving a # blank line in its stead. def encode! - return unless source.encoding == Encoding::BINARY + source = self.source + + return source unless source.encoding == Encoding::BINARY # Look for # encoding: *. If we find one, we'll encode the # String in that encoding, otherwise, we'll use the @@ -290,7 +292,7 @@ module ActionView # In general, this means that templates will be UTF-8 inside of Rails, # regardless of the original source encoding. def compile(mod) - encode! + source = encode! code = @handler.call(self) # Make sure that the resulting String to be eval'd is in the @@ -312,7 +314,7 @@ module ActionView # handler is valid in the default_internal. This is for handlers # that handle encoding but screw up unless source.valid_encoding? - raise WrongEncodingError.new(@source, Encoding.default_internal) + raise WrongEncodingError.new(source, Encoding.default_internal) end mod.module_eval(source, identifier, 0) -- cgit v1.2.3 From 2169bd3d2a9d2f331a5dd6e41d9d638e0da6117c Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Thu, 31 Jan 2019 15:34:09 -0800 Subject: Introduce a file type template, deprecate `Template#refresh` Every template that specifies a "virtual path" loses the template source when the template gets compiled: https://github.com/rails/rails/blob/eda0f574f129fcd5ad1fc58b55cb6d1db71ea95c/actionview/lib/action_view/template.rb#L275 The "refresh" method seems to think that the source code for a template can be recovered if there is a virtual path: https://github.com/rails/rails/blob/eda0f574f129fcd5ad1fc58b55cb6d1db71ea95c/actionview/lib/action_view/template.rb#L171-L188 Every call site that allocates a template object *and* provides a "virtual path" reads the template contents from the filesystem: https://github.com/rails/rails/blob/eda0f574f129fcd5ad1fc58b55cb6d1db71ea95c/actionview/lib/action_view/template/resolver.rb#L229-L231 Templates that are inline or literals don't provide a "virtual path": https://github.com/rails/rails/blob/eda0f574f129fcd5ad1fc58b55cb6d1db71ea95c/actionview/lib/action_view/renderer/template_renderer.rb#L34 This commit introduces a `FileTemplate` type that subclasses `Template`. The `FileTemplate` keeps a reference to the filename, and reads the source from the filesystem. This effectively makes the template source immutable. Other classes depended on the source to be mutated while being compiled, so this commit also introduces a temporary way to pass the mutated source to the ERB (or whatever) compiler. See `LegacyTemplate`. I think we should consider it an error to provide a virtual path on a non file type template an non-file templates can't recover their source. Here is an example: https://github.com/rails/rails/blob/eda0f574f129fcd5ad1fc58b55cb6d1db71ea95c/actionview/lib/action_view/testing/resolvers.rb#L53 This provides a "virtual path" so the source code (a string literal) is thrown away after compilation. Clearly we can't recover that string, so I think this should be an error. --- actionview/lib/action_view.rb | 1 + actionview/lib/action_view/file_template.rb | 33 +++++++++++++++++++++++++ actionview/lib/action_view/template.rb | 13 +++++++++- actionview/lib/action_view/template/resolver.rb | 3 +-- 4 files changed, 47 insertions(+), 3 deletions(-) create mode 100644 actionview/lib/action_view/file_template.rb diff --git a/actionview/lib/action_view.rb b/actionview/lib/action_view.rb index c4a614fa6d..6ff2d70e35 100644 --- a/actionview/lib/action_view.rb +++ b/actionview/lib/action_view.rb @@ -45,6 +45,7 @@ module ActionView autoload :Rendering autoload :RoutingUrlFor autoload :Template + autoload :FileTemplate autoload :ViewPaths autoload_under "renderer" do diff --git a/actionview/lib/action_view/file_template.rb b/actionview/lib/action_view/file_template.rb new file mode 100644 index 0000000000..4921074383 --- /dev/null +++ b/actionview/lib/action_view/file_template.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require "action_view/template" + +module ActionView + class FileTemplate < Template + def initialize(filename, handler, details) + @filename = filename + + super(nil, filename, handler, details) + end + + def source + File.binread @filename + end + + def refresh(_) + self + end + + # Exceptions are marshalled when using the parallel test runner with DRb, so we need + # to ensure that references to the template object can be marshalled as well. This means forgoing + # the marshalling of the compiler mutex and instantiating that again on unmarshalling. + def marshal_dump # :nodoc: + [ @identifier, @handler, @compiled, @original_encoding, @locals, @virtual_path, @updated_at, @formats, @variants ] + end + + def marshal_load(array) # :nodoc: + @identifier, @handler, @compiled, @original_encoding, @locals, @virtual_path, @updated_at, @formats, @variants = *array + @compile_mutex = Mutex.new + end + end +end diff --git a/actionview/lib/action_view/template.rb b/actionview/lib/action_view/template.rb index a283d2467c..2d1762e568 100644 --- a/actionview/lib/action_view/template.rb +++ b/actionview/lib/action_view/template.rb @@ -2,7 +2,9 @@ require "active_support/core_ext/object/try" require "active_support/core_ext/kernel/singleton_class" +require "active_support/deprecation" require "thread" +require "delegate" module ActionView # = Action View Template @@ -279,6 +281,15 @@ module ActionView end end + class LegacyTemplate < DelegateClass(Template) # :nodoc: + attr_reader :source + + def initialize(template, source) + super(template) + @source = source + end + end + # Among other things, this method is responsible for properly setting # the encoding of the compiled template. # @@ -293,7 +304,7 @@ module ActionView # regardless of the original source encoding. def compile(mod) source = encode! - code = @handler.call(self) + code = @handler.call(LegacyTemplate.new(self, source)) # Make sure that the resulting String to be eval'd is in the # encoding of the code diff --git a/actionview/lib/action_view/template/resolver.rb b/actionview/lib/action_view/template/resolver.rb index 12ae82f8c5..3b4594942b 100644 --- a/actionview/lib/action_view/template/resolver.rb +++ b/actionview/lib/action_view/template/resolver.rb @@ -226,9 +226,8 @@ module ActionView template_paths.map do |template| handler, format, variant = extract_handler_and_format_and_variant(template) - contents = File.binread(template) - Template.new(contents, File.expand_path(template), handler, + FileTemplate.new(File.expand_path(template), handler, virtual_path: path.virtual, format: format, variant: variant, -- cgit v1.2.3 From 28f88e0074f473f58c2d3fd54cb3daff81027c12 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Fri, 1 Feb 2019 16:10:02 -0800 Subject: Pass source to template handler and deprecate old style handler This commit passes the mutated source to the template handler as a parameter and deprecates the old handlers. Old handlers required that templates contain a reference to mutated source code, but we would like to make template objects "read only". This change lets the template remain "read only" while still giving template handlers access to the source code after mutations. --- actionview/lib/action_view/template.rb | 3 +-- actionview/lib/action_view/template/handlers.rb | 28 +++++++++++++++++++++- .../lib/action_view/template/handlers/builder.rb | 4 ++-- .../lib/action_view/template/handlers/erb.rb | 10 ++++---- .../lib/action_view/template/handlers/html.rb | 2 +- .../lib/action_view/template/handlers/raw.rb | 4 ++-- actionview/test/abstract_unit.rb | 2 +- .../test/actionpack/controller/layout_test.rb | 4 ++-- .../test/template/dependency_tracker_test.rb | 4 ++-- actionview/test/template/render_test.rb | 15 +++++++++--- 10 files changed, 55 insertions(+), 21 deletions(-) diff --git a/actionview/lib/action_view/template.rb b/actionview/lib/action_view/template.rb index 2d1762e568..3b2c264ed4 100644 --- a/actionview/lib/action_view/template.rb +++ b/actionview/lib/action_view/template.rb @@ -2,7 +2,6 @@ require "active_support/core_ext/object/try" require "active_support/core_ext/kernel/singleton_class" -require "active_support/deprecation" require "thread" require "delegate" @@ -304,7 +303,7 @@ module ActionView # regardless of the original source encoding. def compile(mod) source = encode! - code = @handler.call(LegacyTemplate.new(self, source)) + code = @handler.call(self, source) # Make sure that the resulting String to be eval'd is in the # encoding of the code diff --git a/actionview/lib/action_view/template/handlers.rb b/actionview/lib/action_view/template/handlers.rb index 7ec76dcc3f..f2a2341b8e 100644 --- a/actionview/lib/action_view/template/handlers.rb +++ b/actionview/lib/action_view/template/handlers.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require "active_support/deprecation" + module ActionView #:nodoc: # = Action View Template Handlers class Template #:nodoc: @@ -14,7 +16,7 @@ module ActionView #:nodoc: base.register_template_handler :erb, ERB.new base.register_template_handler :html, Html.new base.register_template_handler :builder, Builder.new - base.register_template_handler :ruby, :source.to_proc + base.register_template_handler :ruby, lambda { |_, source| source } end @@template_handlers = {} @@ -24,11 +26,35 @@ module ActionView #:nodoc: @@template_extensions ||= @@template_handlers.keys end + class LegacyHandlerWrapper < SimpleDelegator # :nodoc: + def call(view, source) + __getobj__.call(ActionView::Template::LegacyTemplate.new(view, source)) + end + end + # Register an object that knows how to handle template files with the given # extensions. This can be used to implement new template types. # The handler must respond to +:call+, which will be passed the template # and should return the rendered template as a String. def register_template_handler(*extensions, handler) + params = if handler.is_a?(Proc) + handler.parameters + else + handler.method(:call).parameters + end + + unless params.find_all { |type, _| type == :req }.length >= 2 + ActiveSupport::Deprecation.warn <<~eowarn + Single arity template handlers are deprecated. Template handlers must + now accept two parameters, the view object and the source for the view object. + Change: + >> #{handler.class}#call(#{params.map(&:last).join(", ")}) + To: + >> #{handler.class}#call(#{params.map(&:last).join(", ")}, source) + eowarn + handler = LegacyHandlerWrapper.new(handler) + end + raise(ArgumentError, "Extension is required") if extensions.empty? extensions.each do |extension| @@template_handlers[extension.to_sym] = handler diff --git a/actionview/lib/action_view/template/handlers/builder.rb b/actionview/lib/action_view/template/handlers/builder.rb index 61492ce448..e5413cd2a0 100644 --- a/actionview/lib/action_view/template/handlers/builder.rb +++ b/actionview/lib/action_view/template/handlers/builder.rb @@ -5,11 +5,11 @@ module ActionView class Builder class_attribute :default_format, default: :xml - def call(template) + def call(template, source) require_engine "xml = ::Builder::XmlMarkup.new(:indent => 2);" \ "self.output_buffer = xml.target!;" + - template.source + + source + ";xml.target!;" end diff --git a/actionview/lib/action_view/template/handlers/erb.rb b/actionview/lib/action_view/template/handlers/erb.rb index 7d1a6767d7..b6314a5bc3 100644 --- a/actionview/lib/action_view/template/handlers/erb.rb +++ b/actionview/lib/action_view/template/handlers/erb.rb @@ -28,8 +28,8 @@ module ActionView ENCODING_TAG = Regexp.new("\\A(<%#{ENCODING_FLAG}-?%>)[ \\t]*") - def self.call(template) - new.call(template) + def self.call(template, source) + new.call(template, source) end def supports_streaming? @@ -40,17 +40,17 @@ module ActionView true end - def call(template) + def call(template, source) # First, convert to BINARY, so in case the encoding is # wrong, we can still find an encoding tag # (<%# encoding %>) inside the String using a regular # expression - template_source = template.source.dup.force_encoding(Encoding::ASCII_8BIT) + template_source = source.dup.force_encoding(Encoding::ASCII_8BIT) erb = template_source.gsub(ENCODING_TAG, "") encoding = $2 - erb.force_encoding valid_encoding(template.source.dup, encoding) + erb.force_encoding valid_encoding(source.dup, encoding) # Always make sure we return a String in the default_internal erb.encode! diff --git a/actionview/lib/action_view/template/handlers/html.rb b/actionview/lib/action_view/template/handlers/html.rb index 27004a318c..65857d8587 100644 --- a/actionview/lib/action_view/template/handlers/html.rb +++ b/actionview/lib/action_view/template/handlers/html.rb @@ -3,7 +3,7 @@ module ActionView module Template::Handlers class Html < Raw - def call(template) + def call(template, source) "ActionView::OutputBuffer.new #{super}" end end diff --git a/actionview/lib/action_view/template/handlers/raw.rb b/actionview/lib/action_view/template/handlers/raw.rb index 5cd23a0060..57ebb169fc 100644 --- a/actionview/lib/action_view/template/handlers/raw.rb +++ b/actionview/lib/action_view/template/handlers/raw.rb @@ -3,8 +3,8 @@ module ActionView module Template::Handlers class Raw - def call(template) - "#{template.source.inspect}.html_safe;" + def call(template, source) + "#{source.inspect}.html_safe;" end end end diff --git a/actionview/test/abstract_unit.rb b/actionview/test/abstract_unit.rb index 56d617309c..acc2ed029b 100644 --- a/actionview/test/abstract_unit.rb +++ b/actionview/test/abstract_unit.rb @@ -58,7 +58,7 @@ module RenderERBUtils template = ActionView::Template.new( string.strip, "test template", - ActionView::Template::Handlers::ERB, + ActionView::Template.handler_for_extension(:erb), {}) template.render(ActionView::Base.empty, {}).strip diff --git a/actionview/test/actionpack/controller/layout_test.rb b/actionview/test/actionpack/controller/layout_test.rb index 6d5c97b7fd..838b564c5d 100644 --- a/actionview/test/actionpack/controller/layout_test.rb +++ b/actionview/test/actionpack/controller/layout_test.rb @@ -70,7 +70,7 @@ class LayoutAutoDiscoveryTest < ActionController::TestCase end def test_third_party_template_library_auto_discovers_layout - with_template_handler :mab, lambda { |template| template.source.inspect } do + with_template_handler :mab, lambda { |template, source| source.inspect } do @controller = ThirdPartyTemplateLibraryController.new get :hello assert_response :success @@ -212,7 +212,7 @@ class LayoutSetInResponseTest < ActionController::TestCase end def test_layout_set_when_using_render - with_template_handler :mab, lambda { |template| template.source.inspect } do + with_template_handler :mab, lambda { |template, source| source.inspect } do @controller = SetsLayoutInRenderController.new get :hello assert_includes @response.body, "layouts/third_party_template_library.mab" diff --git a/actionview/test/template/dependency_tracker_test.rb b/actionview/test/template/dependency_tracker_test.rb index ef7aeac039..42cb14a18a 100644 --- a/actionview/test/template/dependency_tracker_test.rb +++ b/actionview/test/template/dependency_tracker_test.rb @@ -17,8 +17,8 @@ class FakeTemplate end end -Neckbeard = lambda { |template| template.source } -Bowtie = lambda { |template| template.source } +Neckbeard = lambda { |template, source| source } +Bowtie = lambda { |template, source| source } class DependencyTrackerTest < ActionView::TestCase def tracker diff --git a/actionview/test/template/render_test.rb b/actionview/test/template/render_test.rb index 76ffe3415d..f5d251da20 100644 --- a/actionview/test/template/render_test.rb +++ b/actionview/test/template/render_test.rb @@ -450,13 +450,22 @@ module RenderTestCases assert_equal "Hello, World!", @view.render(inline: "Hello, World!", type: :bar) end - CustomHandler = lambda do |template| + CustomHandler = lambda do |template, source| "@output_buffer = ''.dup\n" \ - "@output_buffer << 'source: #{template.source.inspect}'\n" + "@output_buffer << 'source: #{source.inspect}'\n" end def test_render_inline_with_render_from_to_proc - ActionView::Template.register_template_handler :ruby_handler, :source.to_proc + ActionView::Template.register_template_handler :ruby_handler, lambda { |_, source| source } + assert_equal "3", @view.render(inline: "(1 + 2).to_s", type: :ruby_handler) + ensure + ActionView::Template.unregister_template_handler :ruby_handler + end + + def test_render_inline_with_render_from_to_proc_deprecated + assert_deprecated do + ActionView::Template.register_template_handler :ruby_handler, :source.to_proc + end assert_equal "3", @view.render(inline: "(1 + 2).to_s", type: :ruby_handler) ensure ActionView::Template.unregister_template_handler :ruby_handler -- cgit v1.2.3