Opened 15 years ago
Closed 2 years ago
#12267 closed enhancement (maybelater)
Upgrade loop objects to provide identical presentational interfaces
Reported by: | andy | Owned by: | |
---|---|---|---|
Milestone: | Priority: | normal | |
Severity: | normal | Version: | |
Component: | General | Keywords: | has-patch |
Focuses: | Cc: |
Description
Usually wpdb returns rows as stdClass objects. We are used to getting properties from these objects, e.g. $post->ID or $comment->comment_ID, but this class has no methods. As used, the stdClass object is only a syntactic alternative to the array.
As long as we're already using objects, let's have some more useful classes. I propose post and comment classes that implement common interfaces, and classes that extend these for special post_types and comment_types, and filters to allow plugins to use their own classes at instantiation time.
Without actually using PHP5 interface syntax, the idea is to have identical methods to get things from objects in the loop. For example, one common method would be url()
. The same method would work on every kind of compatible object, be it a post, page, attachment, comment, trackback, or pingback, although the underlying logic for getting the URL may differ for each.
<a href="<?php print esc_attr($post->url()); ?>">
It would simplify templates while allowing various object types in loops, not just posts, and give us an opportunity to clean up a lot of the underlying template tag logic (e.g. global $authordata), and give plugins and themes new ways to modify output.
This stemmed from my work on search. I wanted a way to keep the template simple while adding support for different object types in the loop. I figured that it wouldn't hurt anything to upgrade the classes because the way of accessing properties would be unchanged.
Attachments (6)
Change History (85)
#1
@
15 years ago
wp-classes.diff: this works, as far as creating the objects and some of the methods. Not all of the methods are tested and more must be added. (They certainly should be cleaner, more filtered, better coded, etc.) I'm using it right now with a search plugin that does something uncommon. The query gets wp_posts.* and wp_comments.* and returns data like this:
ID | post_title | post_type | ... | comment_ID | comment_post_ID | comment_author |
1 | Welcome | post | ... | NULL | NULL | NULL |
NULL | NULL | NULL | ... | 1 | 1 | Mr. WordPress |
2 | A Page | page | ... | NULL | NULL | NULL |
This is how results can be mixed from two separate tables. (It's also why tables should never include the same column name!) These rows are correctly converted into the wp_post or wp_comment classes because the NULL value causes isset($post->post_type)
to return false.
With a few modifications to the loop template I was able to get comments to appear in the same loop with posts.
#3
follow-up:
↓ 4
@
15 years ago
I think it's a great idea for each post/comment object to have getter methods. It's something I've done in several projects.
Here are some concerns about this specific proposal (now committed code):
- It's not obvious why the "Interface" of the particular object types is also the "factory." It's somewhat odd in theory, it goes against the allowed behavior of real PHP Interfaces, and it forces you to do some code gymnastics. Rather, it would be better to have an independent factory with that as its single purpose. By "better" I mean it would make the code easier to understand, simpler to test and more flexible to use.
- The "Interface" should have a name that reflects its behavior and structure rather than what happens to be the source of the data, in this case a database "row." "Rows" don't usually have permalinks, dates, or titles. Maybe something like "Loop_Object"?
- The WP_Error object should not be one of the possible objects. A WP_Error object violates the "Interface" implementation, and WP usually doesn't return error objects in the context of themes.
- Method names should follow the WP convention of being prefixed by "get_", since they get the values, not print them.
- Class names should follow the WP convention of being capitalized.
#4
in reply to:
↑ 3
@
15 years ago
Replying to filosofo:
- It's not obvious why the "Interface" of the particular object types is also the "factory."
It was my first time identifying the factory pattern. I am quite sure that my patch can be improved here.
- The "Interface" should have a name that reflects its behavior and structure rather than what happens to be the source of the data, in this case a database "row." "Rows" don't usually have permalinks, dates, or titles. Maybe something like "Loop_Object"?
Loop_Object is okay. Since the purpose of the classes is to unify the presentation interfaces, how about Presentable?
- The WP_Error object should not be one of the possible objects. A WP_Error object violates the "Interface" implementation, and WP usually doesn't return error objects in the context of themes.
True. When I wrote that I wanted to support wp_post::get($post_id). It's probably better without it.
- Method names should follow the WP convention of being prefixed by "get_", since they get the values, not print them.
- Class names should follow the WP convention of being capitalized.
I admit I was lazy. I was coding from the edge of my seat and didn't want to write twice as many methods just then. I also didn't want to decide on "get_" and "the_", so I knew that leaving it in a known bad state would guarantee review. :-)
#6
@
15 years ago
- Milestone changed from Unassigned to Future Release
I suggest to take this out of trunk because this looks quite experimental. Can benefit from specs and unit-tests as well. I like the idea to have models in, but I think it's not wise to introduce them in a fly-by.
#7
follow-up:
↓ 10
@
15 years ago
After a discussion with andy and hakre on IRC, I've reworked some of the ideas into my patch WP_Object_Factory.12267.diff
WP_Object_Factory has a static method, "create", which creates and returns WP_Objects.
WP_Object acts like an abstract class (not in reality abstract because we have to follow PHP 4 syntax), currently has just a few properties, some getters for those properties, and a property setter. WP_Post and WP_Comment extend WP_Object.
- I went with "WP_Object" instead of "WP_Presentable" after our discussion, in which we seemed to agree that for now it would be better to separate raw property access from presentation. WP_Object is meant to provide an interface for raw property access; something like WP_Presentable in the future would perhaps decorate WP_Object objects, applying the appropriate filters, etc.
- In my opinion it's better form to keep the factory separate from abstract object parent class:
- The structure is immediately obvious and familiar.
- The create method can be static.
- WP_Object child objects can be instantiated independently.
- WP_Object probably needs some more default properties, such as author_id.
- WP_Object is backwards compatible with existing raw database-returned objects, as its particular properties are supposed to be protected (obviously limited by PHP 4 here).
#8
@
15 years ago
- Cc ShaneF added
- Milestone changed from Future Release to 3.0
+2000000000000000 I love this idea. Already commited to trunk. Marking 3.0.
#9
@
15 years ago
- Milestone changed from 3.0 to Future Release
We have discussed that with some depth and what went in in the commit is far away then ready to be feature processed for 3.0.
I think the commit was done in error. If not I suggest that this feature is discussed on this weeks developer meeting if it can make it into 3.0.
filosofo: Please make the factory method plugable so that it's possible to replace the default factory with another one and it's possible to be backwards compatible (meaning use of no factory at all).
#10
in reply to:
↑ 7
;
follow-up:
↓ 13
@
15 years ago
Replying to filosofo:
...
Regarding your last patch I suggest you keep WP_Object as prefix in all classes:
class WP_Object_Page extends WP_Object_Post { class WP_Object_Post extends WP_Object{ class WP_Object_Comment extends WP_Object {
#13
in reply to:
↑ 10
@
15 years ago
Replying to hakre:
Replying to filosofo:
...
Regarding your last patch I suggest you keep WP_Object as prefix in all classes:
class WP_Object_Page extends WP_Object_Post { class WP_Object_Post extends WP_Object{ class WP_Object_Comment extends WP_Object {
hakre, I was thinking the same thing.
Also, I was thinking that perhaps the factory method should return a WP_Object object with null properties, even if the object type isn't identified, to keep from throwing errors downstream. What do you think?
Also, I think the object type mapping should move to each object's constructor.
It was a rough patch, but I wanted to put something up.
#14
@
15 years ago
Revert what's in since we know we don't want that and keep playing? I'm not sure we'll settle this in time for 3.0.
#16
follow-up:
↓ 19
@
15 years ago
Since the layout for posts and comments, for example, can be quite different, how much do we really save by abstracting some template functions? Would we still be branching on type regardless?
Anyhow, reverted the earlier change while we discuss it more.
#17
follow-up:
↓ 20
@
15 years ago
The part of the class name that follows the object type should not be capitalized. WP_Object_Post should be WP_Object_post because "post" is actually data. The simplicity of taking data from post_type or comment_type for the class name without transforming to leading caps trumps the style guide. IMHO, anyway.
Also I think we might give wpdb::query() a new return type, WP_OBJECT, which would put all rows through the factory.
#18
follow-up:
↓ 21
@
15 years ago
I'd like to suggest changing the way the properties are set and accessed so that all the original data in the db_object is still accessible. Instead of assigning all the properties to WP_Object during construction, can't the WP_Object just have a *private* $db_object property that is then accessed by _get methods, ei:
Class WP_Object { /*private*/ $db_ojbect; function get_property($property_name) { if(isset($this->db_object->$property_name)) return $this->db_object->$property_name; return null; } function get_title() { return $this->get_property('title'); } } class WP_Post extends WP_Object { function get_title() { return $this->get_property('post_title'); } }
This also allows a filter to eventually be added to get_property().
I also don't know if WP_Page should be a separate class. Especially with the addition of custom post_types. Its data is so similar to posts and different enough from other presentable data, that being able see that is of type WP_Post and being able to get the post_type from it should be enough.
#19
in reply to:
↑ 16
@
15 years ago
Replying to ryan:
Since the layout for posts and comments, for example, can be quite different, how much do we really save by abstracting some template functions? Would we still be branching on type regardless?
Comments aren't very useful, but being able to merge custom post types, items from other applications on the domain (such as forums, e-commerce, calendar apps, etc.) into the archives and search results would be handy, and are all things I've done. But so far I've had to change the Loop to do so.
#20
in reply to:
↑ 17
@
15 years ago
Replying to andy:
The part of the class name that follows the object type should not be capitalized. WP_Object_Post should be WP_Object_post because "post" is actually data. The simplicity of taking data from post_type or comment_type for the class name without transforming to leading caps trumps the style guide. IMHO, anyway.
PHP class names are case-insensitive, so we can check that "WP_Object_" . "post" is a defined class when it's actually named "WP_Object_Post." The data function still works but it looks more consistent with WP practice.
Also I think we might give wpdb::query() a new return type, WP_OBJECT, which would put all rows through the factory.
#21
in reply to:
↑ 18
@
15 years ago
Replying to prettyboymp:
This also allows a filter to eventually be added to get_property().
Why would we need to have a separate, private property to have a filter?
I also don't know if WP_Page should be a separate class. Especially with the addition of custom post_types. Its data is so similar to posts and different enough from other presentable data, that being able see that is of type WP_Post and being able to get the post_type from it should be enough.
Well for one thing it's consistent: in each case the name of the class reflects the kind of object it is.
More importantly, either this interface or a decorator will do things like set the permalink, which is determined differently between pages and posts.
#23
follow-up:
↓ 25
@
15 years ago
For the sake of understanding it would be nice if there was an example of how these objects would be used. Is it that we would alter functions like the_title() to use these new objects, or are we positing a new kind of theme that handles objects directly using their methods? i.e.
<?php $post->title()?>
#25
in reply to:
↑ 23
@
15 years ago
Replying to jeremyclarke:
For the sake of understanding it would be nice if there was an example of how these objects would be used.
One place in core it could probably be used right away is the loop of RSS builders.
#26
@
15 years ago
- Keywords early added
- Milestone changed from 3.0 to 3.1
It looks like the consensus here is that it won't happen for 3.0 (already reverted). Marking for early 3.1.
#27
@
14 years ago
- Keywords early removed
- Milestone changed from Awaiting Triage to Future Release
This would be very nice for 3.2 and PHP5 :-)
#28
@
14 years ago
Another benefit of such classes: we could use magic methods like get to evolve these data structures while maintaining back compat.
I mean, comment_post_ID will never go away until we do this. Think about that. :)
#32
@
14 years ago
Also worth noting that since PHP 5.0.0 mysql_fetch_object can instantiate an object of any class.
#34
@
14 years ago
With a real WP_Post object, we can lazy load the ancestors property with a magic get, and suppress the tree-searching queries until they're needed. #16574
#35
follow-up:
↓ 37
@
14 years ago
- Cc mikeschinkel@… added
Can I propose WP_Post_Object
and WP_Page_Object
instead of WP_Object_Post
and WP_Object_Page
? I know non-English speakers whose languages would favor the latter will disagree but AFAIK there's no place else in WordPress where classes are named in that way and I think it might be confusing for less skilled programmers. FWIW.
Also $post->labels->singular_name
works nicely for surfacing Post
vs. post
, etc.
#36
follow-up:
↓ 38
@
14 years ago
There wouldn't be a WP_Page_Object, since then it would only be fair to have a different class for each post type, which isn't obvious how to do or even necessary.
#37
in reply to:
↑ 35
@
14 years ago
Just wanted to add my two-cents on this cuz it's been a pet-peeve of mine ever since the introduction of cpt's + taxonomies (since 3.0). I've been wishing the WP_Object were more flexible, abstracted, or just simply not so confined by the predefined structure of posts/pages/comments/tags/categories....
And it I think it would make more sense to me to do $post->title, $post->oontent if instead of the_title() etc. (I too $like->thisway(), $post->query_var->slug better). since there is already $post->ID, I know there's the_ID(), but it seems silly to have procedural functions to do such things that would be easier to do as an interface. More widely functioning classes, that serve a broader spectrum of things (posts, pages, comments, custom post types, taxonomies, hell even custom comment types, page types, or areas for widget types with stereo types...) The future is not predefined why try to do so in the present. The more control and personalization the better since people will always try to make sites in new, unexplored ways that haven't been done yet.
I also agree with what mikeschinkel pointed out. I get that there's WP_Object, but seems idk, ocd maybe? to keep 'WP_Object' and just add _Post or _Page to the end. Wouldn't be the first time words in a function or class name were reorganized.
I'd love it if this were implemented into 3.2, or 3.1.1 even? </ramble>
#38
in reply to:
↑ 36
@
14 years ago
Replying to scribu:
There wouldn't be a WP_Page_Object, since then it would only be fair to have a different class for each post type, which isn't obvious how to do or even necessary.
That's where I believe that a more abstracted class would be useful, posts,pages,custom types should, I think, use one central class, and depending on types, hierarchical or not, etc. w/e, have a child class for doing that stuff, and then that way, you can create any page or post type by creating new objects off of that sort of class hierarchy. Having "post" and "page" is much too generic to be predefined as builtin types if you ask me. since no one knows really what to call other "post types" or content types, post types is the most used, which contains the word 'post' in what everyone calls cpt's, which is a built in 'type' for 'posts'.
I think posts could even be hierarchically broken down somehow, since posts is more or less, blog posts or site articles of w/e. and now with cpts posts could be anything, so it sort of adds a hierarchy to the type itself upon adding a cpt to your site.
#39
follow-ups:
↓ 40
↓ 41
↓ 42
@
14 years ago
I was going to say it's going to be hard for multiple plugins to extend these classes.
But then I figured we can use the __call()
magic method, since we're on PHP5 now:
function __call( $method, $args ) { array_unshift( $args, $this ); return do_action_ref_array( "post_{$method}", $args ); }
Hooks save the day again. :)
#40
in reply to:
↑ 39
@
14 years ago
Replying to scribu:
There wouldn't be a WP_Page_Object, since then it would only be fair to have a different class for each post type, which isn't obvious how to do or even necessary.
Then I am confused by the examples I've seen thus far on this ticket.
Replying to scribu:
But then I figured we can use the
__call()
magic method, since we're on PHP5 now:
...
Hooks save the day again. :)
Nice. That technique might be useful elsewhere too.
#41
in reply to:
↑ 39
@
14 years ago
Replying to scribu:
I was going to say it's going to be hard for multiple plugins to extend these classes.
But then I figured we can use the
__call()
magic method, since we're on PHP5 now:
function __call( $method, $args ) { array_unshift( $args, $this ); return do_action_ref_array( "post_{$method}", $args ); }
We don't need that. The descendant constructors can just assign the respective properties their object-specific values.
For example, the ancestor class defines the get_title
method:
function get_title() { return $this->_title; }
So for post objects upon instantiation:
$this->_title = $this->post_title;
and for terms:
$this->_title = $this->name;
#42
in reply to:
↑ 39
;
follow-up:
↓ 43
@
14 years ago
Replying to scribu:
function __call( $method, $args ) { array_unshift( $args, $this ); return do_action_ref_array( "post_{$method}", $args ); }
Also, I think this in addition to making things devilishly hard to debug, misses one of the big benefits of this proposal, which is having the same set of standard properties and methods across different types of objects.
For example, I want to be able to be able to return all sorts of objects after a search. Then I should be able to iterate over all of them with links and summaries, etc., agnostic about the particular kind of object returned. That way categories, posts, pages, custom post types, etc., can all be part of the Loop and use the same link, headline, and paragraph markup.
#43
in reply to:
↑ 42
;
follow-ups:
↓ 44
↓ 45
@
14 years ago
Replying to filosofo:
Also, I think this in addition to making things devilishly hard to debug, misses one of the big benefits of this proposal, which is having the same set of standard properties and methods across different types of objects.
For example, I want to be able to be able to return all sorts of objects after a search. Then I should be able to iterate over all of them with links and summaries, etc., agnostic about the particular kind of object returned. That way categories, posts, pages, custom post types, etc., can all be part of the Loop and use the same link, headline, and paragraph markup.
I suggest a standard interface any type can implement and therefore be compatible.
As post types (content data) have a lot of dynamic properties I suggest some kind of ''abstract'' base class as well and an abstract visitor base class. That should offer a lot of flexibility in conjunction with the interface.
IMHO dynamic properties are very important because basically you don't need many classes but normaly only one class with dynamic properties as all the data-types are basically the same. More or less simple data objects.
I see this as a slow evolution: from the arrays and later on standard classes we've been using in core for posts and pages as prototype models so far for the last 7 years.
The magic methods and array access as well as iterators from the spl can help to keep things fluid. Defer the details, just make use of standard PHP, there's already a lot done.
#44
in reply to:
↑ 43
@
14 years ago
Replying to hakre:
As post types (content data) have a lot of dynamic properties I suggest some kind of ''abstract'' base class as well and an abstract visitor base class. That should offer a lot of flexibility in conjunction with the interface.
If you look at my patch from a year ago WP_Object
is an abstract class, in this case with "abstract" in comments because at that point we couldn't have gone PHP 5.
#45
in reply to:
↑ 43
@
14 years ago
Replying to hakre:
I suggest a standard interface any type can implement and therefore be compatible....As post types (content data) have a lot of dynamic properties I suggest some kind of ''abstract'' base class as well and an abstract visitor base class. That should offer a lot of flexibility in conjunction with the interface.
+1
#53
@
12 years ago
- Summary changed from Upgrade loop objects to provide identical presentational interfaces. to Introduce WP_Post class
Going to limit the scope of this ticket to WP_Post only. WP_Comment can come later.
One thing that this would allow is to cache only the raw post data, avoiding all the tip-toeing around $post->filter:
get_permalink():
if ( is_object($id) && isset($id->filter) && 'sample' == $id->filter ) { $post = $id; $sample = true; } else { $post = &get_post($id);
get_post():
} elseif ( is_object($post) && empty($post->filter) ) { _get_post_ancestors($post); $_post = sanitize_post($post, 'raw'); wp_cache_add($post->ID, $_post, 'posts'); } elseif ( is_object($post) && 'raw' == $post->filter ) { $_post = $post; } else {
#55
@
12 years ago
In the WP 3.5 pre-planning meeting today I said I'd take a look at this. I've reviewed the patches and 2 years(!) worth of comments, and I'm struck by the general desire to approach this with One Base Class To Rule Them All (comments, posts, categories, etc). The idea of being able to call a common set of methods for almost anything - to get authors, titles, etc - is definitely seductive, so I understand the appeal. But my concern is that the conceptual foundation of the base class is weak (i.e. comments, posts, categories, etc to my mind are distinctly different things that happen to share some attributes). I think this approach will lead to violations of the Liskov substitution principle. That is, we'd ultimately end up with child classes that have a bunch of degenerate (unimplemented) methods and/or overriddes, and generally buggy/confusing behavior as the child classes evolve in different directions from each other.
When WP is ready for PHP 5.4 I could see using traits to encapsulate the common characteristics among posts, comments, etc. But aside from that, my feeling at this point is to pursue WP_Post as a class designed for use with posts and pages, and not try to tie it to a broadly defined base class (and if we later see that there is a justification for a base class, we can always extract it without breaking external behavior).
I'd like to hear feedback. I have strong OO experience but I have not done a lot with WP core yet, so there may be aspects to this I'm missing.
#57
@
12 years ago
Sounds good. I figured I should explain how I'm thinking about it, as my approach won't lend itself to the kind of generalized extensibility that's been a big part of this discussion (and to make sure I'm not missing something important in my thinking before I sink time into it).
#58
@
12 years ago
The patch I just added is a prototype, and is essentially a simpler version of previous proposed patches. I made the properties public since there is already existing code that directly manipulates their values (but the magic set at least checks that arbitrary properties aren't being added).
The main interest in this discussion so far is to focus on methods related to display, so I have get_the_title() as an example. My thought is that, in the long run, WP_Post would become the home for this functionality (not just a wrapper for existing functions), but I want to see what people think of what I have as a starting point.
#59
@
12 years ago
WP_Post should contain the functionality of get_post_title() now, not in the long run. Otherwise, there's little benefit.
And since you have all the fields explicitly defined as public, __set()
will never succeed. But more than that, we have to allow arbitrary attributes, for back-compat reasons, at least.
#60
@
12 years ago
Got it - let me know if the patch I just added looks like the right approach.
Then the question is - how far to go? We could potentially fold in a lot of what's in post.php and post-template.php. Focus on the display-oriented functions, since that's where most of the interest seems to be, or go for all the post-related functionality?
In regard to the magic set, I think you misunderstood my point. It does disallow adding arbitrary properties (e.g. it won't let you do $post->foo = 'bar'). It doesn't stop you directly changing values to the listed properties. If we do need to allow adding arbitrary properties, then I'll just remove the magic set (and in that case it may not even be worth listing the properties).
#61
@
12 years ago
What I'm saying is that if ( property_exists('WP_Post', $name) )
will never be true, since WP_Post only has public properties and __set()
is not called for public properties.
As for get_the_title()
, I think we can shorten it to WP_Post->get_title()
. Let's stick to porting template tags for now.
#63
@
12 years ago
Thanks scribu - I've corrected the magic set (the "if" block was leftover from when I originally tried making the properties private).
This will be a work in progress for a bit, so instead of posting a series of patches here, anyone interested can follow or contribute on GitHub: https://github.com/toppa/WordPress/compare/adding-wp-post-class
#65
@
12 years ago
I personally am not at all interested in putting a bunch of template functions in WP_Post, especially for 3.5. 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.
#66
@
12 years ago
It seems I misunderstood the plan for WP_Post. I agree that it's a good idea to not try to shove all the template functions inside it.
Sorry for hijacking this ticket.
#67
@
12 years ago
- Summary changed from Introduce WP_Post class to Upgrade loop objects to provide identical presentational interfaces
See #21309
#69
@
11 years ago
Are there any updates related to this ticket? I've been working on a set of common classes that accomplishes much of whats been voiced here but wanted to check on the current status/thinking.
#70
@
9 years ago
- Milestone Future Release deleted
- Resolution set to maybelater
- Status changed from new to closed
Closing as maybelater. Complete lack of interest on the feature on the ticket over the last 3 years. Feel free to reopen when more interest re-emerges (particularly if there's a patch).
#74
@
9 years ago
- Resolution maybelater deleted
- Status changed from closed to reopened
New patch as example. I think that this is good way to OOP in WP :)
#76
@
9 years ago
- Keywords has-patch added
@sebastian.pisula looking at 36235.patch, seems like there's inconsistency in method naming. I think we should err on the side of always excluding _post_
in method names because it's mostly moot in the scope of a post object class.
This also promotes consistency in later decorating other objects with similar methods.
#77
@
9 years ago
@DrewAPicture this is example. I think that final version will be with correct names. I need feedback from other developers. This will small step for OOP for WordPress :)
#78
@
9 years ago
Yeah, the naming should probably be as generic as possible. But that's a detail.
Besides that, I'd probably remove has_excerpt()
and has_post_thumbnail()
methods. The respective functions can just do something like empty( $post->get_excerpt() )
.
https://github.com/humanmade/WordPress-Objects/blob/master/wordpress-objects.post.php could serve as an inspiration for what is possible. I particularly like simple methods like get_title ()
, get_parent()
, get_status()
, and the *_meta()
ones.
The usefulness of such an enhancement has already been discussed a couple of times and we definitely do not need to move everything into WP_Post
, only where it makes sense.
#79
@
2 years ago
- Resolution set to maybelater
- Status changed from reopened to closed
Going to close this one out again as a maybelater
. as it has been 6+ years without any interest shown in the feature.
Discussion can always continue on a closed ticket, and it can be reopened in the future if someone wants to take the lead.
First draft. This works.