@jarednova I think we should absolutely have this for 2.x. I will review it and check what else is needed. Can you please add the label 2.x?

@pascalknecht for now let's keep the focus on #1785 and #1778, etc. After we have those set I think you, me and @gchtr should assess where we are and what items are critical to address for 2.0. If it's a must-have, it's a must-have. But I want to make sure it's something we all decide together b/c this one seems like it could be quite big to integrate and perform the accompanying refactors, etc.

@jarednova @pascalknecht @chrisvanpatten I first thought that we could release a version 2.0 without having an object factory. But I have since changed my mind. I think we need a wrapper for Timber objects. And an object factory could solve that. Here are some reasons:

• When an image is initialized with an URL or a path, we can’t provide all the methods that we provide for normal attachments that come with values from the database, like Image::alt(). @pascalknecht proposed to wrap functionality for images from a path or URL in a new class and provide a reduced set of methods and properties. If we don’t want people to use $asset = new ImageAsset( 'https://example.org/asset.jpg' ) instead of $image = new Image( 'https://example.org/asset.jpg' ), we would need a wrapper function.
• It’s hard to handle objects that can’t be initialized. As discussed in Timber::get_post returns a Timber\Post object even when the post ID doesn't exist #1467, it’s not so trivial to return something meaningful when post objects are created directly through a post. If we had a wrapper, this would be no problem.
• The same goes for images. With the current implementation, it’s hard to check if an image is valid. We’d have to do it with {% if image.file %}, but then again, images from URLs and paths wouldn’t be catched. With a wrapper, we could check for {% if image %}.
• In Twig we already have some kind of wrappers with {{ Post() }}, {{ Image() }}, etc., where we could add the needed logic. But why only have it in Twig, when we can have it in PHP as well?
• For issues like Page Preview should not return a revision post #1752, we could create new classes that decouple logic. For post preview, we could have a PostPreview class that includes all the necessary logic.

I’m sure there are more problems that we can solve. I’m in favor of changing this in 2.0, because when we add object factories, the public API for instantiating posts and queries will definitely change. If everybody upgrades their themes to $post = new Timber\Post() and $posts = new Timber\PostQuery() for 2.0 and we change it again in 3.0, that wouldn’t make so much sense in my opinion. Especially if we already know now that we will eventually need it.

The proposed way for object factories is $post = \Timber\Factory\PostFactory::get( 1 ); for posts, but this seems a little too complicated for me. We can surely find a more elegant public API that will provide us with a good base for the future.

Yes, it’s big to integrate and a lot more work, but I think it might be worth it. It doesn’t have to be full-fledged for every aspect of Timber. That could be handled in 2.1, 2.2 etc.

Factories seem like the right way to go here, for sure. I'd love to see this land in 2.0.

I would also love to see this in 2.0, and I'd be willing to run with what @joshlevinson has so far. I think it's worth the work.

@gchtr I agree there's a more elegant public API: one that already exists in Timber::get_post(), Timber::get_term(), etc. These already wrap calls to object factories, e.g. PostGetter, albeit with somewhat weaker semantics than a true Factory pattern.

What I propose is this:

• switch the *Getter classes out with true factories.
• add a filter in each static get_* method to support implementing your own factory.

That would let users do this:

use Timber\Timber;

$post = Timber::get_post( $post_id );
$image = Timber::get_post( $image_id );

get_class( $post ); // -> "Timber\Post"
get_class( $image ); // -> "Timber\Image"

Furthermore, say you've declared my_custom_taxonomy and a corresponding MyTaxonomyTerm class that extends Timber\Term. The semantics I'm proposing would also let users do this:

use Timber\Factory\TermFactory;

class MyTermFactory extends TermFactory  {
  public function get_class( $type, $taxonomy ) {
    if ( $taxonomy === 'my_custom_taxonomy' ) {
      return MyTaxonomyTerm::class;
    }

    // implicit else
    return parent::get_class( $type, $taxonomy );
  }
}

add_filter( 'timber/factory/term', function() {
  return new MyTermFactory();
});

$tag = Timber::get_term( $some_tag_id );
$custom_term = Timber::get_term( $custom_term_id );

get_class( $tag ); // -> "Timber\Term"
get_class( $custom_term ); // -> "MyTaxonomyTerm"

What's happening under the hood in my mind is that Timber::get_term() is applying the timber/factory/term filter and getting an instance of MyTermFactory, which in turn knows to instantiate MyTaxonomyTerm instances.

