2.x Add Timber::get_post_by() by gchtr · Pull Request #2169 · timber/timber

gchtr · 2020-01-12T20:10:56Z

Ticket: #2152

Issue

Solution

~~Add Timber::get_post_by_slug() and Timber::get_post_by_title().~~
Add Timber::get_post_by().

I chose post_exists() when searching for any post type and get_page_by_title() when searching for specific post types as the functions to retrieve a post by post title, because they seem to be the most direct and performant ones.

Impact

Closes a gap in what Timber::get_post() can’t (and shouldn’t be able) to do in Timber 2.0.

Usage Changes

Instead of Timber::get_post() with a post title or post slug, developers should use one of the two new functions.

Considerations

The two functions always run database queries to get post IDs, which got me thinking about performance. When we use ::build() to create an object, could we eventually use wp_cache_get() and wp_cache_add() in the future to create an internal object cache for Timber objects?

Testing

Yes.

jarednova · 2020-01-12T21:40:26Z

@gchtr I know you're going to scream, but having just spent time with #2160 and #2167, I'm thinking of Timber::get_user_by — does it make sense for us to follow the same pattern with:

Timber::get_post_by('slug', 'my-cool-slug');
Timber::get_post_by('title', "The Old Man and the Sea");

... ? I realize that's piggy-backing on the corresponding WordPress function that isn't matched by Posts. Had you given that any thought?

gchtr · 2020-01-12T21:58:41Z

I know you're going to scream

@jarednova Naaah, that’s fine 😄.

To be honest, I didn’t think of this at all yet. But I like it! Let’s get @acobster’s opinion, before I’m going to change this.

acobster · 2020-01-17T21:58:33Z

@jarednova @gchtr I like it! It's more consistent and keeps the surface area smaller.

acobster

Main thing left to do is to convert consolidate both methods to ::get_post_by(). Also just need to make sure we take the new/correct return value into account.

lib/Timber.php

jarednova

@gchtr looking good! There's a discrepancy between the test results and what I think we're expecting. Right now the behavior with multiple results is ...

get_post_by('slug' => returns the newest post based on WP's default sort behavior
get_post_by('title' => returns the oldest post based on the behavior in get_post_by_title

I think these should be consistent in their behavior (lowest ID? oldest? newest?). Standard use case I imagine to be that a dev is trying to get something like privacy-policy. You could imagine another author (for some stupid reason) using this as a slug or title for a future post which would then break the linkage. Therefore, I'd think it makes sense to always return the oldest match.

Happy to take a whack at this, but wanted to run that by you first

tests/test-timber.php

gchtr · 2020-02-08T20:31:37Z

@jarednova Yeah, I remember reading that in the function description, but then the tests ran successfully, so I thought it’s fine like it is. Apparently it isn’t and I must have missed something. Thank you for catching this! I’d be happy if you could take over to solve this 👍.

jarednova · 2020-02-09T15:30:22Z

np! will take care of this in the next couple days

jarednova · 2020-02-10T14:55:32Z

@gchtr this is now ready for your final review!

When we check for the date, the ID of the post we expect to be returned needs be higher than the ones that should be ignored in order for the test to run correctly

This makes it possible to order by anything we want

gchtr · 2020-02-16T10:27:36Z

Thanks @jarednova for working on this! Your changes actually pointed me in the right direction and made me rethink how we should handle cases when multiple posts are found. I created another pull request in #2195 that we need to discuss first.

2.x Improve Timber::get_post_by()

# Conflicts: # tests/test-timber.php

# Conflicts: # docs/v2/upgrade-guides/2.0.md

jarednova · 2020-06-17T03:10:19Z

@gchtr just wanted to make sure this didn't get lost in the cracks. With #2195 merged, is this also ready?

gchtr · 2020-06-17T06:14:21Z

@gchtr just wanted to make sure this didn't get lost in the cracks. With #2195 merged, is this also ready?

@jarednova Yes, this one’s ready!

jarednova · 2020-06-19T03:05:46Z

We're good here! The test failures are related to static analysis and a failure in an unrelated issue in the 2.x branch

gchtr added 2 commits January 12, 2020 20:50

Add Timber::get_post_by_slug() and Timber::get_post_by_title()

9350200

Add new functions to Upgrade Guide

0bde72e

gchtr added the 2.0 label Jan 12, 2020

gchtr requested review from acobster and jarednova January 12, 2020 20:10

gchtr requested a review from pascalknecht as a code owner January 12, 2020 20:10

gchtr added a commit that referenced this pull request Jan 12, 2020

Update doing_it_wrong() call to include new solution added in #2169

45126aa

gchtr self-assigned this Jan 12, 2020

jarednova added 2 commits January 17, 2020 18:16

Merge branch '2.x' into 2.x-get-post-by

c36e3e3

Merge branch '2.x' into 2.x-get-post-by

6de5ef1

acobster requested changes Jan 17, 2020

View reviewed changes

lib/Timber.php Outdated Show resolved Hide resolved

gchtr added 2 commits January 18, 2020 17:38

Merge functions in to Timber::get_post_by()

5ae15e5

Merge branch '2.x' into 2.x-get-post-by

5970ebd

gchtr added a commit that referenced this pull request Jan 18, 2020

Update help text to use new function introduced in #2169

9677be7

gchtr changed the title ~~2.x Add Timber::get_post_by_slug() and Timber::get_post_by_title()~~ 2.x Add Timber::get_post_by() Jan 18, 2020

jarednova requested changes Jan 20, 2020

View reviewed changes

tests/test-timber.php Show resolved Hide resolved

tests/test-timber.php Show resolved Hide resolved

Merge branch '2.x' into 2.x-get-post-by

0102e77

gchtr assigned jarednova and unassigned gchtr Feb 8, 2020

jarednova and others added 6 commits February 9, 2020 20:29

Merge branch '2.x' into 2.x-get-post-by

0978d38

Fix test with post_type definition; make sure to sort returned posts ASC

ca119ec

Fix test case for getting a post by title

57bb9fb

Ensure that post types are public

731834d

Ensure there isn't another title screwing with this test

f6b9ad4

Does it just need to wait?

dcad91b

jarednova previously approved these changes Feb 10, 2020

View reviewed changes

Merge branch '2.x' into 2.x-get-post-by

402e3c9

jarednova assigned gchtr and unassigned jarednova Feb 15, 2020

gchtr added 4 commits February 16, 2020 11:07

Add expected post as the last one so that ID will be higher

8705f31

When we check for the date, the ID of the post we expect to be returned needs be higher than the ones that should be ignored in order for the test to run correctly

Add checks for both 'any' keyword an array of posts

e599bab

Improve documentation

744d5b8

Replace post_exists() and get_page_by_title() with our own SELECT

d6f95d3

This makes it possible to order by anything we want

gchtr mentioned this pull request Feb 16, 2020

2.x Improve Timber::get_post_by() #2195

Merged

jarednova added 2 commits February 16, 2020 16:01

Allow for $args that support sorting queries

f96c2f7

Update docblock

8e49829

jarednova added the blocked label Feb 17, 2020

Merge pull request #2195 from timber/2.x-get-post-by-oldest

7603c3a

2.x Improve Timber::get_post_by()

jarednova dismissed their stale review via 7603c3a February 28, 2020 00:37

jarednova removed the blocked label Feb 28, 2020

gchtr added 2 commits April 26, 2020 14:03

Merge branch '2.x' into 2.x-get-post-by

0c24052

# Conflicts: # tests/test-timber.php

Merge branch '2.x' into 2.x-get-post-by

faf8207

# Conflicts: # docs/v2/upgrade-guides/2.0.md

This comment was marked as outdated.

Sign in to view

gchtr requested a review from jarednova June 18, 2020 14:53

Merge branch '2.x' into 2.x-get-post-by

22ef841

jarednova approved these changes Jun 19, 2020

View reviewed changes

Merge branch '2.x' into 2.x-get-post-by

9a1a749

gchtr merged commit 7fdd887 into 2.x Jun 22, 2020

gchtr deleted the 2.x-get-post-by branch June 22, 2020 18:16

gchtr mentioned this pull request Jun 22, 2020

Add Timber::get_post_by_slug() and Timber::get_post_by_title() functions #2152

Closed

3 tasks

Uh oh!

Conversation

gchtr commented Jan 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Solution

Impact

Usage Changes

Considerations

Testing

Uh oh!

jarednova commented Jan 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gchtr commented Jan 12, 2020

Uh oh!

acobster commented Jan 17, 2020

Uh oh!

acobster left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jarednova left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gchtr commented Feb 8, 2020

Uh oh!

jarednova commented Feb 9, 2020

Uh oh!

jarednova commented Feb 10, 2020

Uh oh!

gchtr commented Feb 16, 2020

Uh oh!

This comment was marked as outdated.

jarednova commented Jun 17, 2020

Uh oh!

gchtr commented Jun 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jarednova commented Jun 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gchtr commented Jan 12, 2020 •

edited

Loading

jarednova commented Jan 12, 2020 •

edited

Loading

gchtr commented Jun 17, 2020 •

edited

Loading