Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

if the following tag is ['first','last' or 'size] defer to_liquid #145

Closed
wants to merge 1 commit into from

3 participants

@boourns
Admin

@ssoroka @Soleone Please Review /cc @hornairs

Currently, if a liquid user executes collection.products.first, products will be converted to_liquid before the .first is applied.

If the drop provides an ActiveRelation object to the liquid engine, calling .to_liquid on the ActiveRelation will load all the objects, and then throw out all but the first.

If we defer calling .to_liquid until after we've applied .first then ActiveRecord will do the right thing and only load the single product.

This fix is a first-principals approach to solving the problem: if the following tag is in ['first', 'last', 'size'] and we've got an object that responds to the tag, send it before rendering to_liquid.

I feel like there's potential for a bigger refactor here, to recursively apply the following tags and only render .to_liquid when necessary.

@ssoroka

I like where you're going with this, but I think this exposes security holes. A better option might be to pass RelationDrops around that preserve the relation.

@boourns
Admin

@ssoroka Ah I see. Drop#to_liquid returns the drop itself so the relation can be maintained. That's definitely a better approach, will rewrite.

@boourns boourns closed this
@tobi tobi commented on the diff
lib/liquid/context.rb
@@ -204,8 +204,10 @@ def variable(markup)
end
if object = find_variable(first_part)
+ i = 0
+ while i < parts.length
@tobi Admin
tobi added a note

stylisitically this should be a parts.length.times do |i| ... end loop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tobi tobi commented on the diff
lib/liquid/context.rb
((7 lines not shown))
- parts.each do |part|
@tobi Admin
tobi added a note

or even parts.each_with_index do |part, i|

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tobi
Admin

Agreed with @snookca. A better approach would be to add a .to_liquid method for relations that returns a drop.

@fw42 fw42 deleted the branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 12 additions and 2 deletions.
  1. +12 −2 lib/liquid/context.rb
View
14 lib/liquid/context.rb
@@ -204,8 +204,10 @@ def variable(markup)
end
if object = find_variable(first_part)
+ i = 0
+ while i < parts.length
@tobi Admin
tobi added a note

stylisitically this should be a parts.length.times do |i| ... end loop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ part = parts[i]
- parts.each do |part|
@tobi Admin
tobi added a note

or even parts.each_with_index do |part, i|

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
part = resolve($1) if part_resolved = (part =~ square_bracketed)
# If object is a hash- or array-like object we look for the
@@ -216,7 +218,13 @@ def variable(markup)
# if its a proc we will replace the entry with the proc
res = lookup_and_evaluate(object, part)
- object = res.to_liquid
+
+ if i+1 < parts.length && res.respond_to?(next_part = parts[i+1]) && ['size', 'first', 'last'].include?(next_part)
+ object = res.send(next_part.intern).to_liquid
+ i+=1
+ else
+ object = res.to_liquid
+ end
# Some special cases. If the part wasn't in square brackets and
# no key with the same name was found we interpret following calls
@@ -233,6 +241,8 @@ def variable(markup)
# If we are dealing with a drop here we have to
object.context = self if object.respond_to?(:context=)
+
+ i+=1
end
end
Something went wrong with that request. Please try again.