@acobster I guess @pascalknecht started working on this in #1793. If the two of you could stick your heads together and bring this forward, that would be great!

Having flexible ways for developers define their own classes is definitely a must. So a timber/factory/term filter would make sense 👍.

I’ve found the Timber\PostClassMap filter, which is currently named timber/post/post_class in 2.x to be quite a useful pattern to define which classes to use for which post type:

That could be adopted for the other object classes as well. I think it’s great because developers don’t necessarily have to extend classes, which keeps Timber’s entry level low.

I’m still thinking about a proper API. Should we keep the existing functions:

• Timber::get_post()
• Timber::get_term()
• Timber::get_comment()
• Timber::get_user()

Or should we use something that might be a little easier to read?

• Post::get()
• Term::get()
• Comment::get()
• User::get()

Would that work as well? Design patterns in PHP is not my strongest suit, so I don’t know if this would be "allowed".

Hi All,

I've had a lot of thoughts about this, and have shifted positions on it a few times, so I'm going to try and get some thoughts down so we can all discuss and hopefully move this forward.

My first reaction was of course 'Hell Yeah Brotha', factories are a classic OOP pattern and a staple of some SOLID code, so lets just push this one through, but the more I investigated tickets, mulled over potential APIs etc, the muddier it seemed to become to me.

I still believe that we should have a factory or factory like class that encapsulates the concerns of the post map (and hopefully term map / any object map in the future ), however I believe that these should be tied to the respective Query or Collection classes rather than the \Timber\Post or Timber\Term class themselves.

Summary of my points ..

✅ Factory classes
✅ Post Map filter, and extending it to Terms, Users
❌ transparent calling via constructor
❌ implementation as static method on Timber\Object derivative classes.

In the 2.x branch, I can only find one instance of Timber directly calling new Post(), and this is tied to the context and interacting with the global state of WordPress, which is a very valid but seperate concern, and there are different ways of dealing with this too (which I'll also propose).

To me, the query classes, such as PostQuery need a way to interact with the post map, but the singular instance of an object does not. I feel that it is acceptable, if not preferable, for the constructor of an object to only ever return THAT object type, and that static methods on 'named' classes which infer meanings or types within a system that can return other types could lead to confusion and further entanglement.

I'm making my judgement based on my experiences with Timber, and I'll try and explain the mock scenario I'm imagining here, so that if my conclusions don't align with yours, you can hopefully provide another different scenario where it makes more sense.

• Our WordPress site contains a custom post_type of book.
• We wish to have a custom Timber Post Class for this, called MyNamespace\Models\Book
• We wish for Timber to always return Book when we interact with post of the type book

So let's get setup...

// functions.php or somewhere in your app..
<?php
add_filter( 'Timber\PostClassMap', function( $classMap ) { $classMap['book'] = \MyNamespace\Models\Book::class; return $classMap; } );

and also

// src/Models/Book.php or whatever suits you homie
<?php
namespace MyNamespace\Models;
use Timber\Post;
class Book extends Post {
    
    public function amazonRating(){
        // some function to call the amazon API here.. 
    }

}

So let's try and get our books now... here's a quick run through of there different contexts, using the current API.

1. An archive for this post type.. ✅

// archive-book.php
<?php
$books = new \Timber\PostQuery();
/* Works, will return a QueryIterator of \MyNamespace\Models\Book objects */

1.1 Using the new 'magic' context of Timber 2.x ✅

// archive-book.php
<?php
$context = \Timber\Timber::get_context();

assert( $context['posts'][0] instanceof \MyNamespace\Models\Book )
/* will be true, Timbers new default query handling uses the post map */

2. A custom page, or archive / single etc for another post type ✅

<?php
$bookQueryArgs = [ 
    'post_type' => 'book',
    /* Other args for the query here */ 
];
$books = new \Timber\PostQuery( $bookQueryArgs );
/* Works, will return a QueryIterator of \MyNamespace\Models\Books */

So it seem's we're pretty sorted for custom queries and default archives already, so then what about the is_single() query?

3. Single post type template, not using Timber 2.x's magic context ✅

//single-book.php
<?php

$book = new MyNamespace\Models\Book(); 

/* $book = new Post(); is doing_it_wrong(); in my book */ 

3.1 Single post type template, using Timber 2.x's magic context ❌

//single-book.php
<?php
$context = \Timber\Timber::get_context();

assert( $context['post'] instanceof \MyNamespace\Models\Book )
/* will be FALSE, this is the place where new Timber\Post is currently hardcoded */

4. In another custom page / template etc ✅

//page-custom.php
<?php

$context = \Timber\Timber::get_context();
$context['book'] = new \MyNamespace\Models\Book( 21 );

To me, in any other context, you will be getting the books via a query (in which case they use the post map_, or instantiating them directly (in which case you should use your class directly!).

What I really don't want, is to end up in this type of scenario...

//page-custom.php
<?php

$context = \Timber\Timber::get_context();
$context['book'] = Post::get( 21 ) /* returns a \MyNamespace\Models\Book */

or image we have a standard post with an ID of 4. Remember that any model that extends post would have access to the factory if it's a static method on Timber\Post...

//page-custom.php
<?php

$context = \Timber\Timber::get_context();
$context['book'] = \MyNamespace\Models\Book::get( 4 ) /* returns a \Timber\Post */

So from all of this, it seems to me that really the only scenario where we'd benefit from a publicly accessible factory method is when interacting with the global $post, as this is of an unknown type.

As I mentioned earlier, I view the magic Timber does with the main WP_Query as being a seperate concern, so I'd rather fix this via another class, like MainPost and MainQuery which extend Timber\Post and Timber\PostQuery, and interacting with these factory classes in a safe way that generally shielded from developers.

This also means things like Timber\Post() can be simplified .. calling new \Timber\Post() (where $id = null ) would throw a deprecation warning, then call new \Timber\MainPost() or \Timber\MainPost::get_from_query(). Eventually it could be removed or return null / throw an error.

The other big question is, how far do we go in protecting or validating that what the user is fetching works.. if I tried to new \MyNamespace\Models\Book( 4 ) where 4 is post_type of post, should this fail?

I'm all for predictability, so I'd rather that this failed or returned null... but other devs may not feel the same?

I'm really open to some more discussion about this, but I guess I wanted to present an alternative opinion in the hopes we can debate the pros and cons and get this moving.

Hey Alex

Thank you so much for writing such an enthusiastic comment! I like your thinking and that you bring this up again. It’s good to make a step back and look at it again.

I thought about your points and wanted to bring my thoughts in.

In the 2.x branch, I can only find one instance of Timber directly calling new Post(), and this is tied to the context and interacting with the global state of WordPress, which is a very valid but seperate concern, and there are different ways of dealing with this too (which I'll also propose).

To me, the query classes, such as PostQuery need a way to interact with the post map, but the singular instance of an object does not.

There are more case where Timber creates and returns objects:

• $post->terms()
• $post->children()
• $post->thumbnail()
• …

The first two examples run queries, but the third one doesn’t. We need a way to control what objects these methods return. I guess with a Post Class Map including the classes for different post types and taxonomies, we should get the proper objects back in the same way we get them in Post Queries. ✅

I feel that it is acceptable, if not preferable, for the constructor of an object to only ever return THAT object type, and that static methods on 'named' classes which infer meanings or types within a system that can return other types could lead to confusion and further entanglement.

Totally!

Is there something that would speak against get() always returning its own class?

// Always returns instance of Timber\Post
$post = new Timber\Post::get( $post_id );

// Always returns instance of MyNamespace\Models\Book
$book = new MyNamespace\Models\Book::get( $post_id );

The other big question is, how far do we go in protecting or validating that what the user is fetching works… if I tried to new \MyNamespace\Models\Book( 4 ) where 4 is post_type of post, should this fail?

I’d say yes.

What about Twig?

In PHP, I guess it would work, but what happens in Twig? In some circumstances we can’t access PHP files and only have access to Twig files.

Currently, it’s possible to convert a single post or array of posts or post IDs to proper Timber objects with Post(), Term(), User() or Image().

{% for post in Post(post_ids) %}

{% endfor }

Here I guess, we would still apply the Post Map, but it would be against what we say in PHP, where we would always have to be explicit.

How could we solve this any different?

Why do we need a wrapper function?

Even if we say that we don’t want Timber\Post::get() to return an object based on the Post Class Map, I think we still need a wrapper to get Timber objects.

Maybe we don’t have to call it Object Factory, but I feel like for getting singular posts, it makes sense to have a wrapper function, similar to get_post().

This also means things like Timber\Post() can be simplified .. calling new \Timber\Post() (where $id = null ) would throw a deprecation warning, then call new \Timber\MainPost() or \Timber\MainPost::get_from_query(). Eventually it could be removed or return null / throw an error.

We can’t return null or an instance of another object in an object constructor, can’t we? Without a wrapper for instantiating posts, you could end up with an object where a lot of properties are empty, but when you check for the post to exist, it still returns true.

{% if post %}
    {# No idea whether the post is actually a
       valid post or a post with an empty ID #}
{% endif %}

We would have to go for a method to check for validity or any other method listed in #1467 (comment). For example:

{% if post.exists %}

{# or #}
{% if post.is_valid %}

The same goes for images:

• When an image is initialized with an URL or a path, we can’t provide all the methods that we provide for normal attachments that come with values from the database, like Image::alt(). @pascalknecht proposed to wrap functionality for images from a path or URL in a new class and provide a reduced set of methods and properties. If we don’t want people to use $asset = new ImageAsset( 'https://example.org/asset.jpg' ) instead of $image = new Image( 'https://example.org/asset.jpg' ), we would need a wrapper function.
• […] With the current implementation, it’s hard to check if an image is valid. We’d have to do it with {% if image.file %}, but then again, images from URLs and paths wouldn’t be catched. With a wrapper, we could check for {% if image %}.

The book scenario

Your scenarios make total sense to me. A couple of thoughts:

3.1 Single post type template, using Timber 2.x's magic context

Yes, that’s supposed to fail, for now. I’ve reworked the context without yet considering what changes need to be made to accomodate Object Factories. The context logic is not set in stone yet. We could totally refactor it again.

If we directly instantiate an object from a constructor, we would have to remove the automatic context for Singular templates. We would have to be explicit and set up the context the same as in the example for Using a custom post class:

<?php

$context = \Timber\Timber::get_context();

$book = new \MyNamespace\Models\Book( 21 );
$book->setup();

$context['book'] = $book;

// Or in a shorter version
$context['book'] = ( new \MyNamespace\Models\Book( 21 ) )->setup();

What about verbosity?

What I’ve seen in many examples of Timber being used in the wild is that developers use a single single.php or archive.php file to handle most of the cases for singular and archive pages. Just to clear: in your case, we would have to be more verbose, even though you’d have a Post Class Map in place:

single.php

switch ( get_post_type() ) {
    case 'post':
        $post = new Timber\Post( $post_id );
    case 'book':
        $post = new \MyNamespace\Models\Book( $post_id );
}

$context['post'] = $post;

The case of setup()

3. Single post type template, not using Timber 2.x's magic context ✅

4. In another custom page / template etc ✅

What’s missing in these scenarios is that you will also have to call $post->setup() for Timber to be fully compatible with WordPress and third party plugins.

I’m not against removing the automated logic in context. It’s in line with what we have often said recently about removing magic. Maybe it’s a good step to educate developers that they have to setup posts.

Why do we have to call setup() again?

We need to call setup() when we work with third party plugins like WooCommerce, BuddyPress or the Visual Composer. The setup() function sets up the $post global that many plugins rely upon.

Technically, we wouldn’t want to call it for an example I listed in #1771:

I display a list of related posts below the post content in a singular template. After the related posts, I display the post comments through the comments_template() function. The displayed comments will be the comments from the last of the related posts instead of the one from the currently displayed post.

However, it’s probably easier to call it all the time and be on the safe side.

We can integrate the setup() and its accompanying teardown() function in the query iterators. But for the main post in a singular post, it’s harder to hide it. Currently, it’s in the context. It doesn’t make sense to put it in the constructor, a wrapper function or an Object Factory. Like I said, we could also remove that logic and make developers call it explicitly all the time.

Why would we need MainPost and MainQuery classes?

As I mentioned earlier, I view the magic Timber does with the main WP_Query as being a seperate concern, so I'd rather fix this via another class, like MainPost and MainQuery which extend Timber\Post and Timber\PostQuery, and interacting with these factory classes in a safe way that generally shielded from developers.

Can you state what exactly you would move to MainPost and MainQuery?

I thought about it, and it’s a hard one. Because when is it a MainQuery and when not? Which filters do get applied for a post and which don‘t? When working with WooCommerce or other plugins, it doesn’t really matter. You’ll always have to work with the $post global, even when you work with posts not related to the main query or main post, because otherwise, you run into weird errors. If you’d have to use MainQuery to display a collection of product from the same product category, that would be weird, right? As you mentioned, maybe there’s a better name that would make this more clear that you use a query "that works with the WordPress globals".

Educating developers about all the differences is hard. I’d love to see if we can hide a lot of the logic to make Timber easy to use even for someone who doesn’t understand all OOP concepts yet.

I’m not sold on any API yet. What do others think about this, @pascalknecht, @chrisvanpatten, @acobster, @jarednova?

@aj-adl @gchtr thanks for your extensive thoughts here. I think most of the others on this thread have more technical knowledge of the nuances here, but I want to chime in quickly with a few core principles/thoughts based on what I've read through.

Thought 1: The API

@gchtr notes he's not sold on any particular API yet. Let's fix this! Whatever the best approach here is, the engineer might want to dummy-out a sample theme with what feels right and then have the code "answer" what someone might try to do.

Thought 2: Let's start somewhere

I'm okay with the idea that we ship with the 80% solution here. Something like "verbosity" isn't so much a true/false thing, but a balance question (which we'll inevitably have to fine-tune over time).
Timber has gone through lots of variations over the last ~6 years (from theme scaffold to plugin to Composer dependency ...). Similarly, whatever we decide today we should fully expect/accept the idea that we'll build towards the thornier solutions rather than start from them. I think in general, we should ask "more" from devs and introduce more "magic" over time, instead of trying have the code do too much at the outset.

Thought 3: Role Check

The one thing I'd hate to see is all the thought and work on this languish on the vine (I mean, I guess there's a lot I'd hate to see, but that's one of the things). @pascalknecht are you still in the midst of work on this? I want to make sure we don't have confusion about who's doing what next

@jarednova thanks for chiming in. I agree 100%.

Role Check

I'll address roles first: this is, as I understand it, basically subsumed by #1793, where @pascalknecht and I have gotten a little further in the discussion of the API. Maybe we should copy over the recent discussion here (there's lots of good stuff!) and close this PR to avoid future confusion?

And as far as roles go, I'm still nominally the owner of this (accordingly, I assigned it to myself just now), although life circumstances have changed and I'm no longer able to commit personal time to this project, so I'm having to champion company time to work on Timber. Progress there is slower than I'd like, but promising! I expect to be able to devote a handful of hours to Timber each week going forward, and the Factories feature will be my main focus for now.

The API

I'm not sold on an API either. @pascalknecht, @gchtr, and @aj-adl all bring up great points that I'll need to synthesize before I can make meaningful progress on this.

In any case, for new APIs, the strategy I've had recent success with is documentation first. Short of a formal spec, I've found this is one of the best ways for users to understand and critique, and for me to iterate on as an engineer. I can push changes inside /docs to begin with, and move forward with implementation once the team has reached a consensus that concerns raised here have been addressed.

Let's start somewhere

Yes.

Loathe as I am to scrap all the work people have put in so far, I feel like honoring all the feedback we've gotten about the API will be difficult to integrate with what we have so far over on #1793.

(Side note: Not least of my concerns is the fact that many, many unit tests are still failing in my (new) Vagrant environment. This makes it extremely difficult to reliably make incremental changes. Maybe there are upstream changes that will alleviate this, but in any case it's something I will definitely need to figure out before I can make real progress on implementing something.)

Anyway, I have no idea if, once we arrive at a usable spec for an API, it will be a good match with that PR. Plus, it's in a very transitional state that makes it harder to reason about. All that is to say, I think there's a good chance I may have to start over on this. But, I don't think it'll be as difficult as it has been to make progress on this with some solid documentation around what I'm actually implementing.

Thoughts?!

Hey @acobster thanks for your thoughts there! I've never tried documentation first but there are a few hundred reasons why that's a great approach for us here (i.e.: being clear on the transition from 1.0, how the new way works, its benefits ...)

Probably the best thing I can offer is that I'm happy to do some kind of remote session to help get your Vagrant environment up-and-running — I just moved to a new computer and found some "gotchas" between VVV 2 and VVV 3. Lemme know by email if you want to find some time for this!

I had a Slack session with @pascalknecht today and we discussed some questions we both had. We have an idea for an API that could work well.

We too think that starting with documentation is a good idea. We will come up with a pull request with proper documentation for the new possible API and an outline for how it should work in the background.

We’ll keep you posted.

To be continued in #2093