Centralizing to_liquid #1532
Centralizing to_liquid #1532
Conversation
| @@ -861,7 +861,7 @@ def test_all_filters_never_raise_non_liquid_exception | |||
| { foo: "bar" }, | |||
| [{ "foo" => "bar" }, { "foo" => 123 }, { "foo" => nil }, { "foo" => true }, { "foo" => ["foo", "bar"] }], | |||
| { 1 => "bar" }, | |||
| ["foo", 123, nil, true, false, Drop, ["foo"], { foo: "bar" }], | |||
There was a problem hiding this comment.
I think passing a Class here is something simply not supported as it cannot be done in a context that runs outside the standard filters.
Before the current changes, you will see a e.respond_to?(:to_liquid) ? e.to_liquid : e in lib/liquid/standardfilters.rb.
Only the StandardFilters apply that respond_to? and I believe it is the user responsibility to always pass in a "Liquid compatible" object back to the context and not for Liquid to silence the unexpected input.
| @@ -582,8 +582,7 @@ def empty? | |||
|
|
|||
| def each | |||
| @input.each do |e| | |||
| e.context = @context if e.respond_to?(:context=) | |||
| yield(e.respond_to?(:to_liquid) ? e.to_liquid : e) | |||
There was a problem hiding this comment.
This removal is core to the goal I am trying to reach.
While this one filter is in the main codebase, any filters built by the gem users currently need to be aware of build around this.
There was a problem hiding this comment.
This e.respond_to?(:to_liquid) check seems clearly broken and seems to_liquid could be done unconditionally without these other changes.
I think at the moment filters should be able to depend on the immediate value being passed in being a liquid value. They just need to use to_liquid conversions for nested values. Even if that weren't the case, we would still have similar concerns in drops, so I don't really see a good way of providing a non-leaking abstraction around this concern at the moment.
tjoyal
force-pushed
the
centralizing-to_liquid
branch
from
8ff59c2
to
ea38871
Compare
| if object.is_a?(Array) | ||
| object = object.map do |obj| | ||
| contextualize(obj) | ||
| end | ||
| elsif object.is_a?(Hash) | ||
| new_obj = {} | ||
| object.map do |k, obj| | ||
| new_obj[k] = contextualize(obj) | ||
| end | ||
| object = new_obj |
There was a problem hiding this comment.
This seems like a non-trivial conversion to be doing on variable access, especially since there wouldn't be anything to make sure it isn't done redundantly. E.g. if an array of hashes were iterated, then this deep conversion would be done when accessing the array along with the inner hashes, then each hash would be converted again when they are accessed.
There was a problem hiding this comment.
This also completely remove the advantage of Proc lazy evaluation.
I'm not sure how much lazy said evaluation is needed when it comes to non-root elements, but it is an impactful mechanism.
WIP
As part of #1205 I am exploring cases of @context missing through rendering.
The following explore:
object.to_liquidandobject.context=as single operation.Procoptimizations are shared.to_liquid.Still a few TODOs to work through and I need to figure out what goes in this PR vs what can be tackled separately (either before or after).