Skip to content
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

Runtime: improve variable parsing and Builder array plucking #5902

Merged
merged 8 commits into from
May 24, 2022
Merged

Runtime: improve variable parsing and Builder array plucking #5902

merged 8 commits into from
May 24, 2022

Conversation

JohnathonKoster
Copy link
Contributor

@JohnathonKoster JohnathonKoster commented Apr 23, 2022

This PR improves variable parsing (which allowed for some cleanup/simplifying of the NodeProcessor 🎉), which made it easier to correct a few issues with array plucking and query builders.

When assets is a Builder, this will now work as expected (the Builder instances are not sent through the query tag since they are treated as arrays)

{{ assets.2 }}
    {{ url }}
{{ /assets.2 }}

{{ assets[some_index] }}
    {{ url }}
{{ /assets[some_index] }}

If the final part of the variable path returned a Builder instance (regardless of how many parts/nested the variable path was), those instances are now sent through the query tag, making the following work:

{{ nested.level_two:level-three.1.assets order_by="field_name:desc" }}
     {{ url }}
{{ /nested.level_two:level-three.1.assets }}

To reproduce the issues resolved:

  • Plucking issue: Create any field type that returns a Builder (assets, terms, etc.) and attempt to use a tag pair from array plucking ({{ field.0 }} {{ /field.0 }}
  • Final Path Issue: Create a nested variable where the final part of the variable returns a Builder ({{ nested.field.builder }} {{ /nested.field.builder }})

More information that lead to this PR is here: #5881

@mikemartin
Copy link

@JohnathonKoster I ran into this issue when using offset and limit on an asset tag, will this PR address those parameters too?

{{ photos offset="1" limit="2" }}
...
{{ /photos }}

@JohnathonKoster
Copy link
Contributor Author

@JohnathonKoster I ran into this issue when using offset and limit on an asset tag, will this PR address those parameters too?

{{ photos offset="1" limit="2" }}
...
{{ /photos }}

That seems like it may be a separate item entirely - I can do some testing on my end to see what may be going on - do you have any additional details about the issue?

Thanks!

@mikemartin
Copy link

Hey @JohnathonKoster
Just that it returns the images in the wrong order.

@JohnathonKoster
Copy link
Contributor Author

@mikemartin Is this the kind of behavior your are seeing/referring to? If so, this is not related (and is consistent across both parser implementations).

behavior_photos

@jasonvarga
Copy link
Member

jasonvarga commented Apr 28, 2022

Assets in the wrong order is a weird one, and it happens to me too, but is separate from this issue 👍

@jasonvarga
Copy link
Member

@mikemartin I'm fixing that in #5381

@jasonvarga jasonvarga merged commit 7297a6c into statamic:3.3 May 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants