2.x Update function signatures for ::get_post() and ::get_posts() by gchtr · Pull Request #2145 · timber/timber

gchtr · 2020-01-06T21:13:58Z

Issue

Ticket: #2090, #2094 related to Timber\Post objects.

Solution

Add `Helper::doing_it_wrong()`

~~I added a Helper::doing_it_wrong() function, that can be used in combination with a @expectedIncorrectUsage tag in the DocBlock of a test.~~

~~This is helpful for functionality that will not be deprecated, but removed, like certain function parameters in Timber::get_post() and Timber::get_posts().~~

Added in #2196.

Remove query methods that won’t work anymore

The following methods for retrieving post were removed. Yes, removed and not deprecated, because there will be no backwards compatibility.

Query strings - Removed the method of getting posts via a query string (like post_type=portfolio) and added a doing-it-wrong message.
Getting a post by post slug – Added a doing-it-wrong message to help developers in the transition. Alternatively, we could add a new function Timber::get_post_from_slug() that could do this (Discussion in 2.x Documentation for API #2073).
Getting a post by post name – This case is catched with the same doing-it-wrong message as getting a post by post slug (Discussion in 2.x Documentation for API #2073).

Deprecate `Timber::query_post()` and `Timber::query_posts()`

The Timber::query_post() and Timber::query_posts() functions weren’t properly deprecated yet.

Return a `PostCollection`

As per the new API, we always return a PostCollection when using Timber::get_posts(). That’s why I changed the current implementation through PostGetter::get_posts() to use true for the return_collection parameter in Timber::get_posts().

However, a lot of tests still break, because Timber::get_posts() doesn’t return a PostCollection() with the ability to handle pagination yet. I guess we’ll first have to make Timber::get_posts() use the new API before this can be merged. @acobster How would you proceed here?

Impact

This will break backwards compatibility with how Timber::get_post() and Timber::get_posts() worked before. But I added checks and warnings to help developers in the transition.

Usage Changes

See #2073.

Considerations

To keep discussion in one place, I added some remarks and questions in #2073 (comment).

Testing

Yes, but work in progress.

Todo

Define default $options for Timber::get_posts()
Make Timber::get_posts() return a PostCollection that can handle pagination.
Use Iterable as return type for Timber::get_posts(), see 2.x Documentation for API #2073 (comment).

Use these two functions throughout tests, replace instances of APIs that won’t work in the future.

gchtr · 2020-01-07T22:54:05Z

As per #2073 (comment), I guess I should change this against the 2.x-factories branch.

acobster

This is looking great at first glance. I'll dig into this further probably tomorrow. Thanks for adding those Helper notices, those look super helpful!

acobster

Looking great for the most part; please see comments.

lib/Timber.php

gchtr · 2020-01-18T16:22:32Z

@acobster I guess I should rebase this pull request against 2.x-factories, too?

# Conflicts: # lib/Helper.php # tests/test-timber-pagination.php # tests/test-timber-post-getter.php

# Conflicts: # lib/Helper.php

# Conflicts: # lib/Timber.php

gchtr · 2020-06-10T19:24:56Z

@acobster Do you want to merge this into the 2.x-factories branch first and then connect Timber::get_post() and Timber::get_posts() to the Post Factory, or do it here?

acobster · 2020-06-10T20:32:56Z

@gchtr let's do in 2.x-factories first. Thanks!

jarednova · 2020-06-17T03:07:52Z

@gchtr are you waiting on me for a review here? I noticed the WIP tag so I had assumed you were still at-work on this.

There are a number of failing tests, mainly related to pagination. Do we need to resolve those here, or is that better done once the merge you and @acobster were discussing is complete?

acobster · 2020-06-17T05:04:01Z

@jarednova I'm assuming there's going to be a lot of lifting to get these two methods working as we get them over to Factories. That work should probably still be done in 2.x-factories IMHO before we bring it into mainline 2.x.

acobster · 2020-06-17T05:12:09Z

@gchtr @jarednova I just realized I may have misunderstood the question. I don't really have an opinion about whether these fixes happen in 2.x-post-functions or 2.x-factories, in case that's what you were asking.

Sorry, my brain has been kinda everywhere recently. Maybe you know the feeling. 🙃

jarednova · 2020-06-19T18:23:03Z

@gchtr the PR has my 👍 on the substance you raise in the original ticket, but I think we should work out (not necessarily solve, but just we're on the same page on this todo you left ...):

Make Timber::get_posts() return a PostCollection that can handle pagination

Right now PostQuerys are essentially this — a subclass of PostCollection that have methods to handle for pagination and the associated data/logic. I think it's crucial that pagination be attached to the returned object as opposed to a separate request.

I'm imagining that most of this code can be brought over into a new class w/o too much hassle; but just wanted to make sure it wasn't lost.

Another thing I encountered when doing some investigation on my local is just the number of jumps from class-to-class a call to Timber::get_posts makes. I'm very much in favor of simplifying this as we adapt to factories even if it means compromising some backwards compatibility in places. The current chain of functions/methods is dizzying (and can't be great for performance either).

You might already be aware or have plans for these. But just wanted to call them out so they don't bite us later!

jarednova

Approved with my comment above! I think this probably makes most sense to resolve in the factories branch where the other code updates also live — but I'll let @gchtr and @acobster make that call

acobster · 2020-09-27T18:08:12Z

Ported into #2316 in 3fabc80. Calling this good.

gchtr added 3 commits January 5, 2020 22:41

Add Helper::doing_it_wrong()

f7e0b56

Update Timber::get_post() and Timber::get_posts() function signature

6642f7c

Use these two functions throughout tests, replace instances of APIs that won’t work in the future.

Optimize @todo

276b141

gchtr added 2.0 wip Work in Progress labels Jan 6, 2020

gchtr requested review from acobster and pascalknecht as code owners January 6, 2020 21:13

Fix comment in example

0a0cad1

acobster reviewed Jan 7, 2020

View reviewed changes

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

45126aa

acobster requested changes Jan 17, 2020

View reviewed changes

lib/Timber.php Outdated Show resolved Hide resolved

lib/Timber.php Outdated Show resolved Hide resolved

lib/Timber.php Outdated Show resolved Hide resolved

gchtr added 3 commits January 18, 2020 17:17

Improve check for invalid argument

e5e0a45

Remove solved todo

f2d7545

Fix @param type

a2abbb3

gchtr added 2 commits January 18, 2020 17:24

Use more explicit return types

4c2aaca

Update help text to use new function introduced in #2169

9677be7

gchtr mentioned this pull request Feb 8, 2020

2.x Documentation for API #2073

Merged

24 tasks

Merge branch '2.x-factories' into 2.x-post-functions

259ea02

# Conflicts: # lib/Helper.php # tests/test-timber-pagination.php # tests/test-timber-post-getter.php

gchtr requested a review from jarednova as a code owner February 16, 2020 11:00

gchtr changed the base branch from 2.x-api to 2.x-factories February 16, 2020 11:01

gchtr mentioned this pull request Feb 16, 2020

2.x Improve error handling #2196

Merged

gchtr and others added 3 commits March 9, 2020 17:51

Merge branch '2.x-factories' into 2.x-post-functions

973ffbe

# Conflicts: # lib/Helper.php

Merge branch '2.x-factories' into 2.x-post-functions

1181446

Merge branch '2.x-factories' into 2.x-post-functions

4637fbf

# Conflicts: # lib/Timber.php

gchtr mentioned this pull request Jun 10, 2020

2.x Update function signature for Term::posts() #2258

Merged

Add some missing links

12bfcd3

jarednova approved these changes Jun 19, 2020

View reviewed changes

gchtr mentioned this pull request Aug 17, 2020

2.x post collections #2310

Closed

acobster force-pushed the 2.x-factories branch 3 times, most recently from 23c3421 to b0d086a Compare August 17, 2020 22:13

acobster mentioned this pull request Aug 17, 2020

2.x post collections #2311

Merged

Merge branch '2.x-factories' into 2.x-post-functions

11097e7

gchtr mentioned this pull request Aug 26, 2020

2.x posts api #2316

Merged

15 tasks

acobster pushed a commit that referenced this pull request Aug 28, 2020

#2093 port over changes from #2145

3fabc80

acobster closed this Sep 27, 2020

nlemoine deleted the 2.x-post-functions branch June 23, 2022 09:36

Uh oh!

Conversation

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

Uh oh!

Issue

Solution

Add Helper::doing_it_wrong()

Remove query methods that won’t work anymore

Deprecate Timber::query_post() and Timber::query_posts()

Return a PostCollection

Impact

Usage Changes

Considerations

Testing

Todo

Uh oh!

gchtr commented Jan 7, 2020

Uh oh!

acobster left a comment

Choose a reason for hiding this comment

Uh oh!

acobster left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gchtr commented Jan 18, 2020

Uh oh!

gchtr commented Jun 10, 2020

Uh oh!

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

Uh oh!

Uh oh!

jarednova commented Jun 17, 2020

Uh oh!

acobster commented Jun 17, 2020

Uh oh!

acobster commented Jun 17, 2020

Uh oh!

jarednova commented Jun 19, 2020

Uh oh!

jarednova left a comment

Choose a reason for hiding this comment

Uh oh!

acobster commented Sep 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gchtr commented Jan 6, 2020 •

edited

Loading

Add `Helper::doing_it_wrong()`

Deprecate `Timber::query_post()` and `Timber::query_posts()`

Return a `PostCollection`

acobster commented Jun 10, 2020 •

edited

Loading