Execution paths missing @context

General

I've been struggling for some time with certain code paths in my application where the @context within Liquid::Drop is nil and finally decided to deep dive.

In liquid, when the original object is retrieved from the vm environment, context is applied properly:

https://github.com/Shopify/liquid/blob/57c9cf64ebc777fe5e92d4408d31a911f087eeb4/lib/liquid/variable_lookup.rb#L67-L68
https://github.com/Shopify/liquid/blob/57c9cf64ebc777fe5e92d4408d31a911f087eeb4/lib/liquid/context.rb#L191-L192

The @context, is in these cases, used as a global variable to alter behaviour based on the current user for which the template is rendered. This is not always great, but it is what it is it purpose.

Example

require 'liquid'

class BaseDrop < Liquid::Drop
  def initialize(var)
    @var = var
  end

  def value
    if @context.registers.fetch(:upcase)
      @var.upcase
    else
      @var
    end
  rescue StandardError => e
    e.to_s
  end
  alias_method :to_s, :value
end

environments = {
  "single" =>  BaseDrop.new("aaa"),
  "array" => [
    BaseDrop.new("aaa"),
    BaseDrop.new("bbb"),
    BaseDrop.new("ccc"),
  ]
}
registers = {
  upcase: true,
}
context = Liquid::Context.new(environments, {}, registers)

template = 'Result: {{ single | json }}'
Liquid::Template.parse(template).render!(context)
#=> "Result: AAA"

template = 'Result: {{ array | json }}'
Liquid::Template.parse(template).render!(context)
#=> "Result: undefined method `registers' for nil:NilClassundefined method `registers' for nil:NilClassundefined method `registers' for nil:NilClass"

Propose mitigation

Now, I couldn't come up with something great to get around this, maybe just few step to make it less painful.

I see this as a multiple issues.

A) lib/liquid/extensions.rb patches Array#to_liquid. It does not iterate to perform to_liquid and assignation of @context to all the elements. This leaves "non liquid" object when the contract in my opinion should have been that to_liquid is expected to do just that.

B) When using filters, they are pure ruby and can do what they want. I do not think there is much we can do here as the purpose of filters is to enable pretty much any implementation. The only thing I think can be done here is to have filters developers to respect the liquid mentality and when interacting with objects beyond their method inputs to carefully consider if they need to invoke to_liquid on them, and if doing so, they need to attach the obj.context= @context on them.

C) It is too easy to miss appending the context= on liquid drops. Lots of places in my code end up doing to_liquid per the recommendation in B so we do not end up with some code path using one representation while others to have another. Still a lot of cases miss that simply calling to_liquid does not carry over the correct context, sometimes leading to unexpected behaviours (exceptions when invoking method on a nil @context).

While I don't have a suggestion for B, for A and C I think something can be done. I'd like to propose a new mandatory argument to the well known to_liquid which would be the @context (obj.to_liquid(@context)). I couldn't find a clear reason why these 2 operations aren't grouped together. I would even go to say that the context should be part of the initializer of Liquid::Drop, ensuring it presence at all time.

If it was to be built from scratch, I think it would be a good idea. Now it is realistic to do such a large change I am uncertain. I really like the ideas of drops to be initialized with all their needs and to have them become immutable.

A more radical proposition could be to no longer have access to the context in Liquid::Drop and use filters to mutate behaviour in all cases instead. That seems it would require a larger breaking change, might not acceptable. It could still be made an opt-in option. Removing context= from Liquid::Drop and asking those that want to maintain the behaviour to add it back in their drop would work. My worry with this proposition is that as long that it is possible, people will be using it as it is such a powerful tool and there is no point in removing it unless there is a better alternative.

If we desire to avoid changing core signatures, I think we could settle for fixing Array#liquid. Does not really make it clear to developers when they convert an object to a drop via to_liquid or simply instantiating a new drop manually that they need to care about assigning the context manually afterward. We would also need to see if we can make the existing code context assignation logic to iterate over iterable objects in general .

I would really like to at least have to_liquid(context) as this would make it clear that it's a parameter that ideally shouldn't be forgotten.

Thoughts? Rocks? Would love some options.

Start by reading lib/liquid/extensions.rb, Liquid::Drop, and the context assignment paths referenced in variable_lookup.rb and context.rb. Reproduce the single-object and array examples, then inspect existing tests for to_liquid, Array handling, and Drop context behavior. Done would require an agreed, backward-compatible design and tests covering every affected execution path.