Codecov Report

Merging #2073 into 2.x will not change coverage.
The diff coverage is 100%.

@@            Coverage Diff            @@
##                2.x    #2073   +/-   ##
=========================================
  Coverage     96.04%   96.04%           
  Complexity     1530     1530           
=========================================
  Files            50       50           
  Lines          3892     3892           
=========================================
  Hits           3738     3738           
  Misses          154      154

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0cd1ea0...b57318f. Read the comment docs.

@gchtr and @pascalknecht , thanks so much for putting this together! This is really helping me focus on shipping some good code.

Let me first just say that I like what I'm seeing. Keeping the API simple and small is definitely the way to go here, and I like that a Class Map can be either an array or a function. I think that'll keep things really simple for most use cases, yet endlessly extensible for more advanced usage. Just a few clarifying points before I dive in...

I think we should make the Timber::get_posts() usage change more explicit, and provide before/after code block examples (Lando's upgrade guide does a really good job of this IMO).

Throw a doing-it-wrong exception when a Post or PostQuery object is instantiated directly. (Is that even possible?)

I don't think it's possible to detect who's calling a constructor (or any other method) without getting rrrreally hacky. :) But, we can guarantee it will never be called directly by making __construct() protected:

class Thing {
  public static function build() {
    return new static();
  }

  protected __construct() {
    $this->stuff = 'asdf';
  }

  public function get_stuff() {
    return $this->stuff;
  }
}

Thing::build()->get_stuff(); // -> "asdf"

This is a pretty common approach when implementing the Factory/Builder patterns in other OO languages, and I think it makes sense here. However, it does force our hand in terms of the 2.0 vs. 2.x rollout, since it would mean a breaking change instead of a deprecation we can make down the line. That's unfortunate from a timing standpoint because it means we have to do more work up front, but I think it's worth doing that anyway. This will force us to be consistent across the board at 2.0, and I don't really see an alternative.

I know Inheritance is not always the best option for Object Oriented Programming. Does anybody see any problems with class conflicts?

What do you mean by class conflicts? Are you talking about composition over inheritance? Are you worried about problems with inheritance in core Timber code, in user code, or both?

For the API in Twig, we still have Post, PostQuery, etc. For consistency, should we consider renaming these functions to get_post(), get_posts(), etc?

I think renaming and deprecating the old Twig functions is probably a good idea. Seeing "Use Timber::get_posts() instead of new Timber\PostQuery()" and then a paragraph later seeing "You can also use PostQuery in Twig" is a little jarring, and it's one less thing for new users to have to remember.

Should we use Timber::get_posts() or Timber::query_posts()? Would it be more clear that query_posts() returns a query/collection of posts, and not an array of posts?

I think we should keep the API as close as possible, and not rename the method. I'm definitely not dogmatic about backwards-compatibility, but in this case we're talking about one of the single most important methods in the entire library. At the very least, we should keep the name the same.

From what I can tell, the idea for the 'query' => [ ... ] syntax got introduced in #1957 as a way to distinguish between actual WP_Query args and Timber-specific options like post_class. Do I have that right?

Assuming that's the case, I'm not a huge fan of changing the structure of this first argument, because it means users will have to update every single call to Timber::get_posts. On the other hand, if we keep the first argument the same, only calls using a higher arity need to change. I imagine, based on my own code and my colleagues', that this is a significant portion of method calls currently in production, probably most of them.

Here's the API I propose instead:

public function get_posts( array $query, array $options = [] );

This would allow us to extend the API in the future without changing the signature, and support whatever $options we want (merge_default, pagination, etc.) out of the box with 2.0. And if I'm understanding correctly, as far as no_found_rows and friends go, this lets us keep the spirit of your note about performance, except no_found_rows etc. would just be part of the normal $query arg.

I'm sure I'll have more comments and questions as I move along but I feel like this is off to a good start. Thanks again.

Thanks for your deep dive into the proposal!

I think we should make the Timber::get_posts() usage change more explicit, and provide before/after code block examples (Lando's upgrade guide does a really good job of this IMO).

I agree, we could improve that in the documentation 👍. I’m adding this to the list.

I don't think it's possible to detect who's calling a constructor (or any other method) without getting rrrreally hacky. :) But, we can guarantee it will never be called directly by making __construct() protected:

I see, that’s the way to go. Making the constructor protected would work. And yes, we would have to do that in 2.0, because it would be a breaking change.

I know Inheritance is not always the best option for Object Oriented Programming. Does anybody see any problems with class conflicts?

What do you mean by class conflicts? Are you talking about composition over inheritance? Are you worried about problems with inheritance in core Timber code, in user code, or both?

Yes, exactly, I mean composition over inheritance. But probably not in Timber core, mostly in user code, when class maps are used to define which class is used when. For now, I don’t see any direct problem, but maybe there are warning signs that I don’t know of? You certainly have a better grasp of these patterns ;).

I think we should keep the API as close as possible, and not rename the method. I'm definitely not dogmatic about backwards-compatibility, but in this case we're talking about one of the single most important methods in the entire library. At the very least, we should keep the name the same.

Well, of course! I totally forgot again about the simplest reason why it should be Timber::get_post(): because there’s get_post() 🙈. Must have been late 😄.

From what I can tell, the idea for the 'query' => [ ... ] syntax got introduced in #1957 as a way to distinguish between actual WP_Query args and Timber-specific options like post_class. Do I have that right?

Yes, you got that right!

Assuming that's the case, I'm not a huge fan of changing the structure of this first argument, because it means users will have to update every single call to Timber::get_posts.

Here's the API I propose instead:

public function get_posts( array $query, array $options = [] );

Totally! Here it wouldn’t make much sense to make developers change every call to Timber::get_posts(). Let’s do it the way you proposed 👍. I can take care of the relevant inline documentation as well as the documentation in the guides, if you want me to.

@gchtr sounds good! Yes, if you could update the docs that'd be great. I'll continue laying the groundwork for using Factories under the hood, everywhere. There a lot of direct instantiations in the tests...

For now, I don’t see any direct problem, but maybe there are warning signs that I don’t know of?

Since we're talking about "userland" code, I don't think there's a whole lot we can do in core to prevent the kinds of problems that composition-over-inheritance addresses. I say this because these kind of problems are rooted in poor software design. Aside from the premise that custom posts may be instances of different classes, Timber is pretty unopinionated about architecture, and I think that's a good thing.

I do think it would be a good idea at some point to have a guide recommending using traits to accomplish composition over inheritance in tandem with class maps, since traits seem to be a somewhat underappreciated feature of PHP, especially in the WP world, in my experience. I'll write up a future issue for that.

@acobster Great, I’ll be updating the docs in this pull request, then.

Since we're talking about "userland" code, I don't think there's a whole lot we can do in core to prevent the kinds of problems that composition-over-inheritance addresses. I say this because these kind of problems are rooted in poor software design. Aside from the premise that custom posts may be instances of different classes, Timber is pretty unopinionated about architecture, and I think that's a good thing.

I do think it would be a good idea at some point to have a guide recommending using traits to accomplish composition over inheritance in tandem with class maps, since traits seem to be a somewhat underappreciated feature of PHP, especially in the WP world, in my experience. I'll write up a future issue for that.

Alright! I’d say we leave it at "some point". I think there’s no need to add documentation for problems that might arise at some point in the future. I’m never against an additional guide that shows some advanced usage pattern, though 😉.

@gchtr I made a bunch of updates to the description per our discussion, and created some new issues that are big enough to be scoped and tracked separately. Please review. I'll continue working on the internal Factory API for now, and circle back to tying it in with the larger API changes. Thanks!

@pascalknecht after the discussion on this ticket, I think that our best route here is, unfortunately, to close #1219 and #1793 and start on Factories fresh, with the new API proposal as the motivating goal. There are simply more moving pieces, many of them coupled to the old API (or to a new API that we're not going to use), than I can wrap my head around right now. I could try to refactor #1219 to match the new requirements, but I think that would take me about twice as long as just starting fresh and pulling in what I need to accomplish the end goal of this API. Unless you have the bandwidth to take on this major refactor of a completely new API (which also currently lacks tests), I'm going to go ahead and close out these two PRs. Thanks.

@acobster @gchtr thank you for the thoughtful rundown of the issues and outcomes. Between teh limitations of PHP, WordPress and naming conventions, there's unlikely to be a perfect solution, but this one seems damn good (at the very least).

Now that this seems like the appropriate path, closing #1219 and #1793 is probably the right move. Even if the exact code from @pascalknecht and @joshlevinson doesn't make it in — each of these PRs developed some of the key ideas we're now implementing.

Happy to continue contributing my two cents — but mostly I'll stay out of the way as you two keep hammering!!!!

@jarednova @gchtr another handful of questions for the team:

1. Should Timber::get_posts() always return a PostCollection? Will we ever want to return the more specialized PostQuery? Is there a distinction between the two that users will find useful going foward?
2. The same method also sometimes returns a QueryIterator. Can this behavior be subsumed into PostCollection or PostQuery somehow? In other words, can we just always return some kind of PostCollection, and have the Factory layer make sure that that collection is initialized properly?

Here's my take ...

Should Timber::get_posts() always return a PostCollection? Will we ever want to return the more specialized PostQuery? Is there a distinction between the two that users will find useful going forward?
The different behavior of get_posts vs PostQuery was more of a historical accident than a deliberate choice. PostCollections are essentially fancy arrays that are agnostic of their origin. However, PostQuerys are fancy PostCollections that are aware of their origin (the query). Why does this matter? Mainly pagination. A PostCollection of post ids 11-20 doesn't care that it's page 2 of a query that ultimately holds 500 posts (for example). However, the PostQuery does care. My take is that PostQuery is the class that most users would want to interact with and should be what is generally returned from get_posts and other methods.

The same method also sometimes returns a QueryIterator. Can this behavior be subsumed into PostCollection or PostQuery somehow? In other words, can we just always return some kind of PostCollection, and have the Factory layer make sure that that collection is initialized properly?
This one is a little trickier. As I recall/understand QueryIterators are used to handle the requesting and iterating through these PostQuerys. I think this could be subsumed into another class. I think the reason it's separate is to contain this piece of functionality into its own class.

It strikes me now that this is really a _Post_QueryIterator. If this is a pattern to continue, it might provide a model for UserQueryIterator, TermQueryIterator ... However, none of these are really meant to be "public." If there's a better organization paradigm based on factories, we should probably go that route

@jarednova @gchtr alright...buckle up. 🚀

Edit: updated names and added some clarifications.

My take is that PostQuery is the class that most users would want to interact with and should be what is generally returned from get_posts and other methods.

I agree...insofar as we want a unified interface that we return every time.

I think the reason it's separate is to contain this piece of functionality into its own class.

By "this piece of functionality" do you mean translating from a WP_Query to an iterable collection that is aware of pagination?

If there's a better organization paradigm based on factories, we should probably go that route

After some thinking, I believe that the best approach here is to define a PostCollectionInterface, and return a different concrete instance depending on whether we've been passed an array or a WP_Query. Here's the API I propose, based on what's currently in PostQuery, PostCollection, and QueryIterator:

interface PostCollectionInterface extends Traversable, Countable, ArrayAccess {
  public function pagination( array $options = [] );
}

class PostQueryIterator implements PostCollectionInterface {
  public function pagination( array $option = [] ) {
    return new Pagination($options, $this->_query);
  }
  /* all the same traversal/counting methods currently in QueryInterface, plus ArrayAccess methods... */
}

class PostArrayObject extends ArrayObject implements PostCollectionInterface {
  public function pagination( array $options = [] ) {
    return null;
  }
}

A couple things to note:

• I chose to extend those interfaces specifically because they are close to the lowest common denominator of ArrayObject (which PostCollection extends) and Iterator/Countable (which QueryIterator implements). Also because ArrayAccess is nice, and easy to implement. :)
• PostArrayObject already implements Traversable etc. by virtue of extending ArrayObject.
• A Factory can decide which class to instantiate based on arguments to Timber::get_posts() (or whatever other internal code is calling the Factory).

We should document PostCollectionInterface and the classes that implement it. Maybe a future version of Timber can expose a filter for overriding the concrete instance of PostCollectionInterface returned from Timber::get_posts() (i.e. a Class Map filter), but we don't need that right now.

The long version

I hemmed and hawed over this issue for a while before coming to the above conclusion. In case anyone's interested, here's how I got there...

In its constructor, to get the array of posts that it will internally iterate over, PostQuery delegates to PostGetter::query_posts(), which in turn returns either a new instance of PostCollection or an instance of QueryIterator depending on the type of array it's been passed. Lifting that logic out of PostGetter, the crucial lines in PostQuery::__construct() can effectively be translated into:

if ( is_array($args['query']) && count($args['query']) && isset($args['query'][0]) && is_object($args['query'][0]) ) {
	// We have an array of post objects that already have data
	$this->queryIterator = new PostCollection($query, $args['post_class']);
} else {
	// We have a query (of sorts) to work with
	$this->queryIterator = new QueryIterator($query, $args['post_class']);
}

$posts = $this->queryIterator->get_posts();

parent::__construct( $posts, $args['post_class'] );

...meaning that PostQuery (a PostCollection itself) is delegating to QueryIterator or to another PostCollection. Smelly! :)

This complicates the claim that:

A PostCollection of post ids 11-20 doesn't care that it's page 2 of a query that ultimately holds 500 posts (for example). However, the PostQuery does care.

Where this gets really sticky is that PostQuery actually only returns a Pagination object IFF its internal queryIterator is a Timber\QueryIterator instance (and not a PostCollection, as we've seen it could well be):

	public function pagination( $prefs = array() ) {
		if ( !$this->pagination && is_a($this->queryIterator, 'Timber\QueryIterator') ) {
			$this->pagination = $this->queryIterator->get_pagination($prefs, $this->get_query());
		}

		return $this->pagination;
	}

I don't know if there's some subtlety elsewhere in the code guaranteeing that we never actually are instantiating PostQuery in such a way that it will never delegate to a PostCollection, and therefore it will always return a Pagination instance...but in any case that's just a stronger argument for clarifying this distinction.

All that is to say: it seems like what we're doing here abstracting away two different data sources (a primitive array of posts or a WP_Query) within the same class/constructor. So the question then is: do we really need to worry about always returning a concrete PostQuery? Or is our real concern here with the interface? The complicated delegation going on in the constructor seems to work toward unifying pagination and traversal into a single class, when we could make equally strong guarantees by returning different concrete classes that implement the same interface. Hence the above proposal.

My questions/points all feel answered. To the point of:

Or is our real concern here with the interface?

Yes. I don't think we'll have anyone angry/care about whether it's a PostQuery or not. But what does matter is the ability to easily traverse, iterate, etc. (all the things you know and love about arrays). If this can be handled with fewer classes, all the better.

The proposal feels very reasonable and a sensible organization of the classes/interfaces we already have. Interested in @gchtr's interpretation on the above details as it relates to the factories component.

@gchtr I made a bunch of updates to the description per our discussion, and created some new issues that are big enough to be scoped and tracked separately.

Cool, thanks @acobster 👍!

1. Should Timber::get_posts() always return a PostCollection? Will we ever want to return the more specialized PostQuery? Is there a distinction between the two that users will find useful going forward?

From answering a lot of questions here in the issues and on Stack Overflow, I’ve found that in the current state, it’s not very clear what the difference between a PostQuery and a PostCollection is. It’s more like: «Sometimes you have to call get_posts() on the result to get all the posts that you get from a query, and sometimes you don’t.» If we could unify this so that we can always use the same functionality no matter the class of the object we get, that would be great! 👍

As far as I understand it, I think your proposal makes sense. I suppose for developers it’s important they can:

1. Loop over the collection of posts.
2. Use a pagination for the posts.
3. Access additional information, like the amount of found posts.

So I agree with Jared when he says:

Yes. I don't think we'll have anyone angry/care about whether it's a PostQuery or not. But what does matter is the ability to easily traverse, iterate, etc. (all the things you know and love about arrays). If this can be handled with fewer classes, all the better.

We should document PostCollectionInterface and the classes that implement it.

Yes, I think that would help developers understand what’s going on 👍. Remember to ping me if I can help here!

Maybe a future version of Timber can expose a filter for overriding the concrete instance of PostCollectionInterface returned from Timber::get_posts() (i.e. a Class Map filter), but we don't need that right now.

Yes, let’s wait until we actually need it, because it’s probably an edge case. The only case I ever needed to overwrite a class inside the post collection logic was a custom PostIterator, but even that won’t be needed with 2.0.

Awesome. Thanks for the responses, @gchtr and @jarednova. I'll follow up with more specifics later in the week (client projects are calling me back) but I think we're in really good shape for the last big push for this API change.

As soon as #2367 is merged, I want to merge this pull request, too. We discussed and documented the new API thoroughly. The outstanding todos all have separate issues and we don’t have to wait for these to be resolved before we merge this.

@gchtr agreed! I just did a once-over of the 2.0 tag and it's looking like we're in really good shape. Let's just keep an eye out for anything else that needs that tag that doesn't already have it.

Once this is merged, how about we make an actual 2.0.0-alpha.01 release?

cc @jarednova

Oh my. Are we actually here????!!!!! Yes! Let's start the tagging and moving to alpha. #2367 looks good to me!

@acobster

Once this is merged, how about we make an actual 2.0.0-alpha.01 release?

Short notice: as per p.9 of semver specification:

Numeric identifiers MUST NOT include leading zeroes

Actual version number should probably be 2.0.0-alpha.1

@jarednova @acobster Yes, I think we’re getting there! For me, there’s still quite some work for the documentation, but that doesn’t stop us from releasing an Alpha version 👍🚀🔥. I’ll let you one of you review and merge this.

🎉 LTGM! I'll also pull this into the docs style updates (timber/docs#23) so we can more closely review that work with these new additions/changes

woohoooo!! 🎉

@jarednova so how do we get the 2.0 docs up to https://timber.github.io/docs/v2?

Also, I have started on a draft release - please take a look and add your notes when you get a chance.

cc @gchtr

@acobster @jarednova I'll have a look at everything tomorrow and will update the live documentation.