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.

Sign up for GitHub

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

Jump to bottom

set context's template_name with template's name #1692

Merged
merged 3 commits into from
Feb 28, 2023
Merged

set context's template_name with template's name #1692

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0fe4a5d
render error message with actual template path
ggmichaelgo Feb 27, 2023
428c66f
add name attribute to Template for more flexibility to set context's …
ggmichaelgo Feb 27, 2023
24dceef
set context's template_name with template's name
ggmichaelgo Feb 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 8 additions & 2 deletions lib/liquid/tags/include.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,15 @@ def render_to_output_buffer(context, output)

old_template_name = context.template_name
old_partial = context.partial

begin
context.template_name = template_name
context.partial = true
context.template_name = if Template.file_system.respond_to?(:actual_template_name)
Template.file_system.actual_template_name(template_name).delete_suffix('.liquid')
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the .delete_suffix('.liquid') part could be handled in the file system itself.

Also, it would be preferable for this to be cached as part of the PartialCache.load. In fact, it would be more natural to give the Liquid::Template an attr_accessor :name, which could be set when loading the template from a template factory. PartialCache.load could then set template.name ||= template_name to preserve this existing behaviour.

Copy link
Contributor

Choose a reason for hiding this comment

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

The other reason having a Template#name property would be useful is that it could be used as part of given a name to the initial template. I think we could provide that as a parse option, then Template#render can set context.template_name = template.name if template.name. I think that would make it easy to set the top-level template name so that it can be included in error messages.

else
template_name
end

context.partial = true
context.stack do
@attributes.each do |key, value|
context[key] = context.evaluate(value)
Expand Down
8 changes: 7 additions & 1 deletion lib/liquid/tags/render.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,13 @@ def render_tag(context, output)

render_partial_func = ->(var, forloop) {
inner_context = context.new_isolated_subcontext
inner_context.template_name = template_name

inner_context.template_name = if Template.file_system.respond_to?(:actual_template_name)
Template.file_system.actual_template_name(template_name).delete_suffix('.liquid')
else
template_name
end

inner_context.partial = true
inner_context['forloop'] = forloop if forloop

Expand Down
13 changes: 13 additions & 0 deletions test/integration/tags/include_tag_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,4 +356,17 @@ def test_break_through_include
partials: { 'break' => "{% break %}" },
)
end

def test_include_tag_renders_actual_template_name_for_error
original_file_system = Liquid::Template.file_system

Liquid::Template.file_system = MemoryFileSystem.new(
'/some/path/snippets/foo.liquid' => "{{ foo.standard_error }}",
)

template = Liquid::Template.parse("{% include 'foo' with errors %}", line_numbers: true)
assert_equal('Liquid error (/some/path/snippets/foo line 1): standard error', template.render('errors' => ErrorDrop.new))
ensure
Liquid::Template.file_system = original_file_system
end
end # IncludeTagTest
13 changes: 13 additions & 0 deletions test/integration/tags/render_tag_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,4 +264,17 @@ def test_render_tag_with_drop
},
)
end

def test_render_tag_renders_actual_template_name_for_error
original_file_system = Liquid::Template.file_system

Liquid::Template.file_system = MemoryFileSystem.new(
'/some/path/snippets/foo.liquid' => "{{ foo.standard_error }}",
)

template = Liquid::Template.parse("{% render 'foo' with errors %}", line_numbers: true)
assert_equal('Liquid error (/some/path/snippets/foo line 1): standard error', template.render('errors' => ErrorDrop.new))
ensure
Liquid::Template.file_system = original_file_system
end
end
22 changes: 22 additions & 0 deletions test/test_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,3 +214,25 @@ def for(_template_name)
Liquid::Template.new
end
end

class MemoryFileSystem
def initialize(values)
# values is a hash of template_path => template_source
@snippets = {}
values.each do |file_path, source|
key = file_path.split('/').last.delete_suffix(".liquid")
@snippets[key] = {
source: source,
actual_template_name: file_path,
}
end
end

def read_template_file(template_name)
@snippets[template_name][:source]
end

def actual_template_name(template_name)
@snippets[template_name][:actual_template_name]
end
end