#36217 closed enhancement (fixed)
WP_Post_Type class
Reported by: | swissspidy | Owned by: | swissspidy |
---|---|---|---|
Milestone: | 4.6 | Priority: | normal |
Severity: | normal | Version: | 2.9 |
Component: | Posts, Post Types | Keywords: | has-patch has-unit-tests |
Focuses: | Cc: |
Description
I've been dealing with custom post types a lot lately and kept dealing with unhelpful post type objects (basically the post type args converted to an object).
For example, get_post_type_object()
does not provide any information about the available properties. register_post_type()
does in some part, but the actual post type properties differ from the $args
argument.
A WP_Post_Type
class would make this easier by helping with documentation and preventing accidental errors.
The global $wp_post_types
could be an array of WP_Post_Type
objects. The class properties wouldn't change for backward compatibility and any class methods would only add some benefit to it.
This ticket should:
- Introduce the
WP_Post_Type
class and have base properties and methods for interacting with a post type object. - Apply the new class where it can be used within core.
- Provide tests for any methods introduced.
See also #32450 and #31985 for WP_Site and WP_Network classes.
Attachments (8)
Change History (45)
#2
@
9 years ago
If there wasn't already a global variable for post types, I'd put them in a static variable in the WP_Post_Type class. Other than that, simply brilliant
#3
@
9 years ago
+1. There's a lot of improvement that we could make to the generation of default values and other fun stuff once this is in place.
#7
follow-up:
↓ 8
@
9 years ago
Food for thought: Is there value in not locking ourselves into "post type" vernacular and instead going with WP_Content_Type
? There have been some mutterings over the years about the missed opportunity in calling everything "posts".
#8
in reply to:
↑ 7
@
9 years ago
Food for thought: Is there value in not locking ourselves into "post type" vernacular and instead going with
WP_Content_Type
? There have been some mutterings over the years about the missed opportunity in calling everything "posts".
+1 to that. Definitely to be considered.
In general, what we get from such a class:
- Improved documentation on DevHub, moving/copying lots of information out of register_taxonomy() into a separate page.
- Better type hinting thanks to PHPDoc changes.
- Forward compatibility (think magic getter/setter for deprecated arguments, improving the way we handle default values, etc.)
- Better error prevention
- Makes adding a REST API endpoint for getting post types easier
#11
@
9 years ago
Maybe we should consider to handle everything that doesn't modify the post type object itself outside of the class and leave it in register_post_type()
. Right now, some steps (like adding rewrite rules) happen in the WP_Post_Type
constructor while others (_future_post_hook
, registering taxonomies) remain in register_post_type
.
This would mean that the WP_Post_Type
constructor only handles all its properties, while additional steps (like adding rewrite rules, query var etc would remain in register_post_type()
(the WP_Post_Type
class then wouldn't need to access $wp
and $wp_rewrite
for example). I actually like this approach a little more as WP_Post_Type
would then only take care of itself and represent a post type, so if someone manually created an instance of the class, it wouldn't add any rewrite rules or cause other inconsistencies. And we also could possibly allow to insert this object directly into register_post_type()
as a parameter at a later stage.
#12
@
9 years ago
36217.3.diff separates the post type object logic from the additional steps that are performed when registering the post type. There are two new methods _register()
and _unregister()
for that which are called from register_post_type()
and unregister_post_type()
(contrary to my above post I thought it was better to put these steps into the class as well, just separated). Maybe these should set some kind of flag so that they are not executed more than once.
#13
@
9 years ago
- Keywords 4.6-early removed
- Milestone changed from Future Release to 4.6
@DrewAPicture @boonebgorges How do you feel about this?
#14
follow-up:
↓ 17
@
9 years ago
Some general thoughts on the approach here and in #36224.
@flixos90's 36217.3.diff suggests that some aspects of post type registration shouldn't happen in the constructor method. IMO we should take this point further, and do very little of anything in the constructor. Off the top of my head, a sensible pattern might look like this:
class WP_Post_Type { public function __construct( $post_type = '', $args = array() ) { if ( $post_type ) { $this->set( 'post_type', $post_type ); } if ( $args ) { $this->set_props( $args ); } } public function set( $prop, $value ) { ... } public function set_props( $args ) { // parse with defaults, calculate fallback values, then foreach ( $args as $key => $value ) { $this->set( $key, $value ); } } }
So $foo = new WP_Post_Type( 'foo', $args );
is just shorthand for $foo = new WP_Post_Type( 'foo' ); $foo->set_props( $args );
This structure makes it easier to modify a post type/taxonomy after it's been generated, and also makes the construction of a bare-bones post type object much more lightweight.
Stuff that is not related to setting fallback props should be handled in standalone methods. Eg public function generate_rewrite_rules()
.
Then, in place of the monster logic that's currently in register_post_type()
(or __construct()
in 36217.3.diff) we can have a convenience method like start()
:
public function start() { $this->set_supports(); $this->generate_rewrite_rules(); $this->register_meta_boxes(); $this->init_hooks(); $this->register_taxonomies(); }
or whatever we want the method names to be. And register_post_type()
becomes:
$post_type_object = new WP_Post_Type( $post_type, $args ); $post_type->start();
which is similar to 36217.3.diff but without the underscore-prefix method names :)
This kind of structure will be infinitely more testable, more configurable by developers, easier to read, etc. What do others think?
#15
@
9 years ago
Really like that proposed structure, @boonebgorges!
Will have a look at this in the coming days to come up with a working patch.
#16
@
9 years ago
Great work so far. A small detail, not limited to this class, is the use of underscores before public method names.
An example would be _register()
. I've seen this naming convention used in older code, to mark private methods, but this method isn't private.
#17
in reply to:
↑ 14
@
9 years ago
@boonebgorges: This sounds like a great structure for the class, splitting the processes up into separate methods.
@swissspidy: Looking forward to your next iteration of the patch!
#18
@
9 years ago
What we should also consider is creating an abstract base class for both WP_Post_Type
and WP_Taxonomy
(see #36224), something like WP_Content_Type
. This class could contain common abstract methods and would ensure that we develop these classes in the same structure. Post types and taxonomies are both types of certain content. This base class could possibly serve for other use-cases (WP_Comment_Type
for example) in the future.
This ticket was mentioned in Slack in #core by ocean90. View the logs.
9 years ago
#20
@
9 years ago
- Keywords needs-unit-tests added
In 36217.4.diff I re-iterated on the functionality based on the previous comments.
I split everything up into little helper methods to make the code more readable.
Instead of a start()
method I kept the method calls inside (un)register_post_type()
because some stuff needs to happen before $wp_post_types[ $post_type ] = $post_type_object;
and some after that line.
Regarding the abstract class, I think we shouldn't do that just yet, for the same reasons as in #36492.
#21
@
9 years ago
Organization looks good to me, @swissspidy. One small thing: now that we're not putting a stdClass
into the $wp_post_types
global, it's probably no longer necessary to do the (object) array
juggling when parsing props.
#22
@
9 years ago
Good idea! This was bugging me anyway, so I removed the object casting where not needed in 36217.5.diff.
#23
follow-up:
↓ 27
@
9 years ago
First of all, if we are rethinking the data structures, I would also vote for changing from "Post_<something>" to "Content_<something>", as WordPress wants to be a Content Management System, not only a Blog (= Post Management System).
Also, while we're at it, these classes should try to be more SOLID then the rest of WordPress, so I think we should aim for a better way for dealing with the rewrite rules. As rewriting rules are also used for other data types, why not split this out into its own object?
This ticket was mentioned in Slack in #core by ocean90. View the logs.
9 years ago
#25
@
9 years ago
@schlessera like the suggestion of changing from content from post.
#27
in reply to:
↑ 23
@
9 years ago
Replying to schlessera:
First of all, if we are rethinking the data structures, I would also vote for changing from "Post_<something>" to "Content_<something>", as WordPress wants to be a Content Management System, not only a Blog (= Post Management System).
I don't think we should change the naming conventions here. Although I see your point with the Content Management System, to me, "Content" refers to something more general as I think that both posts and terms are a kind of content. I would rather use a Content_<something>
for possible abstract classes in the future (like WP_Post_Type
and WP_Taxonomy
being derived from a WP_Content_Type
for example). However, as we decided not to go with abstract classes, at least for this release, I'd still like to leave that as a possibility.
Also, the terminology of posts is widely established in the community and a change like that would probably lead to confusion as we cannot change it everywhere in the Core code (inconsistency).
#28
@
9 years ago
as I think that both posts and terms are a kind of content
In my view, terms are "meta"content, something that qualifies the content itself. They don't serve any purpose in isolation, only when attached o real content.
like WP_Post_Type and WP_Taxonomy being derived from a WP_Content_Type for example
Conceptually, a WP_Post_Type
and a WP_Taxonomy
share very little, other than that they both have an ID and a name, and that they are both stored in the same DB. You can attach one to the other (as you can attach a group to a user), but that does not make them descendents of a same parent. That's why most CMS systems opt to define these as leafs
within the overarching CMS tree
.
I would think that calling WP_Taxonomy
a type of Content
will add to the confusion about what taxonomies even are and how they are used.
Also, the terminology of posts is widely established in the community and a change like that would probably lead to confusion as we cannot change it everywhere in the Core code (inconsistency).
Yes, unfortunately, because this leads to the misconception of many people that WordPress is a blogging platform, and not much more.
#29
@
9 years ago
Also, while we're at it, these classes should try to be more SOLID then the rest of WordPress, so I think we should aim for a better way for dealing with the rewrite rules. As rewriting rules are also used for other data types, why not split this out into its own object?
Next generation rewrites are being worked on in #36292.
I would think that calling
WP_Taxonomy
a type ofContent
will add to the confusion about what taxonomies even are and how they are used.
Also, the terminology of posts is widely established in the community and a change like that would probably lead to confusion as we cannot change it everywhere in the Core code (inconsistency).
Correct, that would definitely confusing and inconsistent. Imaging register_post_type()
returning a WP_Content_Type
object. https://markjaquith.wordpress.com/2010/11/12/post-formats-vs-custom-post-types/ is a great read for that. I don't think the class name is up for discussion here.
#31
@
8 years ago
36217.7.diff is a refresh of 36217.6.diff without the change to nav-menu.php, see #37211.
#32
@
8 years ago
- Keywords dev-feedback removed
- Resolution set to fixed
- Status changed from assigned to closed
Fixed in [37890].
36217.diff is a first pass as a proof-of-concept, just so one can see where this could go.