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

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

Merged
merged 3 commits into from Feb 28, 2023

Conversation

ggmichaelgo
Copy link
Contributor

@ggmichaelgo ggmichaelgo commented Feb 27, 2023

Problem

When Liquid is used with a FileSystem that allows render and include tags to render a file just by its filename, the error message does not show its actual file path.

setup:

# index.liquid
{% render 'foo' with errors %}

# /some/path/snippets/foo.liquid
{{ foo.standard_error }}

output:

Liquid error (foo line 1): standard error

This becomes an issue when Liquid developers have duplicate filenames in multiple folders, which creates confusion where the error is actually coming from.

Solution

In render and include tag, it will set the context.template with template.name.
The Liquid users can use TemplateFactory to create a Template with a name:

class TemplateFactory
  def for(template_name)
    template = Liquid::Template.new
    template.name = "snippets/" + template_name
    template
  end
end

context = Liquid::Context.build(
  registers: { template_factory: StubTemplateFactory.new }
)

template = Liquid::Template.parse("....", line_numbers: true)
template.render(context)

# renders error message in a format of `Liquid error ({folder}/{filename} line #): error message`
Liquid error (snippets/foo line 1): standard error

@ggmichaelgo ggmichaelgo marked this pull request as ready for review February 27, 2023 15:40
@dylanahsmith
Copy link
Contributor

When Liquid is used with a FileSystem that allows render and include tags to render a file just by its filename, the error message does not show its actual file path.

This is by design, since we wouldn't want to leak the full file path. Liquid is designed to allow users to design the page without given those users full system access, meaning that we do want to limit the path to the subpath that the user has access to.

This becomes an issue when Liquid developers have duplicate filenames in multiple folders, which creates confusion where the error is actually coming from.

Liquid doesn't have the concept of multiple folders, but in practice the partials can end up in a different directory than the template. Once a layout template is introduced, which may be in another folder, then this can easily introduce confusion. As such, it makes sense to have the template name to include the subpath from the root of the directory that the user stores liquid code in. E.g. if they stored their code in a github repo, it could give the path from the root of that repo, as opposed to the subpath for the partials/snippets.

Comment on lines 75 to 76
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.

Comment on lines 20 to 24
partial.name ||= if context.registers[:file_system]&.respond_to?(:actual_template_name)
context.registers[:file_system].actual_template_name(template_name)
else
template_name
end
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 partial.name can be set in the template factory in applications that want to customize this. That way this can just set the default

Suggested change
partial.name ||= if context.registers[:file_system]&.respond_to?(:actual_template_name)
context.registers[:file_system].actual_template_name(template_name)
else
template_name
end
partial.name ||= template_name

begin
context.template_name = template_name
context.partial = true
context.template_name = partial.name if partial.name
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 this can be set unconditionally. The reason why I was saying to make it conditional in Template#render (if we support a template name parse argument) is to preserve the behaviour of any application that sets Context#template_name on the passed in Context.

Suggested change
context.template_name = partial.name if partial.name
context.template_name = partial.name

@@ -76,7 +76,9 @@ 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 = partial.name if partial.name
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
inner_context.template_name = partial.name if partial.name
inner_context.template_name = partial.name

@ggmichaelgo ggmichaelgo merged commit 3ff4170 into master Feb 28, 2023
@ggmichaelgo ggmichaelgo changed the title render error message with actual template path set context's template_name with template's name Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants