Hi @gchtr thanks for this PR, it's on my list for today. For now are you just looking for feedback on those discussion items or should I start a more thorough review for merge?

@jarednova I guess first it would be enough to hear what you think about

• The idea of adding a setup() method for Post and wether you can think of any other implications, e.g. in combination with QueryIterator.
• The idea of adding a PostQuery::merge_default() method, the name of the function and whether there might be a better place for this.
• Whether there are other directions we could take for having a function that mimicks the the_post function in WordPress?

(As usual 🙃) it’s quite a long text, but I like to list my thoughts and how I came to an idea.

I haven’t tested it, so that would be the next step. And then write tests for it. And then I guess you could do a more thorough review. Except if you want to take over from here 😉.

One random opinion:

The suggestion of ways to modify the PostQuery seem a bit strange to me. Why not something like this:

$posts = new Timber\PostQuery( [
    'query' => [
        post_parent' => 0,
    ],
    'reconcile_query' => 'merge', // could also be a boolean flag to replace or merge query, the name of this is less important
] );

This allows PostQuery to have a single array of options which feels a little cleaner and easier to handle.

EDIT: Also I have read this issue a few times but still don't feel like I fully understand it, so feel free to ignore me… but that said, I also feel like the static context_posts or merge_default methods are kind of confusing. The setup method seems like the right way to do this rather than trying to do magic inside static methods.

@gchtr Here are my comments about your PR:

1. Global context

I think the post/s should be in the global context as in WordPress they are also in global scope for each page and there is no neeed to get them explicitly. At the moment I just think the documentation was too unclear about that.

2. Prepare template for the current post

Yes we really need to have this as already mentioned in the discussion at mindkomm/timber-integration-woocommerce#6
What I currently don't like is, that the setup function needs to call explicitely. Is there maybe a way we can do better? I think for a single post the setup function can run immediately when instantiating it. For the iteration it would need to call the setup on current() as you already did and then we would need to reset the query after the last post has been accessed.

I also think that if we have a setup function we will also need a teardown function. Currently the $wp_query->in_the_loop is set to true after the posts have been accessed which I don't like. Thus I am thinking about the teardown method to fix this and reset globals (like $product) global.

What I really like is the $wp_query->in_the_loop = true;. This will fix many incompatibilities with existing plugins.

3. Change the default query

What I don't really get is the merge_default() function, which would be a great function to have btw. But at this time in the rendering flow the global $wp_query query is already completed or am I missing something? This will not take the global query right?

In general great ideas. These features/improvements will be great to have.

EDIT: Also I have read this issue a few times but still don't feel like I fully understand it, so feel free to ignore me… but that said, I also feel like the static context_posts or merge_default methods are kind of confusing. The setup method seems like the right way to do this rather than trying to do magic inside static methods.

Sorry, if this is a little confusing. But there’s a lot going on. I already thought a lot about this. It started out with #1639, which led me to #1766 and then on to this pull request. I didn’t explain everything in this pull request. Probably it’s required to read the linked issue and pull request as well 🙈. Additionally, #1771 is a side issue that plays into this, too.

That’s why your comments are truly appreciated! We need to be on the same page for this. I can’t decide alone how Timber should work. I know, my comments are quite long at times, but I like to list the options that we could choose and think about the pros and cons.

To explain a bit more, I’m gonna start with point 2 and come back to point 1 about the global context in the end.

2. Prepare template for current post

A teardown function

I also think that if we have a setup function we will also need a teardown function. Currently the $wp_query->in_the_loop is set to true after the posts have been accessed which I don't like. Thus I am thinking about the teardown method to fix this and reset globals (like $product) global.

True! I thought about a reset/teardown function after queries in #1771, but not for having it on a post itself. Great point! I think a teardown function would be great to reset all the data that was set up in the setup function.

Automatic setup

What I currently don't like is that the setup function needs to call explicitly. Is there maybe a way we can do better? I think for a single post the setup function can run immediately when instantiating it.

Having a call on post instantiation is the thing I tried to avoid after gathering all weird issues about this in #1639.

Consider the following example:

$context = Timber::context();

$context['post']        = new Timber\Post();
$context['myotherpost'] = new Timber\Post( 744 );

Timber::render( 'single.twig', $context );

In this example, when post.twig is rendered and Timber runs the setup function automatically, for which post would the template be set up? It would be set up for myotherpost, because it runs afterwards, which may not be what you want, when you want post to be your main post of the template.

As long as you’re not in the template to display something, I think there’s no use in setting up a post and calling hooks, because every new instantiation would just overwrite the one before it. This includes the functions that can be used in Twig: {{ Post() }}, {{ Image() }}, etc. Consider the example where you’d have your logo saved as an ID and convert it to a Timber\Image just when you use it:

<img src="{{ Image(my_global_logo_id).src }}">

This would set up the "context" for the content to follow. I myself try to avoid this already. But it’s possible in Timber and developers use it, so we need to consider this as well.

I guess a teardown function wouldn’t change the problem. A teardown function is great for loops over Timber posts, but doesn’t help in the context of a singular templates. Tell me if I’m missing something!

Call setup for singular templates

For the reasons listed above I still think that for singular templates, we have to tell Timber manually which post it should use as the "main post". I’m again listing the options I came up with:

a) Solve it through context

This is practically the functionality I initially had in mind in #1766. The guide for how this solution would be used can be found here.

The idea is that you pass the post you want to setup to the context. By default, it would take $post global and make post available in the context:

$context = Timber::context();

var_dump( $context['post'] ); // Outputs the $post global

If you’d want to set a different post, you could use:

$context = Timber::context( array(
    'post' => new Timber\Post(),
) );

Internally, the Timber::context() function would call Timber::context_post(). That function would be a wrapper that calls setup() on a post. It could also be called manually:

// Use the $post global.
$context['post'] = Timber::context_post();

// Use a custom post.
$context['post'] = Timber::context_post( new Timber\Post( 85 ) );

Maybe we would need to consider different names instead of Timber::context_post(). Maybe Timber::context_singular() would be clearer?

b) Use the $post global

Coming back to what I said above:

If you’d want to set a different post, you could use:

$context = Timber::context( array(
   'post' => new Timber\Post(),
) );

When I think about this again, I’m not so sure anymore if we ever need this, or if this is just an edge case that we don’t really have to cover.

@pascalknecht said:

I think the post/s should be in the global context as in WordPress they are also in global scope for each page and there is no neeed to get them explicitly.

Because that’s right, singular pages already have a $post global they work with. If somebody wanted to switch out the $post global, they should do it through some hook, but it might not be something that Timber has to handle.

So for singular templates, it could work like this:

$context = Timber::context();

Here, post will be automatically set in the context. Timber will use the $post global to set up a new Timber\Post and call its setup() method in the back.

The only thing we could do in addition to that is to make it possible to disable post in the context. But then, a developer is on his own with compatibility.

$context = Timber::context( array( 
    'post' => false,
) );

c) Use a setup wrapper

Another option I see is a function that wouldn’t return anything, but would call the setup function for the post you pass in.

Timber::the_post( $post );
Timber::render( 'single.twig', $post );

If we try to find a name for «You need to tell Timber which post it should use as the main post, it could also be called Timber::main_post(). But probably we don’t want to introduce more static methods (thanks for point this out, @chrisvanpatten).

d) Call the setup function on a post explicitly

$post = new Timber\Post();
$post->setup();

$context['post'] = $post;

Because the setup() function would return the post again, it could also be shortened:

$post = new Timber\Post();
$context['post'] = $post->setup();

// Or even this, for seasoned developers.
$context['post'] = ( new Timber\Post() )->setup();

This would be the most direct approach that doesn’t try to hide the setup() function in a wrapper function. And it wouldn’t introduce more static functions, like @chrisvanpatten mentioned.

Archive templates and post loops

For the iteration it would need to call the setup on current() as you already did and then we would need to reset the query after the last post has been accessed.

This is also what I’m looking for in #1771. How can we automate this? Can we have a hook or a function that is called after a loop without having to manually call it?

3. Change the default query

Sorry about the confusion on this one. This point is not really about improving compatibility with WordPress and third party plugins, but about the context and about the main query that is used in a template. In #1766, I explored how we could improve the context and I figured it would be good to have a function to adapt the default query that WordPress prepares for a template, before that query is used to fetch posts.

But at this time in the rendering flow the global $wp_query query is already completed or am I missing something? This will not take the global query right?

When we are in a template, no posts are fetched yet. Posts are fetched when we create a new PostQuery(). When we don’t pass any parameters into it, it will use the default query for the current template. As soon as we pass arguments into PostQuery(), it will overwrite the default query.

There’s no easy method to merge arguments that are passed into PostQuery with the default query. That’s why I proposed merge_default().

In the current version, PostQuery works like this:

$posts = new Timber\PostQuery( $query_args, $post_class = 'Timber\Post' );

I thought about a pattern like this:

$posts = new Timber\PostQuery( $query_args, $options );

• The $options arg could be filled with variables that affect the query passed in the first parameter.
• The $options arg would also the take a post_class parameter, where you could pass in the class that Timber should use to create posts.

$posts = new Timber\PostQuery( array( 
    'post_parent' => 0,
), array(
    'merge_default' => true,
) );

@chrisvanpatten You proposed to use a pattern like this:

$posts = new Timber\PostQuery( $args );

This would mean that PostQuery would only accept one parameter, right?

If we wanted to merge in the default query and pass a custom post class, it would look like this, right? Because merge_default, or reconcile_query would both be options for the query that should live on the same level.

$posts = new Timber\PostQuery( array(
    'query' => array(
        'post_parent' => 0
    ),
    'merge_default' => true, // Or whatever name we choose.
    'post_class' => 'Timber\Post',
) );

Of course this will work too, yet I see that setting up a query that uses the default options would always require you to wrap it in a query array and require you to write more:

$posts = new Timber\PostQuery( array(
    'query' => array(
        'post_type' => 'wow',
        'posts_per_page' => -1,
        'post_status' => 'publish',
    )
) );

Backwards compatibility could work. We’d have to check if a query is present.

If we take the perspective that for 2.x, we try to eliminate more static functions, then Timber\PostQuery::merge_default() is not really a good solution to be honest 😅.

Solve it through context

With what I said above about using the $post global in 2. b), maybe we don’t need to solve this through the PostQuery() or a merge_default() function. We might use PostQuery() quite often, but the only place we ever need it is for the main query in archive templates.

Here’s a new idea: What if archive templates always set posts in the context and take the default query to fetch posts? When you pass in a posts parameter to Timber::context(), you have two options:

• Pass in query arguments to change the default query by default. No need to use cancel_default_query or other keywords.
• If you don’t want posts in the context, you could pass in false. You could then write your own query.

// posts will be set in context using the default query.
$context = Timber::context();

// The passed arguments will be merged with the default query.
$context = Timber::context( array(
    'posts' => array(
        'post_parent' => 0,
    ),
) );

// Set posts to false to create your own query
$context = Timber::context( array(
    'posts' => false
) );

$context['posts'] = new Timber\PostQuery( $my_own_args );

1. Global context

The solution for having dynamic template contexts was proposed in #1766. I didn’t like it for the reasons mentioned right at the beginning of the pull request you’re reading now. In this comment, I’ve explored more options, which in the end could affect the global context.

Summary

Here’s what I prefer now:

• Timber::context() will accept an argument array, containing post or posts.
• For singular templates, post will be automatically set when Timber::context() is called (Option 2. b)).
  • It’s possible to disable this by setting post to false.
• For archive templates, posts will be automatically set using the default query when Timber::context() is called.
  • It’s possible to change the default query by passing query arguments to the posts argument in Timber::context().
  • It’s possible to disable the default query by setting posts to false.
• Have a setup() method for Timber\Post.
  • It can be used by Timber::context() to setup the post for the current template.
  • This will make it more versatile to use it in post loops.

I hope it’s a little clearer now.

Maybe sometimes I’m trying to bring in too many edge cases. But I’ve seen so many questions on Stackoverflow and in the issues of this repo that I’d like to have a solution that is versatile, easy to understand and easy to read.

What do you think?

@gchtr I wouldn't remote the PostsIterator as it makes sense to have it. The fact that you included loop code in the setup method says that one such class would still be good to have to not make Timber\Post responsible for too many things.
Here is my current implementation (not tested yet):

class PostsIterator extends \ArrayIterator {
	/**
	 * Calls the setup function of the current post
	 * to setup post data
	 *
	 * @return mixed
	 */
	public function current() {
		// Fire action when the loop has just started.
		if ( 0 === $this->key() ) {
			do_action_ref_array( 'loop_start', array( &$GLOBALS['wp_query'] ) );
		}
		$post = parent::current();
                $post->setup();
                return $post;
	}

	public function next()
	{
		$post = $this->current();
		$post->teardown();
		// Fire action when the loop has ended.
               // Maybe there is a better solution for this if
		if ( $this->count() === $this->key() ) {
			do_action_ref_array( 'loop_end', array( &$GLOBALS['wp_query'] ) );
		}
		parent::next();
	}
}

@gchtr I think the "Here’s what I prefer now" section of your last post there seems like a good compromise!

The one thing I would reiterate from my original comment (and this is not specifically referencing any of your examples or anything, just a general observation) is to be careful of too much magic.

