Make WordPress Core

Opened 12 years ago

Closed 12 years ago

Last modified 8 years ago

#21309 closed task (blessed) (fixed)

Introduce WP_Post class

Reported by: scribu's profile scribu Owned by: nacin's profile 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 12 years ago.
21309.2.diff (11.5 KB) - added by scribu 12 years ago.
21309.3.diff (12.8 KB) - added by scribu 12 years ago.
21309.4.diff (13.1 KB) - added by scribu 12 years ago.
21309.5.diff (12.8 KB) - added by scribu 12 years ago.
21309.6.diff (12.8 KB) - added by scribu 12 years ago.
21309.7.diff (12.9 KB) - added by scribu 12 years ago.
21309-ut.diff (3.7 KB) - added by ryan 12 years ago.
21309-global.diff (480 bytes) - added by mdawaffe 12 years ago.
21309-stickies.diff (553 bytes) - added by mdawaffe 12 years ago.
21309.8.diff (2.2 KB) - added by nacin 12 years ago.
stickies-get_posts.diff (1.4 KB) - added by scribu 12 years ago.
21309.9.diff (14.6 KB) - added by ryan 12 years ago.
Make sure get_default_post_to_edit() always returns WP_Post. Remove ref returns.
21309-auto-draft.diff (584 bytes) - added by ryan 12 years ago.
21309-page_template.diff (638 bytes) - added by ryan 12 years ago.
post-to-edit.diff (6.4 KB) - added by scribu 12 years ago.
post-to-edit.tests.diff (681 bytes) - added by scribu 12 years ago.
21309-get_adjacent_post.diff (811 bytes) - added by ryan 12 years ago.
21309-get_adjacent_post-ut.diff (1.2 KB) - added by ryan 12 years ago.
21309.10.diff (32.3 KB) - added by nacin 12 years 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 12 years ago.
21309-ancestors-cache.diff (1.0 KB) - added by ryan 12 years ago.
21309.11.diff (674 bytes) - added by SergeyBiryukov 12 years ago.
get_the_terms.21309.diff (1.5 KB) - added by scribu 12 years ago.
get_the_terms.21309.2.diff (2.5 KB) - added by scribu 12 years ago.
21309.12.diff (625 bytes) - added by ryan 12 years ago.
Patch the symptom
21309.13.diff (791 bytes) - added by SergeyBiryukov 12 years ago.

Download all attachments as: .zip

Change History (148)

#1 @scribu
12 years ago

  • Description modified (diff)

@scribu
12 years ago

#2 follow-up: @scribu
12 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()

#3 @scribu
12 years ago

TODO:

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

#4 @toscho
12 years ago

  • Cc info@… added

#5 @sabreuse
12 years ago

  • Cc sabreuse@… added

#6 @obenland
12 years ago

  • Cc konstantin@… added

#7 @Mamaduka
12 years ago

  • Cc georgemamadashvili@… added

#8 @ryan
12 years ago

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

#9 @ryan
12 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?

#10 @ryan
12 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 methods for everything.

Last edited 12 years ago by ryan (previous) (diff)

#11 @scribu
12 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.

#12 in reply to: ↑ 2 @mikeschinkel
12 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;
 *
 */

#13 follow-up: @scribu
12 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.

#14 in reply to: ↑ 13 @nacin
12 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?

#15 follow-up: @ryan
12 years ago

get_pages() also needs its results cast to WP_Post.

#16 follow-up: @scribu
12 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.

@scribu
12 years ago

#17 @scribu
12 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

#18 in reply to: ↑ 16 ; follow-up: @mikeschinkel
12 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 12 years ago by mikeschinkel (previous) (diff)

#19 in reply to: ↑ 18 @scribu
12 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.

#20 @scribu
12 years ago

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

@scribu
12 years ago

#21 @scribu
12 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()

#22 @scribu
12 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 12 years ago by scribu (previous) (diff)

@scribu
12 years ago

#23 @ryan
12 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.

#24 follow-up: @ryan
12 years ago

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

#25 in reply to: ↑ 24 @scribu
12 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.

#26 @scribu
12 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 12 years ago by scribu (previous) (diff)

#27 follow-up: @johnbillion
12 years ago

  • Cc Johnbillion added

@scribu
12 years ago

#28 follow-up: @scribu
12 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.

#29 @ryan
12 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?

@scribu
12 years ago

#30 @scribu
12 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].

#31 @ryan
12 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?

#32 @ryan
12 years ago

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

#33 follow-up: @ryan
12 years ago

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

#34 in reply to: ↑ 33 @scribu
12 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.

@scribu
12 years ago

#35 @scribu
12 years ago

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

#36 @simonwheatley
12 years ago

  • Cc simon@… added

@ryan
12 years ago

#37 @ryan
12 years ago

In [21559]:

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

#39 @nacin
12 years ago

In [21567]:

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

#12 @nacin
12 years ago

In [21568]:

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

#13 follow-up: @mdawaffe
12 years 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.

#14 @mdawaffe
12 years 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?

@nacin
12 years ago

#15 follow-up: @nacin
12 years 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

#16 follow-up: @Viper007Bond
12 years 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


#17 @Viper007Bond
12 years 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!

#18 follow-up: @nacin
12 years ago

In [21569]:

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

#19 in reply to: ↑ 15 @mdawaffe
12 years 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 :)

#20 @scribu
12 years ago

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

#21 @scribu
12 years 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.

#22 @scribu
12 years ago

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

#23 @sirzooro
12 years ago

  • Cc sirzooro added

#24 follow-up: @ryan
12 years ago

In [21572]:

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

#26 @ryan
12 years ago

[UT988[

#27 follow-up: @ryan
12 years 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.

#28 in reply to: ↑ 27 ; follow-up: @nacin
12 years 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.

#29 in reply to: ↑ 28 @scribu
12 years 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.

#30 @nacin
12 years ago

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

#31 @obenland
12 years ago

  • Cc konstantin@… removed

#32 @ryan
12 years ago

In [21585]:

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

#33 follow-up: @SergeyBiryukov
12 years 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".

#34 @stephenh1988
12 years ago

  • Cc contact@… added

#35 @husobj
12 years ago

  • Cc ben@… added

@ryan
12 years ago

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

#36 @ryan
12 years 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.

#37 @ryan
12 years ago

In [21596]:

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

#38 @ryan
12 years 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

#39 @ryan
12 years ago

Worth creating some get magic for things like this?

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

#40 @ryan
12 years ago

Looks like get_adjacent_post() needs to return WP_Post.

#41 @ryan
12 years ago

In [21598]:

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

#42 @ryan
12 years ago

In [21599]:

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

#43 @ryan
12 years ago

In [21601]:

Remove typo in phpdoc. see #21309 Props TobiasBg

@scribu
12 years ago

#44 @scribu
12 years 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.

#45 follow-up: @ryan
12 years 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.

#46 in reply to: ↑ 45 ; follow-up: @scribu
12 years 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.

#47 @ryan
12 years ago

In [21627]:

Return WP_Post from get_adjacent_post(). see #21309

#48 in reply to: ↑ 46 @ryan
12 years 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.

#49 @scribu
12 years 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().

@nacin
12 years ago

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

#51 follow-up: @nacin
12 years 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.

#52 @scribu
12 years ago

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

#53 in reply to: ↑ 51 @scribu
12 years 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.

#54 @wonderboymusic
12 years ago

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

#55 @scribu
12 years 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 12 years ago by scribu (previous) (diff)

#56 @ryan
12 years 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

#58 @ryan
12 years ago

In [21654]:

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

#60 @ryan
12 years ago

In [21655]:

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

#61 @ryan
12 years ago

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

#62 @scribu
12 years 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 12 years ago by scribu (previous) (diff)

#63 @ryan
12 years 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

#64 @nacin
12 years 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.

#65 @ryanduff
12 years ago

  • Cc ryan@… added

#66 @SergeyBiryukov
12 years 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

#67 @SergeyBiryukov
12 years ago

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

#68 @ryan
12 years 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().

#69 @ryan
12 years ago

Or send along the post_category array to wp_popular_terms_checklist().

#70 @ryan
12 years 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

#71 @nacin
12 years ago

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

#72 @scribu
12 years 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.

#73 @scribu
12 years 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.

#74 @scribu
12 years 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 12 years ago by scribu (previous) (diff)

#75 @scribu
12 years ago

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

#76 @ryan
12 years 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

#77 follow-up: @ryan
12 years 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

#78 @sc0ttkclark
12 years ago

  • Cc lol@… added

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

#79 @ryan
12 years ago

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

#80 @ryan
12 years ago

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

#81 @ryan
12 years ago

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

Last edited 12 years ago by ryan (previous) (diff)

@ryan
12 years ago

Patch the symptom

#82 @ryan
12 years ago

In [22163]:

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

#83 @nacin
12 years ago

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

#84 @miqrogroove
12 years 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.

#85 @miqrogroove
12 years 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.

#86 @miqrogroove
12 years ago

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

#87 @nacin
12 years 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.

#88 @nacin
12 years ago

New tickets for anything else, please. Thanks!

#89 in reply to: ↑ 77 @ntm
12 years 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?

#90 @ntm
12 years ago

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

#91 @ntm
12 years ago

The new Ticket is #22448.

#92 @DrewAPicture
10 years ago

#14302 was marked as a duplicate.

This ticket was mentioned in Slack in #core by boone. View the logs.


8 years ago

Note: See TracTickets for help on using tickets.