Make WordPress Core

Opened 8 years ago

Last modified 5 years ago

#36492 reviewing enhancement

Add `WP_Post_Status` class

Reported by: flixos90's profile flixos90 Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.0
Component: Posts, Post Types Keywords: has-patch dev-feedback
Focuses: Cc:

Description

Similar to #36217 and #36224, a dedicated class for post statuses would make it easier to work with those as they are currently represented through raw objects.

The global variable $wp_post_statuses would then hold objects of WP_Post_Status which would provide the benefits of autocompletion in IDEs, and we could possibly add methods to the class in the future to make post status management easier.

Attachments (5)

36492.diff (3.4 KB) - added by flixos90 8 years ago.
initial patch implementing WP_Post_Status
36492.2.diff (8.9 KB) - added by flixos90 8 years ago.
fix for last patch
36492.3.diff (12.3 KB) - added by DrewAPicture 8 years ago.
Docs fixes + coding standards
36492.4.diff (14.7 KB) - added by flixos90 8 years ago.
WP_Status as base class for WP_Post_Status
36492.5.diff (13.6 KB) - added by flixos90 8 years ago.
update from 36492.3.diff (without abstract class)

Download all attachments as: .zip

Change History (23)

@flixos90
8 years ago

initial patch implementing WP_Post_Status

#1 @flixos90
8 years ago

  • Keywords has-patch dev-feedback added

36492.diff is an initial patch implementing the class and using it in register_post_status() and get_post_status_object().

Update: This patch is broken, please use 36492.2.diff.

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

@flixos90
8 years ago

fix for last patch

#2 @flixos90
8 years ago

I forgot to do svn add in the first patch, so please ignore it :)

36492.2.diff is an actually valid patch.

#3 @sebastian.pisula
8 years ago

I like this !

#4 @ocean90
8 years ago

A WP_Post_Status class was already proposed 3 years ago in #12706, see ticket:12706:135.

@DrewAPicture Any chance you could review this patch?

#5 @DrewAPicture
8 years ago

  • Owner set to DrewAPicture
  • Status changed from new to reviewing

@DrewAPicture
8 years ago

Docs fixes + coding standards

#6 follow-up: @DrewAPicture
8 years ago

@flixos90 I've iterated on 36492.2.diff with 36492.3.diff. Changes mostly comprise coding standards and docs fixes. Some notes not implemented in the new patch:

My primary concern with the current approach is that we're doing argument handling in the constructor.

Outside of the Customizer APIs and list table classes, we don't usually do much argument handling in constructors, as they're typically relegated to populating properties in core classes.

With that in mind, I think we'd do well to implement a base/sub-class pattern with something like WP_Status – as suggested by @johnjamesjacoby in #12706 – that could be then extended for the posts, or users, or comments, or * use cases.

Implementing argument handling in the constructor could throw up some roadblocks when it comes to implementing a base class.

All that said, I wouldn't really have a problem with committing a WP_Post_Status first-run, though I would package it with a WP_Status base class in the same release. There's enough cross-over and possibility for standardized methods of handling underlying status architecture elsewhere in core to give the base/sub-class approach due consideration. Post statuses is an excellent place to start.

#7 in reply to: ↑ 6 ; follow-up: @flixos90
8 years ago

Replying to DrewAPicture:

My primary concern with the current approach is that we're doing argument handling in the constructor.

Outside of the Customizer APIs and list table classes, we don't usually do much argument handling in constructors, as they're typically relegated to populating properties in core classes.

The thing is the arguments are only validated and filled with defaults to populate the properties afterwards. I agree in so far that the constructor should not do anything external with the arguments. But we can think about alternatives, for example we could put all the arguments into an $args property in the class and then move the logic from the constructor into a validate() method. We could then use magic getters to access these values as properties. I definitely think the validating logic should stay in the class and not in the register_post_status() function. Anyway, we should probably discuss this in a broader scope since there are also a WP_Post_Type and WP_Taxonomy class in the making which use a similar approach (see #36217 and #36224).

With that in mind, I think we'd do well to implement a base/sub-class pattern with something like WP_Status – as suggested by @johnjamesjacoby in #12706 – that could be then extended for the posts, or users, or comments, or * use cases.

That definitely makes sense. Just for an overview, can you outline to me which parts of WordPress have statuses? Not sure atm if there are others beside posts and comments. I would also like to broaden this discussion for the above two tickets - a post type and a taxonomy (and a comment type) are basically "content types", similar to how a post status and a comment status are both statuses.

#8 in reply to: ↑ 7 @swissspidy
8 years ago

Replying to flixos90:

Anyway, we should probably discuss this in a broader scope since there are also a WP_Post_Type and WP_Taxonomy class in the making which use a similar approach (see #36217 and #36224).

Sure! Drew mentioned pointed them out to me as well. I have yet to review the most recent patches on those tickets though.

I like the idea of handling arguments in a get_instance() method or similar. I'm not 100% fond of the current approaches anyway.

This shouldn't stop us from introducing WP_Status in a revised patch. Such base class will surely help with handling the arguments in a better way, because it makes us think more about it :-)

That definitely makes sense. Just for an overview, can you outline to me which parts of WordPress have statuses? Not sure atm if there are others beside posts and comments. I would also like to broaden this discussion for the above two tickets - a post type and a taxonomy (and a comment type) are basically "content types", similar to how a post status and a comment status are both statuses.

Posts, comments and users all currently have some kind of statuses. See also #36594 and #34316.

#9 @flixos90
8 years ago

I will work on a new patch in the next days. Should I attach that patch to this ticket? Or should it be in a new ticket for a WP_Status base class? I'm not sure how we should handle something like this on Trac since an abstract WP_Status really doesn't belong to one component.

#10 @swissspidy
8 years ago

@flixos90 It's fine to do this in this ticket.

@flixos90
8 years ago

WP_Status as base class for WP_Post_Status

#11 @flixos90
8 years ago

36492.4.diff introduces an abstract WP_Status class, and the WP_Post_Status class extends it. I put all properties which should also apply to a possible WP_Comment_Status and WP_User_Status into the base class. So for now, all that the WP_Post_Status class still takes care of are the exclude_from_search and publicly_queryable properties since I feel they are post-specific properties (correct me if I'm wrong here).

For the overall approach, I modified the class to follow the structure proposed by @boonebgorges in the WP_Post_Type discussion.

@DrewAPicture: I wasn't completely sure what would be the best way to document the two classes, especially in terms of duplicate docs or unclear docs because of the abstraction. These definitely need some review.

This ticket was mentioned in Slack in #core by flixos90. View the logs.


8 years ago

@flixos90
8 years ago

update from 36492.3.diff (without abstract class)

#13 follow-up: @flixos90
8 years ago

After discussion in weekly Core meeting, it was decided not to use abstract classes at this point.

Therefore 36492.5.diff is an iteration built upon 36492.3.diff which however includes the internal structural improvements from 36492.4.diff.

#14 in reply to: ↑ 13 @johnjamesjacoby
8 years ago

Replying to flixos90:

After discussion in weekly Core meeting, it was decided not to use abstract classes at this point.

That's too bad. It's not unlike the base list-table class, where we have a specific problem that requires solving, and foreseeable problems that we can also architect for and begin solving at the same time.

In WordPress core, users, comments, and taxonomy terms will (all, eventually) derive a great deal of value from a structure around what a status includes and details. Without a base class, so far, each of these objects has redefined what a status is in a unique (and mostly terrible) way. BuddyPress and bbPress have followed suit, unfortunately.

In plugins, any other imaginable object will likely need a status. Events, Downloads, Shopping cart items, Activity streams, Friendship requests, Group memberships, etc...

The current WP_Post_Status class is a great start, and hopefully we'll be able to use it as a jumping off point for refactoring statuses for the other WordPress objects at a later date.

#15 @boonebgorges
8 years ago

I think we should hold off on discussion of a base class (or, better, some technique for extensibility that doesn't rely on inheritance) until we have clear concept of what a "status" is, in general: what are the properties that make something a status (rather than a post, or a term, etc), and what abilities a status should have. As things stand, *post* statuses are not even very well conceptualized, much less a general "status" type that could inform other implementations.

36492.5.diff seems OK to me. A couple thoughts and questions:

  • I don't think we need to describe default values for object properties in the documentation for those properties. We already do that in set_props().
  • What is the benefit of the get_instance() method? Methods like this are generally used when the constructor is private - ie for a singleton. It seems like a mixing of concerns to have the object make reference to the global $wp_post_statuses. I'd remove the method and move that logic back to get_post_status_object().
  • Code formatting: ! $args['internal'], not $args['internal'], etc

#16 @schlessera
8 years ago

Was there any consideration for implementing post statuses as an actual "enumerated" (or "enum") data type (or at least a PHP simulation thereof)?

Benefits would be:

  • Correct data type, as it is the correct representation of a finite set of values.
  • Proper, semantic equality checking.
  • Proper, semantic way of getting all accepted values.
  • Type-hinting on the actual status value if wanted.

A useful implementation example would be https://github.com/marc-mabe/php-enum, which could probably be rewritten to work without namespaces.

#17 @DrewAPicture
8 years ago

  • Owner DrewAPicture deleted

#18 @GaryJ
8 years ago

Was there any consideration for implementing post statuses as an actual "enumerated" (or "enum") data type (or at least a PHP simulation thereof)?

An enumerated list wouldn't allow much in the way of custom statuses.

and what abilities a status should have

This is crying out for an interface to be created first, so that each WP_*_Status, including WP_Post_Status can implement it.

Note: See TracTickets for help on using tickets.