ORIGINAL COMMENT FROM @gchtr 2020-08-17 #2310 (comment)

Up until here, what you write makes sense to me. But I also have to say that the exact inner workings of all the classes that are in involved here were always a bit hard to keep track of for me. So if we can reduce the complexity here, that would be great 👍.

* I _think_ we can do away with the array wrapping the `WP_Query` in an array (i.e. `[ 'query' => $wp_query ]`) when passing to the constructor. The `post_class` option is a precursor to the new Class Maps design and the `merge_default` seems like something that would better be implemented as an option to `Timber::get_posts()`.

Yes! I think I already partially worked on this in #2145. Maybe you could build on that.

* As a nice bonus, we can save on performance and insantiate Posts inside of collection lazily, meaning while we already have the `WP_Post` objects they wrap, the `Timber\Post` instances themselves are not actually created until they're needed. See `PostsIterator` for how this works.

Doesn’t this have big implications on caching if the actual work is only done after fetching cached posts in PHP templates during Timber::render() or earlier?

And then this is probably also related to handling posts in multisite environments (see #1434 and #2207).

And from what I see in issues and StackOverflow questions, a lot of developers are relying on dumping post objects to see what’s in there. This of course wouldn’t be reliable anymore. I’m not saying I wouldn’t add lazy-loading, but we need to check what it means for other parts of Timber and probably add documentation about this in various places.

A related example: we already fetch meta values lazily. A lot of developers were used to work with {{ dump(post.custom) }} to see what’s there. Now, this won’t work anymore. See https://timber.github.io/docs/v2/guides/custom-fields/#accessing-all-meta-values.

If I'm missing something here and we need this class after all, please let me know! But if we can move out the logic we want to keep I'm hoping we can just get rid of it.

I’m all in favor of simplifying this. What I figured when doing more complex stuff with iterators is that as soon as we have factories ready, we can move some logic regarding globals into Post::setup() method.

One specific example is this class: https://github.com/mindkomm/timber-integration-woocommerce/blob/master/lib/ProductsIterator.php. We discussed this in mindkomm/timber-integration-woocommerce#6. With factories, we can get rid of it completley and a WooCommerce product object might solve WooCommerce globals like this:

class Product extends Timber\Post {
    public function setup( $loop_index = 0 ) {
        parent::setup( $loop_index );

        wc_setup_product_data( $this->ID );

        return $this;
    }
}		

</ORIGINAL COMMENT>

I think I already partially worked on this in #2145. Maybe you could build on that.

Awesome! I will take a look there, thanks!

Doesn’t this have big implications on caching if the actual work is only done after fetching cached posts in PHP templates during Timber::render() or earlier?

I don't think I understand what you mean. :) Which code are you referring to as "fetching cached posts," and how is that affected by this change?

I agree we'd want to document the effects of the change if it actually affects the behavior of the API. But as far as I can tell this will be completely transparent to the caller. That is, by the time you have a post var within a Twig template, you've already done all the work that's needed. And, to be clear, this only affects how Posts are instantiated in PostQuery for now; in every other context they're instantiated eagerly. So I'm not sure why {{ dump(post.custom) }} wouldn't work.

Again, maybe I'm missing something...wouldn't be the first time. :) I'm also cool with being cautious and reverting the lazy instantiation piece for now. If I'm right and it's truly transparent, that means we can roll it out after 2.0 lands.

I’m all in favor of simplifying this. What I figured when doing more complex stuff with iterators is that as soon as we have factories ready, we can move some logic regarding globals into Post::setup() method.

Makes sense to me! Good idea.

I don't think I understand what you mean. :) Which code are you referring to as "fetching cached posts," and how is that affected by this change?

I think the behavior is not really affected by this change. I realize it’s probably something related to Timber in general.

$posts = Timber::get_posts( [
    'post_type' => 'book',
] );

var_dump( $posts );

If we dump the collection, this might not contain all the data we need for posts, because posts didn’t run through a factory yet. For example, I couldn’t check here whether the correct Post class from the class maps was used.

And about the cache. Imagine a custom cache mechanism:

if ( false = get_posts_from_cache() ) {
    $posts = Timber::get_posts( [
        'post_type' => 'book',
    ] );

	set_cache_for_posts( $posts );
}

This will cache the results from the query, but not the posts themselves, because they didn’t run throught a factory yet. Caching only the query might not be want you want. Maybe you want to cache all the data. This apparently is no problem when using Timber::render(), because there, we cache the output and not the data.

Another example is maybe if you want to use post data in JavaScript:

$posts = Timber::get_posts( [
    'post_type' => 'book',
] );

// Don’t do this.
$posts_json = wp_json_encode( $posts );

// Do this.
$posts_json = [];