I think explicitness is always better, even if it means a extra lines of code, extra characters, etc. I love Timber but think it sometimes errs on the side of being a little too clever and magical.

Yes, explicitness requires developers to do a little more work, but I think it promotes a better understanding of how Timber interacts with WordPress and better development patterns. I called out static methods in particular, but I think it's worth thinking about in all situations.

That's one reason I like the explicitness of setting a 'query' item inside a single $args array, rather than separate method arguments: it makes it clear exactly what you're passing in, and the purpose of the code. (This would be a moot point if PHP supported named method parameters, but sadly it does not.)

Really excited to see this come together and thanks for tolerating my philosophical rambling :)

I wouldn’t remove PostsIterator either. I think putting the loop_start and loop_end hooks into the PostsIterator class is a really clean solution.

And it looks like @pascalknecht found the solution for #1771 🎉. Thanks! We can also put wp_reset_postdata() into the next() function, so that the loop will be properly reset. I added two small tests, and it seems to be working.

@chrisvanpatten Well, it looks like we’re on the same page here!

I think explicitness is always better, even if it means a extra lines of code, extra characters, etc. I love Timber but think it sometimes errs on the side of being a little too clever and magical.

Yes, explicitness requires developers to do a little more work, but I think it promotes a better understanding of how Timber interacts with WordPress and better development patterns. I called out static methods in particular, but I think it's worth thinking about in all situations.

Yes! Totally agree! I’m probably guilty of trying to add some sprinkles of magic here and there. You make a really solid argument for having only one array for PostQuery. But it’s cool that we wouldn’t need this now. Except that we do if we won’t add the little "magic" to the context. And by magic I mean setting posts or post by default. This question maybe sits a little in-between, because we could make Timber behave more like WordPress by default, or leave it open and give developers the choice whether they follow the standards. That’s where Timber also shines: it’s versatile!

Except that we do if we won’t add the little "magic" to the context. And by magic I mean setting posts or post by default.

This is actually a good example of what I think is bad magic, but I'm also the type of person who gets frustrated that WordPress does this ;-) Generally my take is that it's better to ask developers to be specific about the type of data they want passed into the Twig template. It keeps the core pretty minimal and allows you to more easily use your own objects extending from core classes (or replace them entirely). It also means less of an ongoing burden to maintain compatibility for the various types of data inside context… because once something goes Into context, it will be really hard to pull out without breaking a lot of things.

@gchtr I'm just catching up on all the back-and-forth I missed. I think you, me, @chrisvanpatten and @pascalknecht all are on the same page philosophy-wise. Let me know when you're ready for me to take a full in-depth review of the code/tests

@gchtr what should we do with $posts_iterator = apply_filters('timber/class/posts_iterator', $posts_iterator, $returned_posts, $post_class); then? It will probably not be needed after these changes.

Generally my take is that it's better to ask developers to be specific about the type of data they want passed into the Twig template. It keeps the core pretty minimal and allows you to more easily use your own objects extending from core classes (or replace them entirely).

True that! Extensibility is a good point as well. Though for Timber, I guess there are also a lot of developers who use it without much of their own OOP coding. A cool thing about Timber is that it has a low entry level, but allows to bring in your own complexity.

It also means less of an ongoing burden to maintain compatibility for the various types of data inside context… because once something goes Into context, it will be really hard to pull out without breaking a lot of things.

Definitely! The thing here is that currently posts already exists in the context, but for every template and not just for archive templates. This wasn’t documented very well yet. So we can either improve this or break it with a major version of Timber.

what should we do with $posts_iterator = apply_filters('timber/class/posts_iterator', $posts_iterator, $returned_posts, $post_class); then? It will probably not be needed after these changes.

@pascalknecht Good catch! No, we wouldn’t need the timber/class/posts_iterator filter, because most of what’s needed can be changed through the setup() function in the post class or an extended post class. At least for the WooCommerce integration. But I see no reason why it shouldn’t work for other use cases.

I pulled in Pascal’s pull request.

Let me know when you're ready for me to take a full in-depth review of the code/tests

@jarednova I guess we’re still trying to find the best conceptual solution before we dig deep on tests. But if you have any input on that, it would help as well!

I think the biggest question we should solve first is: How do we setup a post for a singular template? How and when do we call a post’s setup() function? Should it be automatic? Should it be explicit? I’ve explored some options in #1778 (comment), but I’m still not quite happy and not sold on anything yet. Any other ideas?

