Coverage decreased (-5.04%) to 89.203% when pulling 9151372 on feature/interfaced-object-factories into fae5977 on master.

Current todo

• Fix tests
• Write new tests for the factory
• Document API
• Document Filters
• Adjust Timber Functions (Post, Image, etc.)

Hey @pascalknecht I'd like to help out here if I can. There are lots of different tasks flying around so I wanted to consolidate the discussion and make sure everybody's on the same page before we move too fast on something as big as this. :) To that end, I'm reposting my comment from #1219 here:

Timber::get_post(), Timber::get_term(), etc. 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.

I believe this preserves backwards compatibility nicely while allowing complete extensibility around the factory system. TermFactory::get() is nice, but it still locks you down to the TermFactory class, whereas having a generic wrapper like Timber::get_term() around the whole factory setup lets you keep things encapsulated inside your own factory class. The Timber\\{$type}Class filter mostly accomplishes this, and we should probably keep some version of that going forward, but I'd like to also be able to implement my own factory classes when the logic becomes too complex to have sitting around in an add_filter() call.

The other thing I wanted to call out before we go too far down the road of testing this stuff is all these static methods. I've found that putting as much logic as possible inside instance methods and making static methods as few and as short as possible makes the code much DRYer, more reusable, and easier to reason about, and often much easier to test. It also allows the proposed timber/factory/* filters to work on objects rather than strings, which seems less error prone to me.

Would love to hear your thoughts on this and see whether/how you might approach splitting up this work. Let me know! 🌲

@gchtr thanks for the heads up, moving discussion over here. I wanted to reply directly to some of your ideas in your comment:

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

Yeah, a fully fledged custom Factory class might be overkill sometimes, and this filter would serve users well for those cases. Implementing and injecting your own Factory class should probably short-circuit the, as having both a custom MyPostFactory and the timber/post/post_class in operation could be confusing.

I’m still thinking about a proper API. Should we keep the existing functions...Or should we use something that might be a little easier to read?

• Post::get()
• ...

See my comment above. I'm not married to keeping the Timber::get_* convenience methods but I do think it is the most flexible because it doesn't lock you down to a specific factory class.

Yeah, a fully fledged custom Factory class might be overkill sometimes, and this filter would serve users well for those cases. Implementing and injecting your own Factory class should probably short-circuit the, as having both a custom MyPostFactory and the timber/post/post_class in operation could be confusing.

I agree.

See my comment above. I'm not married to keeping the Timber::get_* convenience methods but I do think it is the most flexible because it doesn't lock you down to a specific factory class.

True that. I figured Timber::get_post() is also closer to what we are used in WordPress, where we have get_post().

@acobster So let's get on the drawing board here 👍

I would prefer having seperate classes for the object factories as this is more clean. I kinda like the Post::get() syntax. What I currently don't like is the fact that this implementation is stateful as we would need to access the global queried object. This should in my opinion not be necessary. I would prefer to create the implementation stateless and keep all state in the calling code.

Example:

// single.php or page.php
$context = Timber::get_context();
$context['post'] = Post::get(get_queried_object()->ID);

The same thing would be needed for term pages

// taxonomy.php
$context = Timber::get_context();
$context['term'] = Term::get(get_queried_object()->ID);

When the id is undefined a null object or null should be returned. I don't like when it returns a fully fledged object if the post is not found.

Then we will think about the implementation of the filters. I will need to think about this further.
Furthermore I would really like to have the possibility of having a class that everytime maps to a certain post type.

@pascalknecht sounds good.

I would prefer having seperate classes for the object factories as this is more clean. I kinda like the Post::get() syntax.

Fine with me, as long as we're not locking calling code in to a specific factory class. :) That syntax looks nice to me.

I would prefer to create the implementation stateless and keep all state in the calling code.

Yes, relying on global state in library code scares me too. Let's get rid of it!

Furthermore I would really like to have the possibility of having a class that everytime maps to a certain post type.

Could you say more about this? Not sure what you mean.

I would prefer to create the implementation stateless and keep all state in the calling code.

My only fear was that writing Post::get( get_queried_object()->ID ) would look very complicated and repetitive. But then I realized: We don’t have to do this! With the newly introduced template contexts in 2.0, this line will be automated:

$context['post'] = Post::get(get_queried_object()->ID);

So pretty much the only time where you’d have to pass in the $post global will fall away anyways. The other times where you’d use the factory, you’ll probably always have a post ID available to pass in. I like!

@gchtr Yes exactly. So we will have the best of both worlds: Stateless factories and easy implementation.

Furthermore I would really like to have the possibility of having a class that everytime maps to a certain post type.

Could you say more about this? Not sure what you mean.

@acobster The factory should be able to map certain post types to a specific subclass of TimberPost. For example if i have a post type event I would like to be able to add a class Event and have it automatically loaded for this post type. That's a feature which I think was already possible but not as nice to use.

@gchtr @acobster @jarednova Do we all agree on this new API or do you have any inputs/objections/improvements?

I agree! No objections so far.

@pascalknecht ah ok, so specifically it's mapping post_type to a class name, essentially?

@gchtr @acobster I created a first test implementation here: https://github.com/timber/timber/tree/new_object_factory . I was able to get rid of the PostClass String as we can now use polymorphism for this. Not yet sure whether we should use a seperate class for the Factory. We will probably also be able to get rid of all hardcoded class names with the factories,

@pascalknecht what should we do about querying multiple posts? I noticed this in PostGetter:

	// TODO is this right?
	public static function get_posts( $query = false, $PostClass = '', $return_collection = false ) {
		add_filter('pre_get_posts', array('Timber\PostGetter', 'set_query_defaults'));
		$posts = self::query_posts($query, $PostClass);

		/**
		 * Filters the posts returned by `Timber::get_posts()`.
		 *
		 * There’s no replacement for this filter, because it’s called in a function that will be
		 * removed in the future. If you’re using `Timber::get_posts()`, you should replace it with
		 * `new Timber\PostQuery()`.
		 *
		 * @deprecated 2.0.0
		 */
		$posts = apply_filters_deprecated(
			'timber_post_getter_get_posts',
			array( $posts->get_posts( $return_collection ) ),
			'2.0.0',
			false,
			'There’s no replacement for this filter, because it’s called in a function that will be removed. If you’re using Timber::get_posts(), you should replace it with new Timber\PostQuery().'
		);

		return $posts;
	}

Is the plan to make PostQuery the go-to solution for this use-case, and making it use PostFactory under the hood? If so, should we do that as part of this PR, or just shim it for now to get the tests passing?

@acobster Yeah I think everything should use the Post Factory under the hood. Would be probably best to continue here so that we have all discussion here in this thread.

I guess __construct doesn't need to take any arguments now, and if people really want to bootstrap some internal state they still can. So, I don't really see any reason to keep the default constructor around...is that your thinking?

Yeah we can remove the default contructor and just do everything in the factory. That would be the best I think.

@gchtr great questions. I definitely don't know the answer to all of them. :)

• Is the logic from WIP: Feature/interfaced object factories #1793 (comment) already included in this pull request?

Not all of it. I think there are still a few instances where the new API is still broken. (I want to try to get all the tests passing again, or at least the right ones failing, before I try to hack at the factory code any more. I haven't tried this since Jared's most recent 2.x merge.)

We also agreed that the get() method always needs a post ID or an object. If a post can’t be found, it will return null.

Should the get() method have a default of null for the $post_identifier or would that fall away?

Currently, the get() method lies in the Timber\Factory\PostFactory class.

Would we move that over to Timber\Post or do we keep it in the factory and add Timber\Post::get() as a shorthand for the factory?

Not sure about these ones. @pascalknecht ? Having two seems like it would be confusing and unnecessarily complicated. I don't really see a compelling argument not have a Post::get() ...but if we did, what would be the point of exposing PostFactory::get()? Perhaps the Factory one should remain hidden? We could still allow people to swap out Factory implementations if they really needed to through filters, but maybe Factories don't need to be part of the public API after all.

Because get_object() is public, I assume there’s a case where we call get_object() directly? When is that?

I'm not convinced that get_object() should be public. I'm looking into making this protected or private. (I'm also not totally convinced that we need an ObjectClassFactory; seems like the kind of thing that could be refactored away to a private instance methods, a sensible array of default mappings, and a well-placed filter.)

I will try to circle back to the other questions about PostQuery, Users, and Comments, but in short, I think the 1.x APIs for all of those things suffer from the same problems that Posts do, and should all be driven by Factories under the hood.

@acobster makes sense. We should probably just have Post::get() and all related functions (Comment::get, etc.). We should then be careful to just let the users use these API's and hide the rest of the implementation such that they cannot be accessed.

Closing this, but work on Factories will continue in #2093. Thanks so much for your contributions to this effort, @joshlevinson !

Thanks for taking the charge and pushing the idea forward to reality!