-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Nested search in where filter #1436
Conversation
Co-Authored-By: ADTC <[email protected]>
This would be really nice in sort, too. I have a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't bring any breaking changes to Shopify, but could bring breaking changes to other platforms using Liquid, as it won't consider attributes with dots.
I'm not sure if that is actually the case. Doesn't Shopify support JSON metafield types? If the where
filter were used on some parsed json blob, then I would think it could end up filtering on keys with a period in them.
lib/liquid/standardfilters.rb
Outdated
raise_property_error(property) | ||
return [] if ary.empty? | ||
return nil unless ary.first.respond_to?(:[]) | ||
return raise_property_error(property) unless ary.first.is_a?(Hash) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would no longer check all the array elements. Also, this seems more restrictive to have it only work for Hash objects rather than supporting any object that responds to []
as the above line implies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added this check:
is_deep = ary.first.is_a?(Hash) && ary.first.include?(property) == false
Is it good enough to only check the first item? I think that checking all items wouldn't perform too well.
Alternatively, we can do something like:
def where(input, property, *target_values = nil)
And then only search nested if target_values contains 2 or more arguments? I just like the dot notation typing more.
In that case, the |
I don't mean storing JSON in a string type field, I mean there is native support for JSON metafield types. The primary reason for that json type is so the parsed json is made accessible to liquid, meaning it could be arrays of hashes with keys that could have a period in it. |
Got it. Should we cover that case? |
Does it make sense to add this nested feature to other filters, such as |
Opened a new PR which solves this in a way better way: #1749 |
This PR allows nested search in where filter.
Resolves #1388