#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 )
Attachments (27)
Change History (148)
#9
@
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
@
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.
#11
@
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
@
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:
↓ 14
@
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
@
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?
#16
follow-up:
↓ 18
@
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.
#17
@
12 years ago
- 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:
↓ 19
@
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.
#19
in reply to:
↑ 18
@
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.
#21
@
12 years ago
- 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
@
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
#23
@
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:
↓ 25
@
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
@
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
@
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.
#28
follow-up:
↓ 29
@
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
@
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?
#30
@
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
@
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
@
12 years ago
[UT969] demonstrates the need to do the filter checks when WP_Post is passed to get_post().
#33
follow-up:
↓ 34
@
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
@
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.
#35
@
12 years ago
21309.7.diff introduces WP_Post->filter(), which passes [UT969]. Also removed the unneeded call to update_post_caches().
#13
follow-up:
↓ 14
@
12 years ago
Stickies in WP_Query cause fatal errors now.
Steps to reproduce:
- Create more than posts_per_page posts.
- Make a post from page 2 sticky
- 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
@
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?
#15
follow-up:
↓ 19
@
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:
↓ 18
@
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
@
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!
#19
in reply to:
↑ 15
@
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
@
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
@
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.
#27
follow-up:
↓ 28
@
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:
↓ 29
@
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 DESCLooks 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
@
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
@
12 years ago
Ah, I missed that post_type was being passed appropriately to get_posts(). Looks good to me, then.
#36
@
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.
#39
@
12 years ago
Worth creating some get magic for things like this?
$post->page_template = get_post_meta( $id, '_wp_page_template', true );
#44
@
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:
↓ 46
@
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:
↓ 48
@
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.
#48
in reply to:
↑ 46
@
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
@
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().
@
12 years ago
Eliminates most direct calls to $post, by making get_post() take an *optional* $post argument.
#51
follow-up:
↓ 53
@
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
@
12 years ago
post-to-edit.2.diff merges WP_Post_To_Edit into WP_Post.
#53
in reply to:
↑ 51
@
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
@
12 years ago
I disagree - this ticket radically redefines the implementation of Post, auditing the global is reasonable
#55
@
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.
#62
@
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.
#66
@
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
@
12 years ago
21309.11.diff restores $post_ID
global in wp_popular_terms_checklist()
.
#68
@
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().
#71
@
12 years ago
[21952] removes post ancestor caching. A post's cache is now completely relied upon.
#72
@
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
@
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
@
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.
#75
@
12 years ago
get_the_terms.21309.2.diff adds the missing cache invalidation in wp_set_post_terms(). Tests pass now.
#80
@
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
#84
@
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
@
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.
#87
@
12 years ago
- Owner set to nacin
- Resolution set to fixed
- Status changed from reopened to closed
In 22564:
#89
in reply to:
↑ 77
@
12 years ago
Replying to ryan:
In [22011]:
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?
21309.diff is basically a refresh of ticket:16574:16574.8.diff: