WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 20 months ago

Last modified 20 months ago

#21309 closed task (blessed) (fixed)

Introduce WP_Post class

Reported by: scribu Owned by: nacin
Milestone: 3.5 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch
Focuses: Cc:

Description (last modified by scribu)

In the wise words of Ryan Boren:

Just do what we did with WP_User. Add some magic methods so we can load things like ancestors on demand and get better cache behavior. Make sure everything is back compat. Do more some other time.

Previously: #12267, #16574

Attachments (27)

21309.diff (7.9 KB) - added by scribu 2 years ago.
21309.2.diff (11.5 KB) - added by scribu 2 years ago.
21309.3.diff (12.8 KB) - added by scribu 2 years ago.
21309.4.diff (13.1 KB) - added by scribu 2 years ago.
21309.5.diff (12.8 KB) - added by scribu 2 years ago.
21309.6.diff (12.8 KB) - added by scribu 2 years ago.
21309.7.diff (12.9 KB) - added by scribu 2 years ago.
21309-ut.diff (3.7 KB) - added by ryan 23 months ago.
21309-global.diff (480 bytes) - added by mdawaffe 23 months ago.
21309-stickies.diff (553 bytes) - added by mdawaffe 23 months ago.
21309.8.diff (2.2 KB) - added by nacin 23 months ago.
stickies-get_posts.diff (1.4 KB) - added by scribu 23 months ago.
21309.9.diff (14.6 KB) - added by ryan 23 months ago.
Make sure get_default_post_to_edit() always returns WP_Post. Remove ref returns.
21309-auto-draft.diff (584 bytes) - added by ryan 23 months ago.
21309-page_template.diff (638 bytes) - added by ryan 23 months ago.
post-to-edit.diff (6.4 KB) - added by scribu 23 months ago.
post-to-edit.tests.diff (681 bytes) - added by scribu 23 months ago.
21309-get_adjacent_post.diff (811 bytes) - added by ryan 23 months ago.
21309-get_adjacent_post-ut.diff (1.2 KB) - added by ryan 23 months ago.
21309.10.diff (32.3 KB) - added by nacin 23 months ago.
Eliminates most direct calls to $post, by making get_post() take an *optional* $post argument.
post-to-edit.2.diff (12.9 KB) - added by scribu 23 months ago.
21309-ancestors-cache.diff (1.0 KB) - added by ryan 23 months ago.
21309.11.diff (674 bytes) - added by SergeyBiryukov 23 months ago.
get_the_terms.21309.diff (1.5 KB) - added by scribu 22 months ago.
get_the_terms.21309.2.diff (2.5 KB) - added by scribu 22 months ago.
21309.12.diff (625 bytes) - added by ryan 21 months ago.
Patch the symptom
21309.13.diff (791 bytes) - added by SergeyBiryukov 20 months ago.

Download all attachments as: .zip

Change History (146)

comment:1 scribu2 years ago

  • Description modified (diff)

scribu2 years ago

comment:2 follow-up: scribu2 years ago

  • Keywords has-patch added

21309.diff is basically a refresh of ticket:16574:16574.8.diff:

  • merge _get_post_ancestors() into get_post_ancestors()
  • use get_posts() in get_post_ancestors()
  • introduce WP_Post class with magic method that calls get_post_ancestors()

comment:3 scribu2 years ago

TODO:

  • simplify logic in get_post();
  • unit tests?

comment:4 toscho2 years ago

  • Cc info@… added

comment:5 sabreuse2 years ago

  • Cc sabreuse@… added

comment:6 obenland2 years ago

  • Cc konstantin@… added

comment:7 Mamaduka2 years ago

  • Cc georgemamadashvili@… added

comment:8 ryan2 years ago

Update query.php to cast the stdClass posts in the ::posts list to WP_Post?

comment:9 ryan2 years ago

Use real properties with a get_instance() method as done in http://core.trac.wordpress.org/attachment/ticket/12267/wp-post-patch-take-2.diff?

comment:10 ryan2 years ago

Discussed with scribu in IRC and the advantage of the data blob is that we get __get() for all "properties". There we can check the filter property as we do in WP_User and run sanitizers without having to write individual get method for everything.

Version 1, edited 2 years ago by ryan (previous) (next) (diff)

comment:11 scribu2 years ago

Update query.php to cast the stdClass posts in the ::posts list to WP_Post?

This is already handled if $split_query is true:

$this->posts = array_map( 'get_post', $ids );

but not for the other case. Will fix.

comment:12 in reply to: ↑ 2 mikeschinkel2 years ago

  • Cc mikeschinkel@… added

Replying to scribu:

21309.diff is basically a refresh of ticket:16574:16574.8.diff:

  • merge _get_post_ancestors() into get_post_ancestors()
  • use get_posts() in get_post_ancestors()
  • introduce WP_Post class with magic method that calls get_post_ancestors()

I really like what you've done with this patch.

Can I suggest changing $post to 'protected' so a subclass can access it, and add @property PHPDoc comments so that we can get auto-complete in editors like PhpStorm (and so it won't flag those properties as errors?):

/**
 * Wrapper class to preserve back-compat for $post->ancestors
 *
 * @since 3.5.0
 *
 * @property $ID;
 * @property $post_author;
 * @property $post_date;
 * @property $post_date_gmt;
 * @property $post_content;
 * @property $post_title;
 * @property $post_excerpt;
 * @property $post_status;
 * @property $comment_status;
 * @property $ping_status;
 * @property $post_password;
 * @property $post_name;
 * @property $to_ping;
 * @property $pinged;
 * @property $post_modified;
 * @property $post_modified_gmt;
 * @property $post_content_filtered;
 * @property $post_parent;
 * @property $guid;
 * @property $menu_order;
 * @property $post_type;
 * @property $post_mime_type;
 * @property $comment_count;
 * @property $filter;
 *
 */

comment:13 follow-up: scribu2 years ago

Can I suggest changing $post to 'protected' so a subclass can access it,

You can have another decorator class instead, i.e. have a My_WP_Post class that contains a WP_Post instance, just like WP_Post contains an instance of a stdClass.

add @property PHPDoc comments so that we can get auto-complete in editors like PhpStorm

Sure.

comment:14 in reply to: ↑ 13 nacin2 years ago

Replying to scribu:

You can have another decorator class instead, i.e. have a My_WP_Post class that contains a WP_Post instance, just like WP_Post contains an instance of a stdClass.

If that's the design pattern we're going to suggest for now, should WP_Post be made final to protect us in doing future changes?

comment:15 follow-up: ryan2 years ago

get_pages() also needs its results cast to WP_Post.

comment:16 follow-up: scribu2 years ago

If that's the design pattern we're going to suggest for now, should WP_Post be made final to protect us in doing future changes?

Yes, if we're going to go with composition instead of inheritance, we should use the final keyword. We can remove it later, if necessary.

get_pages() also needs its results cast to WP_Post.

Also, it has a funky 'get_pages' cache, which will need some work.

scribu2 years ago

comment:17 scribu2 years ago

21309.2.diff:

  • call sanitize_post_field() in __get()
  • always cast to WP_Post in WP_Query and get_pages()
  • make WP_Post final and add @property list
  • introduce WP_Post::get_instance() and make constructor private

comment:18 in reply to: ↑ 16 ; follow-up: mikeschinkel2 years ago

Replying to scribu:

If that's the design pattern we're going to suggest for now, should WP_Post be made final to protect us in doing future changes?

Yes, if we're going to go with composition instead of inheritance, we should use the final keyword. We can remove it later, if necessary.

This worries me greatly, but I assume there is a discussion thread that details the decision somewhere? Would you mind providing a link?

Reasons it worries me is I would like to be able do doing something like this:

class Event_Post extends WP_Post {
  var $start_time;
  var $end_time;
  function duration() {
     return $this->end_time - $this->start_time;
  }
}

And then being able to do things like this:

foreach( $posts as $post ) {
  if ( is_a( $post, 'WP_Post') ) {
     // Do something that requires a WP_Post object
  }
}

I can simulate the former with composition, but not in a reliable way, but I can't do the latter without composition or using conventions that not everyone will follow.

Oh, and it also adds overhead to every call.

Last edited 2 years ago by mikeschinkel (previous) (diff)

comment:19 in reply to: ↑ 18 scribu2 years ago

Replying to mikeschinkel:

This worries me greatly, but I assume there is a discussion thread that details the decision somewhere?

There isn't, so here goes:

With the current stdClass objects, you are forced to use composition already, so there's nothing lost and nothing gained.

Maybe we come up with a more flexible extension mechanism than composition or inheritance, but that's outside the scope for 3.5.

comment:20 scribu2 years ago

Added some unit tests for get_post(): [UT939]

scribu2 years ago

comment:21 scribu2 years ago

21309.3.diff:

  • make WP_Post::get_instance() only accept a post id and keep the complicated logic in get_post()
  • make WP_Post::__construct() public, out of necessity, but also since you can change any field anyway
  • clean up get_post_field()

comment:22 scribu2 years ago

How about adding some more magic for custom fields, like we did in WP_User:

$some_value = $post->some_key;

would be equivalent to:

$some_value = sanitize_post_field( 'some_key', get_post_meta( $post->ID, 'some_key', true ), $post->ID, 'display' );

Benefits:

  • increased conciseness and readability
  • automatic sanitization, thus better security
  • consistency with WP_User
Last edited 22 months ago by scribu (previous) (diff)

scribu2 years ago

comment:23 ryan2 years ago

Both WP_Post->filter and WP_Post->post->filter exist. I guess because of to_array().

get_object_vars() will no longer return much if fed WP_Post. We use get_object_vars() in get_children(), sanitize_post(), wp_get_recent_posts(), wp_update_post(), _wp_put_post_revision(), wp_get_post_revision(), and elsewhere. I notice that several plugins including the bbPress plugin also do this.

comment:24 follow-up: ryan2 years ago

Magic for ->post_category, tags_input, and tax_input would allow us to deprecate wp_get_single_post().

comment:25 in reply to: ↑ 24 scribu2 years ago

Replying to ryan:

Magic for ->post_category, tags_input, and tax_input would allow us to deprecate wp_get_single_post().

I don't know. We should keep WP_Post->__get() fairly light, as it gets called a lot.

Both WP_Post->filter and WP_Post->post->filter exist. I guess because of to_array().

Hm... I didn't notice that; will investigate.

get_object_vars() will no longer return much if fed WP_Post. We use get_object_vars() in get_children(), sanitize_post(), wp_get_recent_posts(), wp_update_post(), _wp_put_post_revision(), wp_get_post_revision(), and elsewhere.

Right, will have to fix those.

I notice that several plugins including the bbPress plugin also do this.

It was the same situation when we started returning WP_User everywhere. There's nothing we can do, except encouraging the authors to use get_post( $id, ARRAY_A ) instead.

comment:26 scribu2 years ago

On the other hand, WP_User already had a $data property, so it made sense to remove duplication by relying entirely on __get(). Real properties on WP_Post might be a good idea yet.

Last edited 2 years ago by scribu (previous) (diff)

comment:27 follow-up: johnbillion2 years ago

  • Cc Johnbillion added

scribu2 years ago

comment:28 follow-up: scribu2 years ago

21309.5.diff stores the properties directly on the WP_Post instance, which means that get_object_vars() works again and it also fixes the doubled 'filter' property.

comment:29 ryan2 years ago

How does filtering work for the properties? __get() handles meta, but I'm failing to see what happens for properties. Do we need a filter() method to set the filter and run sanitize_post() on the properties?

scribu2 years ago

comment:30 scribu2 years ago

We don't need a filter() method, since get_object_vars() works now. I just needed to add back the sanitize_post() call at the end of get_post: 21309.6.diff and [UT961].

comment:31 ryan2 years ago

Does the is_a( $post, 'WP_Post' ) branch in get_post() need the filter checks performed in is_object( $post ) now that we aren't using magic methods?

comment:32 ryan2 years ago

[UT969] demonstrates the need to do the filter checks when WP_Post is passed to get_post().

comment:33 follow-up: ryan2 years ago

What's the extra update_post_caches() call in query.php for? It doesn't check $q[ 'cache_results' ].

comment:34 in reply to: ↑ 33 scribu2 years ago

Replying to ryan:

[UT969] demonstrates the need to do the filter checks when WP_Post is passed to get_post().

On it.

What's the extra update_post_caches() call in query.php for? It doesn't check $q[ 'cache_results' ].

At some point, get_post() forced another query, even when you passed a raw post column; that's not the case anymore, so I'll remove it.

scribu2 years ago

comment:35 scribu2 years ago

21309.7.diff introduces WP_Post->filter(), which passes [UT969]. Also removed the unneeded call to update_post_caches().

comment:36 simonwheatley2 years ago

  • Cc simon@… added

ryan23 months ago

comment:37 ryan23 months ago

In [21559]:

Introduce WP_Post class. Clean up ancestors handling. Props scribu, toppa. fixes #10381 see #21309

comment:39 nacin23 months ago

In [21567]:

Return a variable reference from get_pages(). see [21559], see #21309, fixes #20756.

comment:12 nacin23 months ago

In [21568]:

Restore _get_post_ancestors() in deprecated.php to prevent fatal errors. see #21309.

comment:13 follow-up: mdawaffe23 months ago

Stickies in WP_Query cause fatal errors now.

Steps to reproduce:

  1. Create more than posts_per_page posts.
  2. Make a post from page 2 sticky
  3. View your home page.

PHP Fatal error: Call to undefined method stdClass::filter() in /Users/mdawaffe/Sites/wordpress/wp-includes/post.php on line 404

Attached 21309-stickies.diff.

comment:14 mdawaffe23 months ago

get_post() does not handle the case when no arguments are passed and $GLOBALS['post'] is not a WP_Post. I guarantee code exists that manually sets the global.

To reproduce:

$post = $wpdb->get_row( "SELECT * FROM `$wpdb->posts` WHERE `ID` = 10047" );
$post_id = 0;
var_dump( get_post( $post_id ) );

PHP Fatal error: Call to undefined method stdClass::filter() in /Users/mdawaffe/Sites/wordpress/wp-includes/post.php on line 404

Attached a possible solution in 21309-global.diff. I'm not sure what the best thing to do here is. Should the global be modified to be a WP_Post object? What will that do to anyone depending on odd reference behavior?

mdawaffe23 months ago

mdawaffe23 months ago

nacin23 months ago

comment:15 follow-up: nacin23 months ago

get_post() does not handle the case when no arguments are passed and $GLOBALSpost? is not a WP_Post. I guarantee code exists that manually sets the global.

Yeah, I'm looking at that same thing. I was playing with 21309.8.diff. I do think we should definitely modify the global.

Perhaps it is time to break the reference in get_post( &$post ). There's also no good reason to have it return by reference anymore either — objects are byref in PHP5. Doing both of those things should

comment:16 follow-up: Viper007Bond23 months ago

The front page of my site is currently fataling with no plugins enabled and Twenty Twelve enabled.

Fatal error: Call to undefined method stdClass::filter() in [path]\wp-includes\post.php on line 404
Call Stack
#	Time	Memory		Function														Location
1	0.0003	335776		{main}( )														..\index.php:0
2	0.0005	339496		require( '[path]\wp-blog-header.php' )							..\index.php:17
3	0.1825	19693752	require_once( '[path]\wp-includes\template-loader.php' )		..\wp-blog-header.php:16
4	0.1840	19710312	include( '[path]\wp-content\themes\twentytwelve\index.php' )	..\template-loader.php:43
5	0.2176	19792368	get_post_format( )												..\index.php:24
6	0.2176	19792400	get_post( )														..\post.php:611


comment:17 Viper007Bond23 months ago

Trac is going crazy right now with comment numbering so I can't edit my comment but I just realized this is exactly what mdawaffe is talking about, heh. Yes, I have a sticky. :)

I did a terrible job of skimming recent comments. Sorry!

comment:18 follow-up: nacin23 months ago

In [21569]:

Ensure sticky posts are WP_Post objects. props mdawaffe. see #21309.

comment:19 in reply to: ↑ 15 mdawaffe23 months ago

Replying to nacin:

Yeah, I'm looking at that same thing. I was playing with 21309.8.diff. I do think we should definitely modify the global.

Perhaps it is time to break the reference in get_post( &$post ). There's also no good reason to have it return by reference anymore either — objects are byref in PHP5. Doing both of those things should

21309.8.diff is much prettier than my attempt.

I'm all for getting rid of all the referencing. Here's an oldie but goodie about how these references are probably harmful. http://blog.golemon.com/2007/01/youre-being-lied-to.html

Unless things have changed in the last 5 years. I haven't looked :)

comment:20 scribu23 months ago

Shouldn't we use _prime_post_caches() after doing that direct query to get stickies? Or why not just use get_posts()?

scribu23 months ago

comment:21 scribu23 months ago

stickies-get_posts.diff avoids a lot of duplicate logic by using get_posts() to fetch the sticky posts. Since get_posts() sets 'ignore_sticky_posts' => true, Inception is avoided.

comment:22 scribu23 months ago

Also, +1 for removing references from get_post() and get_page().

comment:23 sirzooro23 months ago

  • Cc sirzooro added

comment:24 follow-up: ryan23 months ago

In [21572]:

Remove references from get_post() and get_page().
Handle $GLOBALSpost? containing stdClass instead of WP_Post.
Props nacin
see #21309

comment:26 ryan23 months ago

[UT988[

comment:27 follow-up: ryan23 months ago

Before stickies-get_posts.diff:

SELECT * FROM wp_trunk_posts WHERE wp_trunk_posts.ID IN (2131) 

After:

SELECT wp_trunk_posts.* FROM wp_trunk_posts WHERE 1=1 AND wp_trunk_posts.ID IN (2131) AND wp_trunk_posts.post_type = 'post' AND (wp_trunk_posts.post_status = 'publish') ORDER BY wp_trunk_posts.post_date DESC

Looks sane to me.

comment:28 in reply to: ↑ 27 ; follow-up: nacin23 months ago

Replying to ryan:

Before stickies-get_posts.diff:

SELECT * FROM wp_trunk_posts WHERE wp_trunk_posts.ID IN (2131) 

After:

SELECT wp_trunk_posts.* FROM wp_trunk_posts WHERE 1=1 AND wp_trunk_posts.ID IN (2131) AND wp_trunk_posts.post_type = 'post' AND (wp_trunk_posts.post_status = 'publish') ORDER BY wp_trunk_posts.post_date DESC

Looks sane to me.

Might break some situations where someone is using stick_post() on a non-post.

That first query is faster, and could be replaced with _prime_post_caches(), though the order-by date is tempting.

comment:29 in reply to: ↑ 28 scribu23 months ago

Might break some situations where someone is using stick_post() on a non-post.

Example?

That first query is faster, and could be replaced with _prime_post_caches(), though the order-by date is tempting.

Not really. If you look at the code that the patch removes, you'd still have to:

1) check the post type
2) check the post status
3) order by date

In other words, exactly what the new query does.

comment:30 nacin23 months ago

Ah, I missed that post_type was being passed appropriately to get_posts(). Looks good to me, then.

comment:31 obenland23 months ago

  • Cc konstantin@… removed

comment:32 ryan23 months ago

In [21585]:

Use get_posts() to fetch stickies rather than custom bare SQL. Props scribu. see #21309

comment:33 follow-up: SergeyBiryukov23 months ago

The bug with "Auto Draft" page titles (mentioned in the dev chat) was introduced in [21572].

Once get_post() is called in edit-form-advanced.php (via get_permalink()), the empty $post->post_title from get_default_post_to_edit() is overridden with "Auto Draft".

comment:34 stephenh198823 months ago

  • Cc contact@… added

comment:35 husobj23 months ago

  • Cc ben@… added

ryan23 months ago

Make sure get_default_post_to_edit() always returns WP_Post. Remove ref returns.

comment:36 ryan23 months ago

_get_page_link() is overwriting global $post with the version from the cache. Since get_default_post_to_edit() changes a few properties in the post object after the post is created and cached, global $post ends up not being the same as what is in the cache. We need to preserve the global $post in edit-form-advanced.php. 3.4 must have been poisoning the cache with the hacked global $post.

ryan23 months ago

comment:37 ryan23 months ago

In [21596]:

Don't overwrite the post global in _get_page_link(). see #21309

comment:38 ryan23 months ago

In [21597]:

Remove return ref from all calls to get_post()
Return WP_Post from get_default_post_to_edit()
Replace all calls to get_page() with get_post()
see #21309

comment:39 ryan23 months ago

Worth creating some get magic for things like this?

$post->page_template = get_post_meta( $id, '_wp_page_template', true );

comment:40 ryan23 months ago

Looks like get_adjacent_post() needs to return WP_Post.

comment:41 ryan23 months ago

In [21598]:

Update phpdoc for functions that return WP_Post. Soft deprecate get_page(). see #21309

comment:42 ryan23 months ago

In [21599]:

Use get_post() instead of bare SQL in do_trackbacks(). see #21309

ryan23 months ago

comment:43 ryan23 months ago

In [21601]:

Remove typo in phpdoc. see #21309 Props TobiasBg

scribu23 months ago

scribu23 months ago

comment:44 scribu23 months ago

post-to-edit.diff is a preliminary patch that:

  • introduces the WP_Post_To_Edit class
  • deprecates wp_get_single_post() and get_post_to_edit() (but doesn't stop using them yet)

post-to-edit.tests.diff contains some preliminary unit tests.

comment:45 follow-up: ryan23 months ago

I'm still not seeing the need for WP_Post_To_Edit. Providing these things via __get() is so cheap it is practically free. I'm all for keeping WP_Post light, but this seems very much a part of its purpose as a struct with some smarts.

comment:46 in reply to: ↑ 45 ; follow-up: scribu23 months ago

Providing these things via __get() is so cheap it is practically free.

Even when people start to use $post->some_custom_field everywhere? The additional checks would have to be done before calling get_post_meta().

The idea of WP_Post_To_Edit is that it's something you pass to wp_insert_post(), which could contain some additional logic in the future.

comment:47 ryan23 months ago

In [21627]:

Return WP_Post from get_adjacent_post(). see #21309

comment:48 in reply to: ↑ 46 ryan23 months ago

Replying to scribu:

Providing these things via __get() is so cheap it is practically free.

Even when people start to use $post->some_custom_field everywhere? The additional checks would have to be done before calling get_post_meta().

We'll probably have to put checks in there to reserve these keywords anyhow.

comment:49 scribu23 months ago

In that case, nevermind. I'll work on a finished patch that removes all the existing calls to wp_get_single_post() and get_post_to_edit().

nacin23 months ago

Eliminates most direct calls to $post, by making get_post() take an *optional* $post argument.

comment:51 follow-up: nacin23 months ago

21309.10.diff is a (tedious) experiment to eliminate nearly all $post references, making get_post() return the global $post if no arguments are passed. The list tables should be smarter, and instead of using outside global state, the methods should be passed a post object.

Clarifies that a number of functions can take either an ID or an object. #20495 likely covers more instances.

scribu23 months ago

comment:52 scribu23 months ago

post-to-edit.2.diff merges WP_Post_To_Edit into WP_Post.

comment:53 in reply to: ↑ 51 scribu23 months ago

Replying to nacin:

21309.10.diff is a (tedious) experiment to eliminate nearly all $post references, making get_post() return the global $post if no arguments are passed.

I feel that patch is way outside the scope of this ticket and should be moved to it's own ticket.

comment:54 wonderboymusic23 months ago

I disagree - this ticket radically redefines the implementation of Post, auditing the global is reasonable

comment:55 scribu23 months ago

Back in http://core.trac.wordpress.org/ticket/16574#comment:18, there was a problem with __get() issuing a notice, caused by get_post() using references.

Since that's no longer the case, I'm thinking it would be good to convert function &__get() back to function __get() since it masks errors related to arrays. For example:

$post = get_post( ... );
$post->ancestors[] = 111;

This will fail silently, which is bad.

Last edited 23 months ago by scribu (previous) (diff)

comment:56 ryan23 months ago

In [21651]:

Add tags_input, page_template, and post_category get magic to WP_Post.
Deprecate get_post_to_edit() and wp_get_single_post().
Props scribu
see #21309

comment:58 ryan23 months ago

In [21654]:

Lose return ref from WP_Post::get(). see #21309

comment:60 ryan23 months ago

In [21655]:

Simplify return from WP_Post::get() now that references are no longer returned. see #21309

comment:61 ryan23 months ago

Did we decide to switch ancestors to a persistent cache bucket?

comment:62 scribu23 months ago

I think the conclusion was that we should try flipping the switch to make it persistent. Then nacin will jump in and say we should use the 'posts' bucket, which I'm fine with, if you guys say that makes things easier.

Last edited 23 months ago by scribu (previous) (diff)

comment:63 ryan23 months ago

In [21735]:

Use get_post() instead of global $post.
Make the $post argument to get_post() optional, defaulting to the current post in The Loop.

Props nacin
see #21309

comment:64 nacin23 months ago

In [21736]:

Restore global references that broke the media and comment list tables. Todo, make list tables rely far less on global state. see #21309.

comment:65 ryanduff23 months ago

  • Cc ryan@… added

comment:66 SergeyBiryukov23 months ago

Since [21735], creating a new category on Edit Post screen throws a notice:

Notice: Trying to get property of non-object in wp-admin/includes/template.php on line 170

SergeyBiryukov23 months ago

comment:67 SergeyBiryukov23 months ago

21309.11.diff restores $post_ID global in wp_popular_terms_checklist().

comment:68 ryan22 months ago

$post_ID is empty too. No post info is sent in $_POST to _wp_ajax_add_hierarchical_term(). Either we need to send along the post ID or we check for an empty return from get_post() in wp_popular_terms_checklist().

comment:69 ryan22 months ago

Or send along the post_category array to wp_popular_terms_checklist().

comment:70 ryan22 months ago

In [21791]:

Check for an empty post in wp_popular_terms_checklist(). _wp_ajax_add_hierarchical_term() doesn't set up global post info. Props SergeyBiryukov. see #21309

comment:71 nacin22 months ago

[21952] removes post ancestor caching. A post's cache is now completely relied upon.

comment:72 scribu22 months ago

nacin, in IRC:

I noticed some issues in WP_Post's __get() while looking into this.

wp_get_post_tags(), wp_get_post_categories() are not cached. They call wp_get_object_terms() ultimately.

previously, someone would call wp_get_recent_posts() and those would each be called once and set against the object. now, it happens on every access.

I'm investigating if we could move the caching from get_the_terms() into wp_get_object_terms() and fix things like #18968 in the process.

comment:73 scribu22 months ago

Yeah, so the problem is that wp_get_object_terms() has a bunch of additional parameters, like 'fields', 'orderby' etc., which would be hard to cache and especially to invalidate.

comment:74 scribu22 months ago

get_the_terms.21309.diff uses get_the_terms().

However, the unit tests fail because the $taxonomy . '_relationships cache isn't invalidated:

wp_set_post_categories( $post_id, array( $term1['term_id'], $term2['term_id'], $term3['term_id'] ) );
$this->assertEquals( 3, count( $post->post_category ) );

The cached value is still array(), instead of either not existing or containing the appropriate terms.

Last edited 22 months ago by scribu (previous) (diff)

comment:75 scribu22 months ago

get_the_terms.21309.2.diff adds the missing cache invalidation in wp_set_post_terms(). Tests pass now.

comment:76 ryan22 months ago

In [21981]:

Fetch full terms for the post_category and tags_input queries and then wp_list_pluck() the desired fields. Fetching full terms primes the cache and reduces overall queries. Add cache invalidation to wp_set_post_terms(). Props scribu. see #21309

comment:77 follow-up: ryan22 months ago

In [22011]:

Convert the object in the posts array to WP_Post only if the posts array is not empty. Some post caching plugins can cause it to be empty. see #21309

comment:78 sc0ttkclark22 months ago

  • Cc lol@… added

OMG can't wait to start using this! Great job guys!

comment:79 ryan21 months ago

  • Resolution set to fixed
  • Status changed from new to closed

comment:80 ryan21 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened
Argument #2 should be an array in /.../wp-includes/post.php on line 3498

comment:81 ryan21 months ago

I am sometimes seeing 1 as the value of $cache[ 'key' ]. Strange.

Last edited 21 months ago by ryan (previous) (diff)

ryan21 months ago

Patch the symptom

comment:82 ryan21 months ago

In [22163]:

Make sure cache bucket is an array to avoid warning. see #21309

comment:83 nacin21 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

comment:84 miqrogroove20 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Hey guys, you've broken the pre_get_shortlink filter by removing the $id argument in the_shortlink().

I can try to work around this, but it is incompatible with existing code.

SergeyBiryukov20 months ago

comment:85 miqrogroove20 months ago

It looks like the intention is to now require the filter callback to run get_post() when $context != 'query'. This is easily done. I just wanted to point out the change in behavior and potential breakage for any filters that weren't designed this way.

comment:86 miqrogroove20 months ago

Sergey's patch would restore the old behavior and prevent breakage. +1

comment:87 nacin20 months ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from reopened to closed

In 22564:

Pass the post ID from the_shortlink() to wp_get_shortlink() to avoid a change in filters. props SergeyBiryukov, fixes #21309.

comment:88 nacin20 months ago

New tickets for anything else, please. Thanks!

comment:89 in reply to: ↑ 77 ntm20 months ago

Replying to ryan:

In [22011]:

Convert the object in the posts array to WP_Post only if the posts array is not empty. Some post caching plugins can cause it to be empty. see #21309

I started testing with the plugin I maintain and WP 3.5 beta 3. The plugin extends the array $this->posts via the filter the_posts (wp-includes/query.php - function get_posts()). But the array_map() command which has been added with the Changeset 22011

$this->posts = array_map( 'get_post', $this->posts );

set the $this->posts array back to the content it has contained before the filter.
It retrogrades the_posts filter in my case.

Is it possible to move the array_map() command some lines up or the filter down?

comment:90 ntm20 months ago

Sorry, I have read the last comment just now. I will open a new Ticket.

comment:91 ntm20 months ago

The new Ticket is #22448.

Note: See TracTickets for help on using tickets.