Changes

Here are the changes since my last comment:

• Added context_global() function to get the global context.
• Added context_post() function to get post context for singular contexts and added call to Post::setup() inside that function.
• Added context_posts() function to get posts context for archive contexts.
• Updated Timber\PostQuery() to take an argument array instead of two parameters.

Tests

• Removed deprecated tests.
• Updated existing tests with new PostQuery argument array.
• Added new test to check if merge_default works for PostQuery.
• Added new test to check whether there’s a proper deprecation message when using the old parameter style for PostQuery.

Explanation

Here’s the current behavior:

• The context will set post for singular templates and call the setup() function on a post.
• The context will set posts for archive templates.
• If you want to overwrite something in the context, you can do that explicitly. If you overwrite post in the context, you have to call setup() manually.
• If you want to merge your query arguments with the default query, you can do that through the merge_default argument for PostQuery.
• If you want to use the context inside a partial, in a shortcode for example, it’s probably better to use Timber\context_global().

Automatic template contexts

I thought again about not adding too much magic. I kinda liked the idea to not introduce parameters for Timber::context(). I didn’t check this properly before, but WordPress already runs queries for archive templates and has the $post global ready for singular templates. Setting up posts or post in the context for the current template shouldn’t create new database queries. It should be cheap to do that. And the best benefit of this is that for singular templates, we can call the setup() method on a post inside Timber::context_post(). I talked with @pascalknecht about that, and we didn’t see any easier solution. If somebody still wanted to use something different for posts or post, they could do that.

For singular templates, using a custom class would mean that you have to call the setup() function yourself.

// Using an extended class
$context = Timber::context();

$post = new Extended_Post();
$post->setup();

$context['post'] = $post;

// Or very short
$context['post'] = ( new Extended_Post() )->setup();

For archive templates, they can write a new PostQuery.

// Using a custom query
$context          = Timber::context();
$context['posts'] = new Timber\PostQuery( array(
    'query' => array(
        'posts_per_page' => 3,
    )
) );

// Merge with default query
$context          = Timber::context();
$context['posts'] = new Timber\PostQuery( array(
    'query' => array(
        'posts_per_page' => 3,
    ),
    'merge_default' => true,
) );

Here I added the argument style for PostQuery that @chrisvanpatten proposed. I think it‘s quite easy to read and to see what’s going on. And I’m a big fan of argument arrays myself, used in combination with wp_parse_args(). Using an array also has the benefit that if you only want to the change the post class that is used, you don’t have to pass in a first parameter.

A question I had was: How can I change the post class that Timber will use when I can’t pass in arguments to the context?

// Using a custom post class
$context          = Timber::context();
$context['posts'] = new Timber\PostQuery( array(
    'query' => array(
        'posts_per_page' => 3,
    ),
    'post_class' => 'Book',
) );

The recommended way would probably still be use the timber/post/post_class filter and add a class map there.

Performance

In any case where you run a new Timber\PostQuery with query arguments or create a new Timber\Post() with an argument, data will be fetched from the database. It’s always a performance hit.

As long as you don’t change any query parameters, no additional database query will be run.

Considerations

• @chrisvanpatten proposed to use reconcile_query as the keyword for «merging in the default query». As a non-native English speaker, I have to say that the term «reconcile» might be advanced vocabulary that I fear will be hard to understand. But I’m open to suggestions for better wording.
• Do we need a filter to disable automatic template contexts?

TODO

• Update Context guide.
• Update documentation for using PostQuery in Twig.

Apart from the documentation, I’d say this pull request is "ready". It’s the best I have now. Suggestions are still very welcome.

@gchtr Just had a look at the code. It looks really good and clean.
One last thing: I don't understand why we need an extra context_post method. Can't we do that based on the $wp_query global? We could keep the API smaller like that. I will try to open a PR.

@pascalknecht We could easily put that into context(), indeed! In an earlier version, I had much more logic in there, but I see now that separate methods do not bring much benefit.

I updated the context guide to reflect the current implementation. I removed the todo for documenting PostQuery in Twig and open a separate pull request for that.

From my side, this pull request is finished. At least for the current state in 2.x. Depending on what happens with object factories we maybe have to update the context again.

Awesome! I'll give things a final pass and merge

Coverage increased (+0.05%) to 93.579% when pulling 069cf97 on 2.x-context-revision into 966afa5 on 2.x.

@jarednova Thanks for taking a detailed look at the context guide as well as the rest of the code. What do you think, is this ready to merge now?

'tis! Let's make it happen