#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)
@swissspidy
10 months ago
#1
@swissspidy
10 months ago
- Keywords has-patch added; needs-patch removed
#2
@Funkatronic
10 months 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
@boonebgorges
10 months 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.
#4
@swissspidy
10 months ago
- Type changed from defect (bug) to enhancement
#6
@DrewAPicture
10 months ago
- Keywords 4.6-early added
- Milestone changed from Awaiting Review to Future Release
#7
follow-up:
↓ 8
@DrewAPicture
10 months 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
@swissspidy
10 months 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
#9
@swissspidy
10 months ago
Related: #36224 (WP_Taxonomy class)
#11
@flixos90
9 months 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.
@flixos90
9 months ago
Separated object logic from additional logic (methods to register and unregister)
#12
@flixos90
9 months 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(). Maybe these should set some kind of flag so that they are not executed more than once.
#13
@ocean90
8 months 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
@boonebgorges
8 months 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
@swissspidy
8 months 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
@Frank Klein
8 months 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
@flixos90
8 months 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
@flixos90
8 months 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.
8 months ago
@swissspidy
8 months ago
#20
@swissspidy
8 months 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
@boonebgorges
8 months 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.
@swissspidy
8 months ago
#22
@swissspidy
8 months 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
@schlessera
8 months 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.
7 months ago
#25
@lukecavanagh
7 months ago
@schlessera like the suggestion of changing to content from post.
#26
@chriscct7
7 months ago
- Owner set to swissspidy
- Status changed from new to assigned
#27
in reply to:
↑ 23
@flixos90
7 months 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
@schlessera
7 months 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
@swissspidy
7 months 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 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).
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.
@swissspidy
7 months ago
#30
@swissspidy
7 months ago
- Keywords has-unit-tests added; needs-unit-tests removed
#31
@ocean90
7 months ago
36217.7.diff is a refresh of 36217.6.diff without the change to nav-menu.php, see #37211.
#32
@ocean90
7 months ago
- Keywords dev-feedback removed
- Resolution set to fixed
- Status changed from assigned to closed
Fixed in [37890].
#33
@swissspidy
6 months ago
Looks like the docblock for the $labels property is wrong. get_post_type_labels() returns an object, not an array.
#34
@DrewAPicture
6 months ago
In 38030:
#35
@DrewAPicture
6 months ago
In 38051:
#36
@westonruter
6 months ago
In 38097:
#37
@swissspidy
3 months ago
In 38747:
36217.diff is a first pass as a proof-of-concept, just so one can see where this could go.