foreach ( $posts as $post ) {
    $posts_json[] = [
        'ID'   => $post->ID,
        'link' => $post->link(),
        // And other data you need…
    ];
}

$posts_json = wp_json_encode( $posts_json );

That is all related to issues like #2207.

Again, maybe I'm missing something...wouldn't be the first time. :) I'm also cool with being cautious and reverting the lazy instantiation piece for now. If I'm right and it's truly transparent, that means we can roll it out after 2.0 lands.

I also want to confirm I’m not missing something :D. I guess I’m merely thinking out loud and I should handle this in a different issue, which is about documentation, because it has effects which cause issues many developers have run into. Let’s keep it lazy!

I will have a look at the rest later today.

Coverage decreased (-0.6%) to 89.507% when pulling 0985f82 on 2.x-post-collections into b0d086a on 2.x-factories.

@gchtr gotcha. I looked into this and you're right about {{ dump(posts) }} (plural) - it will be the uninstantiated version. I assumed it would implicitly iterate over the internal array but I was wrong. We should be able to override this behavior by implementing a custom __debugInfo() method. Unfortunately, due to a bug in PHP <= 7.3, this is not possible when extending SPL classes like ArrayObject. So we should definitely document this somewhere.

The good news is that JSON serialization and on-demand eager instantiation are both easy to implement. See latest updates, particularly the realize() method. I'm borrowing terminology from Clojure, which has various flavors of laziness built in. Dumping individual posts when iterating or indexing directly (i.e. $coll[0]) will perform as expected because those instances are realized.

Quick aside about your multisite example - it got me thinking so I wrote up #2312 .

Feeling much better about the state of this PR now. I'd still like to get @jarednova's input on the general direction, but for now my only remaining question is: should I also implement laziness on PostArrayObject? It would be pretty simple, and the biggest hurdle is just implementing a custom ArrayIterator that does the same thing as PostsIterator minus the WP loop stuff. Unless you see a reason not to?

@gchtr @jarednova I think I was mistaken and we actually do want to use exactly the same logic from PostsIterator for both kinds of Post Collections, to abstract over The Loop. Right?? This is how PostCollection currently works. What's different between the two is mainly pagination and origin (WP_Query vs a vanilla array).

If that's the case, it's good news because it means there's even less to implement, and the latest few commits should round this out (minus any outstanding details)!

I really really like the way we’re going here 🎉

One path worth considering here is whether using the WP_DEBUG flag is helpful to assist with outputting as much useful data as possible in var_dump() while not compromising on performance when a site goes live.

IIRC the dump Twig function only works if WP_DEBUG is falsey? Or are you talking about explicitly realizing posts internally if WP_DEBUG is truthy? Definitely an avenue worth exploring, I'd just want to be careful because it could lead to an implicit dependency on environment-specific behavior, and suddenly it doesn't work in production and we've created more confusion than we've solved. 😝

I wouldn’t do that either, because I already see new issues popping up because of it.

Yes! I just pushed a big update to the Posts guide with details about collections and laziness. Would love support on filling in the gaps!

I looked at it, very nice! Thanks man!

Yeah, in addition to providing some sensible default behavior in Timber\Post (I just wanted to avoid touching Post too much in this PR, and just focus on collections). Child implementations can easily call parent::jsonSerialize() to get the data to use as a basis, so we should capture that in the docs.

Right, good idea! We should definitely also have an example that explains that removing data would make sense, too, because you don’t want to fill the HTML of a page with data that has to be downloaded but that you will never need.

@acobster I saw you removed the WIP label. Are we ready for a final review, or do you have more commits to come before me or @gchtr should do a deep dive?

@jarednova @gchtr yeah, consider this ready. I will follow up with a deprecation notice in PostQuery::get_posts(), but other than that I wasn't planning any more changes for this PR.

@gchtr correct, I did not.

@jarednova did you want to give this another look before I merge this?

@acobster almost! I wanted to investigate the remaining failing tests and caught an issue that I wanted to make sure you were aware of. In b0efe7d I made a few updates to fix some test deprecation notices (easy). However, the test for TestTimberContext::testPostsContextSimple() revealed that we need to handle for what's returned with the default Timber::context() call.

I didn't know if this was already tackled in another branch or PR so I created my own to demonstrate and (potentially/hopefully) resolve in #2315.

Can you review that and see if you agree with those changes? That seems to me to be the only last thing we need to tackle on this piece

@jarednova your changes absolutely make sense, and I'm actually already working on those in a separate branch built off of this one. I'm not going to merge it in to avoid merge conflicts, but I'll keep your PR open so you can verify against the new branch once I push it.

Great! In that case let's call this one 👍 — please do the honors of merging into the factories branch so we can follow along w that other stuff