Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce a file type template #35119

Merged
merged 3 commits into from
Feb 2, 2019
Merged

Introduce a file type template #35119

merged 3 commits into from
Feb 2, 2019

Conversation

tenderlove
Copy link
Member

Every template that specifies a "virtual path" loses the template source
when the template gets compiled:

@source = nil if @virtual_path

The "refresh" method seems to think that the source code for a template
can be recovered if there is a virtual path:

# Receives a view object and return a template similar to self by using @virtual_path.
#
# This method is useful if you have a template object but it does not contain its source
# anymore since it was already compiled. In such cases, all you need to do is to call
# refresh passing in the view object.
#
# Notice this method raises an error if the template to be refreshed does not have a
# virtual path set (true just for inline templates).
def refresh(view)
raise "A template needs to have a virtual path in order to be refreshed" unless @virtual_path
lookup = view.lookup_context
pieces = @virtual_path.split("/")
name = pieces.pop
partial = !!name.sub!(/^_/, "")
lookup.disable_cache do
lookup.find_template(name, [ pieces.join("/") ], partial, @locals)
end
end

Every call site that allocates a template object and provides a
"virtual path" reads the template contents from the filesystem:

contents = File.binread(template)
Template.new(contents, File.expand_path(template), handler,

Templates that are inline or literals don't provide a "virtual path":

Template.new(options[:inline], "inline template", handler, locals: keys)

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:

[ActionView::Template.new("Template generated by Null Resolver", path.virtual, handler, virtual_path: path.virtual, format: format, variant: variant)]

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.

@compiled = true
end
end

class LegacyTemplate < DelegateClass(Template) # :nodoc:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the plan to this class? We will always have a LegacyTemplate or we plan to remove it some day?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove it eventually. I want to change this call to pass in the view and the source. But for backwards compatibility we need to pass an object that has a reference to the mutated source.

I'll follow up with another commit that deprecates arity one blocks

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.
@tenderlove tenderlove changed the title Introduce a file type template, deprecate Template#refresh Introduce a file type template Feb 1, 2019
@tenderlove
Copy link
Member Author

I changed this to not deprecate refresh. I think we might be able to deprecate and remove it in the future, but right now it's not clear. In test, we have a custom resolver that instantiates template objects that have a view path, but aren't associated with a file:

def query(path, exts, _, _)
query = +""
EXTENSIONS.each_key do |ext|
query << "(" << exts[ext].map { |e| e && Regexp.escape(".#{e}") }.join("|") << "|)"
end
query = /^(#{Regexp.escape(path)})#{query}$/
templates = []
@hash.each do |_path, array|
source, updated_at = array
next unless query.match?(_path)
handler, format, variant = extract_handler_and_format_and_variant(_path)
templates << Template.new(source, _path, handler,
virtual_path: path.virtual,
format: format,
variant: variant,
updated_at: updated_at
)
end
templates.sort_by { |t| -t.identifier.match(/^#{query}$/).captures.reject(&:blank?).size }
end

After this PR, the common case will be to instantiate FileTemplate objects, so I just implemented a refresh on FileTemplate that returns itself (since it can get the source).

@rafaelfranca
Copy link
Member

I changed this to not deprecate refresh. I think we might be able to deprecate and remove it in the future, but right now it's not clear. In test, we have a custom resolver that instantiates template objects that have a view path

Ah right. This is something we support and José's book talk about it in the "Retrieving View Templates from Custom Stores" chapter https://pragprog.com/book/jvrails2/crafting-rails-4-applications

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.
@tenderlove tenderlove merged commit e50ed29 into master Feb 2, 2019
@tenderlove tenderlove deleted the file-template branch February 2, 2019 01:25
Edouard-chin added a commit to Shopify/better-html that referenced this pull request Feb 5, 2019
Edouard-chin added a commit to Shopify/better-html that referenced this pull request Feb 5, 2019
- `ActionView::Handlers#call` now needs to accept the `template` and
  `source` as arguments. Change in this patch is backward compatible

- Change upstream rails/rails#35119
@morgoth
Copy link
Member

morgoth commented Mar 1, 2019

While upgrading to Rails 6.0 I'm getting warning:

DEPRECATION WARNING: Single arity template handlers are deprecated.  Template handlers must
now accept two parameters, the view object and the source for the view object.
Change:
  >> Class#call(template)
To:
  >> Class#call(template, source)
 (called from <top (required)> at /home/wojtek/Projects/some_project/config/environment.rb:5)

which is not really clear where to find it. I suspect some gem, as I don't mess with templates in the app.
Do you have a clue how to track it?

cbothner added a commit to cbothner/markerb that referenced this pull request Mar 19, 2019
Single-arity template handlers are deprecated as of Rails 6.
rails/rails#35119
cbothner added a commit to cbothner/markerb that referenced this pull request Mar 19, 2019
Single-arity template handlers are deprecated as of Rails 6.
rails/rails#35119
r7kamura added a commit to r7kamura/hamlit that referenced this pull request Apr 9, 2019
@Sun-Mountain
Copy link

While upgrading to Rails 6.0 I'm getting warning:

DEPRECATION WARNING: Single arity template handlers are deprecated.  Template handlers must
now accept two parameters, the view object and the source for the view object.
Change:
  >> Class#call(template)
To:
  >> Class#call(template, source)
 (called from <top (required)> at /home/wojtek/Projects/some_project/config/environment.rb:5)

which is not really clear where to find it. I suspect some gem, as I don't mess with templates in the app.
Do you have a clue how to track it?

I have the same question. I am now updating our rails app and getting the same message but cannot find anywhere this might affect our app. (We also do not use templates outside of the devise gem.)

@eugeneius
Copy link
Member

Setting config.active_support.deprecation = :raise in config/environments/development.rb and trying to run the application will give you a backtrace that includes the line that's causing the problem. It's most likely due to an older version of the jbuilder or coffee-script gems.

I think the code that tries to extract the culprit from the backtrace could be doing a better job here.

mrpinsky pushed a commit to mrpinsky/markdown-rails that referenced this pull request Aug 2, 2020
voxik added a commit to voxik/ammeter that referenced this pull request Sep 15, 2020
This fixes compatibility with ActionView 6.0.0+, whic deprecated old
style template handlers [[1]].

Fixes alexrothenberg#61.

[1]: rails/rails#35119
bquorning added a commit to zendesk/curlybars that referenced this pull request Jan 4, 2021
See rails/rails#35119 for details about the
change to `Curlybars::TemplateHandler.call`.
bquorning added a commit to zendesk/curlybars that referenced this pull request Jan 4, 2021
See rails/rails#35119 for details about the
change to `Curlybars::TemplateHandler.call`.
bquorning added a commit to zendesk/curlybars that referenced this pull request Jan 4, 2021
See rails/rails#35119 for details about the
change to `Curlybars::TemplateHandler.call`.
bquorning added a commit to zendesk/curlybars that referenced this pull request Jan 5, 2021
See rails/rails#35119 for details about the
change to `Curlybars::TemplateHandler.call`.
michaelzedwards added a commit to michaelzedwards/hamlit that referenced this pull request Feb 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants