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: Preserve builder instances when being supplied in dynamic bindings and modifiers, or when they've been scoped/nested #5807

Merged
merged 8 commits into from Apr 18, 2022

Conversation

JohnathonKoster
Copy link
Contributor

@JohnathonKoster JohnathonKoster commented Apr 9, 2022

This PR adjusts the behavior of query builder instances to bring parity with Regex:

Builders are left alone when being passed as tag parameters (closes #5811 ):

{{ partial:tags :tags="entries" }}

Builders are left alone in modifiers:

{{ if builder_field | length > 0 }}

{{ /if }}

Example of scoped/nested behavior that previously broken (has to be a tag pair to be broken):

{{ scope_name:builder_field }}

{{ /scope_name:builder_field }}

Additionally, this PR adds extra test coverage to make sure that any resolved query builder values are not leaked in other nested scopes (allowing for changing query parameters, sort order, etc.)

@JohnathonKoster JohnathonKoster changed the title Runtime: builder modifier behavior Runtime: builder modifier and tag behavior Apr 11, 2022
@JohnathonKoster JohnathonKoster changed the title Runtime: builder modifier and tag behavior Runtime: Preserve builder instances when being supplied in dynamic bindings and modifiers Apr 12, 2022
@MariusSpring
Copy link

Awesome!

@JohnathonKoster JohnathonKoster changed the title Runtime: Preserve builder instances when being supplied in dynamic bindings and modifiers Runtime: Preserve builder instances when being supplied in dynamic bindings and modifiers, or when they've been scoped/nested Apr 12, 2022
@jasonvarga
Copy link
Member

The first two fixes are great. But I'm having trouble recreating the last bit. What can I do to reproduce the bug?

I'm talking about this bit:

Example of scoped/nested behavior that previously broken (has to be a tag pair to be broken):

{{ scope_name:builder_field }}

{{ /scope_name:builder_field }}

@JohnathonKoster
Copy link
Contributor Author

JohnathonKoster commented Apr 14, 2022

@jasonvarga This test will fail if you bring it in the main branch:

public function test_query_builder_loops_receive_tag_parameters_and_can_be_scoped()
{
$builder = Mockery::mock(Builder::class);
$builder->shouldReceive('get')->twice()->andReturn(collect([
['title' => 'Foo'],
['title' => 'Baz'],
['title' => 'Bar'],
]));
$builder->shouldReceive('orderBy')->twice()->withArgs(function ($field, $direction) {
return $field == 'title' && $direction == 'desc';
});
$data = [
'block' => [
'taxonomies' => $builder,
],
];
$template = <<<'EOT'
{{ block:taxonomies order_by="title:desc" }}{{ title }}{{ /block:taxonomies }}
EOT;
$this->assertSame('FooBazBar', $this->renderString($template, $data));
$template = <<<'EOT'
{{ block:taxonomies order_by="title:desc" as="entries" }}{{ entries }}{{ title }}{{ /entries }}{{ /block:taxonomies }}
EOT;
$this->assertSame('FooBazBar', $this->renderString($template, $data));
}

I've spotted something else (not hitting the Cascade when it should) while making you a repository 🤦 -- will fix that as well, and then also get you a repo.

@jasonvarga
Copy link
Member

Oh you mean when there are params on it. In your description you didn't:

{{ scope_name:builder_field }}

{{ /scope_name:builder_field }}

@JohnathonKoster JohnathonKoster marked this pull request as draft April 14, 2022 20:11
@JohnathonKoster JohnathonKoster marked this pull request as ready for review April 14, 2022 22:20
@JohnathonKoster
Copy link
Contributor Author

JohnathonKoster commented Apr 14, 2022

@jasonvarga I've created a repository here that contains examples of all the different things:

https://github.com/Stillat/antlers-fun-with-builders

All of them are on the home page (for best results npm install && npm run production -- after fighting with these Builders, decided to have a little fun with the repo)

@jasonvarga
Copy link
Member

Thanks for that!

This is a separate problem, but FYI the home page on that repo loads super slowly (almost 6s, and I'm on an M1 Mac). If I remove all the code chunks (the noparse tag pairs) it loads quick (~360ms).

@jasonvarga
Copy link
Member

If I move the noparse chunks into partials, the page displays correct, and it's fast.

@JohnathonKoster
Copy link
Contributor Author

JohnathonKoster commented Apr 15, 2022

If I move the noparse chunks into partials, the page displays correct, and it's fast.

Thanks for letting me know! I'll look into that and get that fixed up

@jasonvarga
Copy link
Member

Kinda looks a lot to do with the size of the template. If I replace the code chunks with about the same number of regular text characters, the load time jumps up.

Anyway, getting off topic. Thanks again for the repo.

@jasonvarga jasonvarga merged commit dc18738 into statamic:3.3 Apr 18, 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.

Call to a member function getActiveNode() on null (antlers: runtime)
4 participants