Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#36217 closed enhancement (fixed)

WP_Post_Type class

Reported by: swissspidy's profile swissspidy Owned by: swissspidy's profile 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)

36217.diff (23.2 KB) - added by swissspidy 8 years ago.
36217.2.diff (28.2 KB) - added by swissspidy 8 years ago.
Moar docs
36217.3.diff (33.4 KB) - added by flixos90 8 years ago.
Separated object logic from additional logic (methods to register and unregister)
36217.4.diff (34.7 KB) - added by swissspidy 8 years ago.
36217.5.diff (34.8 KB) - added by swissspidy 8 years ago.
36217.6.diff (39.2 KB) - added by swissspidy 8 years ago.
36217.7.diff (38.3 KB) - added by ocean90 8 years ago.
36217.8.diff (40.9 KB) - added by ocean90 8 years ago.
Doc updates

Download all attachments as: .zip

Change History (45)

@swissspidy
8 years ago

#1 @swissspidy
8 years ago

  • Keywords has-patch added; needs-patch removed

36217.diff is a first pass as a proof-of-concept, just so one can see where this could go.

@swissspidy
8 years ago

Moar docs

#2 @Funkatronic
8 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 @boonebgorges
8 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.

#4 @swissspidy
8 years ago

  • Type changed from defect (bug) to enhancement

#5 @ramiy
8 years ago

Not a bad idea.

#6 @DrewAPicture
8 years ago

  • Keywords 4.6-early added
  • Milestone changed from Awaiting Review to Future Release

#7 follow-up: @DrewAPicture
8 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 @swissspidy
8 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

#9 @swissspidy
8 years ago

Related: #36224 (WP_Taxonomy class)

#10 @ocean90
8 years ago

#36478 was marked as a duplicate.

#11 @flixos90
8 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.

@flixos90
8 years ago

Separated object logic from additional logic (methods to register and unregister)

#12 @flixos90
8 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.

Last edited 8 years ago by flixos90 (previous) (diff)

#13 @ocean90
8 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: @boonebgorges
8 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 @swissspidy
8 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 @Frank Klein
8 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 @flixos90
8 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 @flixos90
8 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.


8 years ago

@swissspidy
8 years ago

#20 @swissspidy
8 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 @boonebgorges
8 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.

@swissspidy
8 years ago

#22 @swissspidy
8 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: @schlessera
8 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.


8 years ago

#25 @lukecavanagh
8 years ago

@schlessera like the suggestion of changing to content from post.

Last edited 8 years ago by lukecavanagh (previous) (diff)

#26 @chriscct7
8 years ago

  • Owner set to swissspidy
  • Status changed from new to assigned

#27 in reply to: ↑ 23 @flixos90
8 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 @schlessera
8 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 @swissspidy
8 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 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
8 years ago

#30 @swissspidy
8 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

@ocean90
8 years ago

#31 @ocean90
8 years ago

36217.7.diff is a refresh of 36217.6.diff without the change to nav-menu.php, see #37211.

@ocean90
8 years ago

Doc updates

#32 @ocean90
8 years ago

  • Keywords dev-feedback removed
  • Resolution set to fixed
  • Status changed from assigned to closed

Fixed in [37890].

#33 @swissspidy
8 years ago

Looks like the docblock for the $labels property is wrong. get_post_type_labels() returns an object, not an array.

#34 @DrewAPicture
8 years ago

In 38030:

Docs: The $labels property in WP_Post_Type is of type object as returned from get_post_type_labels(), not an array.

Props swissspidy.
See #36217.

#35 @DrewAPicture
8 years ago

In 38051:

Docs: Add and clarify changelog entries for elements that can now accept, use, or return WP_Post_Type objects.

Also adds a missing initial @since version for wp_xmlrpc_server::_prepare_post_type().

See [37890]. See #36217.

#36 @westonruter
8 years ago

In 38097:

Docs: Correct type of WP_Post_Type::$cap from array to object.

Fixes typo introduced in r37890. The WP_Post_Type::$cap property is set to the return value of get_post_type_capabilities() which is an object.

See #36217.

#37 @swissspidy
7 years ago

In 38747:

Taxonomy: Introduce WP_Taxonomy and use it in register_taxonomy() and unregister_taxonomy().

This changes the global $wp_taxonomies to an array of WP_Taxonomy objects. WP_Taxonomy includes methods to handle rewrite rules and hooks.
Each taxonomy argument becomes a property of WP_Taxonomy. Introducing such a class makes further improvements in the future much more feasible.

Props boonebgorges for review.
Fixes #36224. See #36217.

Note: See TracTickets for help on using tickets.