Consolidate contextualization of inputs via the context

[tjoyal]

This PR is all over the place, I am sorry for that. I found it hard to separate isolated deliverables as there is a lot of inter-dependency at play.

This PR adds Context#contextualize which acts as a central hub to handle logic regarding transformation of variable into liquid aware representation.

This abstraction handles Proc resolutions when needed, the call to to_liquid and assignment of context via context=.

Changes

Context:

• Pushed contextualization from find_variable to lookup_and_evaluate. No behavior change specific to this class, this will allow for VariableLookup#evaluate to re-use said logic.

VariableLookup:

• Main path can delegate the responsibility of contextualization to Context#lookup_and_evaluate, allowing some of the logic duplication to be removed.
• The "command methods" have to deal with manual contextualization have been updated to the unified code path. Context#contextualize logic properly handles Proc which solves for the newly added tests who would have been failing otherwise. We could look at moving this send call to Context but I opted to limit the changes in the current PR.

StandardFilters:

• There is a bug fix and breaking change due to the removal of respond_to?(:to_liquid). The previous code would have left a possibly unexpected object exposed to the template which could have lead to unforeseen privilege given to the template. Procs were notably not being converted.

Behavior of unhandled Procs

Cases when Proc are not handled sometimes lead to exceptions, but other times they could lead to exposure of internal code structure (eg.: rendered as strings in the template such as"#<Proc:0xXXXXXX /Users/tjoyal/projects/liquid/test/integration/standard_filter_test.rb:485>").

North star

The question of supporting Proc behavior is to be asked.

I think most of that “lazy evaluation” can be achieved with an implementation over Liquid::Drop wrapping the underlying implementation which needs to be delayed.

I’ll need to do some git archeology to learn more as to when it was introduced and if it still have merit to be maintained going forward.

[dylanahsmith]

Behavior of unhandled Procs

Cases when Proc are not handled sometimes lead to exceptions, but other times they could lead to exposure of internal code structure

Where a Proc isn't supported, I would expect to_liquid to at least be called on the Proc, which should lead to an exception. Otherwise, this missing to_liquid call is the bug in the liquid feature, not the missing Proc outside of lookup_and_evaluate.

[dylanahsmith]

Ideally I think it would be better to allow passing the context to drops in Drop#invoke_drop(method_or_key, context), and get rid of context=.

Would that assign the context every time the drop is invoked? I suppose that would avoid having to incur this overhead for other types, although having that overhead on property access on drops still seems undesirable.

Also, we can't just get rid of context= without a deprecation cycle, since that could break liquid extensions that use it. It would also mean that the context would be set more lazily, which makes it less obvious to me whether that could result in other breaking changes.

Instead, I want to get to the point where to_liquid takes a context, so that not only would context setting overhead be isolated to drops, but it would also be isolated to initialization of the drops and avoid having drop instances that are in an invalid state (i.e. more obviously correct code).

[dylanahsmith]

Is context= only useful for drops?

Liquid seems to use duck typing for interactions with Liquid::Drop, rather than checks like .is_a?(Liquid::Drop). I'm not sure if this flexibility is actually used to have something like a Liquid::Drop that doesn't actually inherit from that class. However, I would expect that typically it is only a Liquid::Drop derived class that would use context=.

[macournoyer]

Instead, I want to get to the point where to_liquid takes a context, so that not only would context setting overhead be isolated to drops, but it would also be isolated to initialization of the drops and avoid having drop instances that are in an invalid state (i.e. more obviously correct code).

Ah that'd really good! I totally agree this is much better: to_liquid(context).