Make WordPress Core

Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#30292 closed enhancement (duplicate)

Stop get_post() overwriting non-WP_Post objects passed as first parameter

Reported by: mikeschinkel's profile MikeSchinkel Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.5
Component: Posts, Post Types Keywords: has-patch dev-feedback 2nd-opinion close
Focuses: Cc:

Description

In his 2014 New York WordCamp @nacin answered a question about WP_Post being declared final and @nacin suggested using composition as a workaround. Our team has been using composition for a while now but it creates a problem when trying to create a modular custom theme for complex client applications, the types of applications that benefit so much from encapsulating the business logic into a class rather than having the business logic littered through theme templates.

The problem is that the get_post() function checks is_a( $post, 'WP_Post' ) and also does new WP_Post( $post ) and WP_Post::get_instance( $post ) for different values of $post resulting in it destroying our object that contains an instance of WP_Post. And you can't get around this by just calling your own function instead of get_post() because so many other WordPress API functions call get_post() internally such as get_the_title() and WP_Screen::get(), for example.

I'd like to propose is a 'do_get_post' filter.

This filter would be similar to `'do_parse_request'` in that it would call a filter passing it a false value along with other values and if the filter returns a value that does not evaluate to false it will cause get_post() to simply return that value. This code would be included as the first 3 lines within get_post(), additionally passing along all the parameters that might have been passed into the get_post() function:

if ( $_post = apply_filters( 'do_get_post', false, $post, $output, $filter ) ) {
   return $_post;
}

This filter would be responsible for ensuring that a proper WP_Post object is returned and also responsible to respect the $output and $filter parameters.

This filter would allow us to build complex custom WordPress applications that make the use of objects that contain and emulate a WP_Post object (which we are already doing) without forcing us to replace all of the functionality of WordPress that assumes a global WP_Post object (which we cannot currently do, at least not without hacking core.)

Attachments (3)

30292.diff (1.0 KB) - added by MikeSchinkel 9 years ago.
Adds a 'do_get_post' filter to get_post()
30292.2.diff (2.5 KB) - added by MikeSchinkel 8 years ago.
Adds 'do_get_post' hook to get_post(), revised for 4.4.
30292.3.diff (2.5 KB) - added by MikeSchinkel 8 years ago.
Adds 'do_get_post' hook to get_post(), revised for 4.4. Also passes $post to filter.

Download all attachments as: .zip

Change History (19)

@MikeSchinkel
9 years ago

Adds a 'do_get_post' filter to get_post()

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


9 years ago

#2 @boonebgorges
9 years ago

  • Version changed from trunk to 3.5

#3 follow-up: @sc0ttkclark
9 years ago

This looks good to me, do we need a set of unit tests to test that custom objects aren't entirely destroyed when using the WP API template tags as you said? Or do all of those ultimately call get_post() which this would entirely resolve?

#4 in reply to: ↑ 3 @MikeSchinkel
8 years ago

  • Keywords has-patch dev-feedback added

Replying to sc0ttkclark:

This looks good to me, do we need a set of unit tests to test that custom objects aren't entirely destroyed when using the WP API template tags as you said? Or do all of those ultimately call get_post() which this would entirely resolve?

In my review they all call get_post().

Related #26877

#5 @MikeSchinkel
8 years ago

BTW, we are now running into some cases where we need to provide integration between 3rd party APIs but make them appear as if they are post types so it is very easy for themers to be able to interact with them.

For one of many examples imagine a invoices table from online Quickbooks. We could create a "virtual" post type that would synchronize with QB and appear to all practical purposes to have the data locally in a post type. But we need this hook to do it.

With this hook we could bring all the following integrations into WordPress:

#6 follow-up: @sc0ttkclark
8 years ago

For starters to help move this patch forward, it needs a refresh and the apply filters needs to be on it's own line outside of the if() logic and using null instead of false. Then you'll want to explicitly check if null !== and possibly override the variable here from $_post to be $override or similar. I could also be mistaken but I think hook docs for filters need a @return too, but maybe not.

I'm +1 on this patch once those things are done because of what it can provide for people.

#7 @sc0ttkclark
8 years ago

  • Keywords needs-refresh added

#8 in reply to: ↑ 6 @MikeSchinkel
8 years ago

  • Keywords 2nd-opinion added; needs-refresh removed

Replying to sc0ttkclark:

the apply filters needs to be on it's own line outside of the if() logic and using null instead of false. Then you'll want to explicitly check if null !== ...

Done.

and possibly override the variable here from $_post to be $override or similar.

Don't follow. Please elaborate.

I could also be mistaken but I think hook docs for filters need a @return too, but maybe not.

I checked about five (5) cases where apply_filters() was documented in core and none of them have an @return. Since the first @param implicitly documents the @return so including the latter would be redundant and thus come easily become inconsistent.

I refreshed the patch and also changed the logic to also run ->filter() and optionally to run ->to_array() when the return value of the filter is not null. This way the developer of the hook need not worry about filtering or converting to array, but the developer of the class of the returned object would obviously need to be concerned (if other than WP_Post).

I can refresh the patch again once I understand your point about $override.

I'm +1 on this patch once those things are done because of what it can provide for people.

:-)

Getting this in 4.4 would IMO be a huge boon to what could be made possible in WP.

@MikeSchinkel
8 years ago

Adds 'do_get_post' hook to get_post(), revised for 4.4.

#9 @MikeSchinkel
8 years ago

#26877 was marked as a duplicate.

#10 @MikeSchinkel
8 years ago

Decided to update patch to also pass $post because if passed to get_post() it would be important for the 'do_get_post' hook to have access to it.

Also added a @return value to the PHPDoc thus contradicting myself. (Doh!)

@MikeSchinkel
8 years ago

Adds 'do_get_post' hook to get_post(), revised for 4.4. Also passes $post to filter.

#11 @sc0ttkclark
8 years ago

While this patch could allow for circumventing of the getting of the post before it starts, there's another ticket #12955 which at minimum could provide the ability to filter the end result, which may be more useful for more developers. Wanted to mention it on this ticket as they can actually help to accomplish the same thing.

#12 @MikeSchinkel
8 years ago

@sc0ttkclark In an ideal world we'd get both filters: 'do_get_post' and 'get_post'.

But if we have to have just one then I guess #12955 would be preferable.

#13 @westonruter
8 years ago

#36879 was marked as a duplicate.

#14 follow-up: @westonruter
8 years ago

  • Keywords close added

Should we close this in favor of #12955? That seems to be where activity has continued.

#15 in reply to: ↑ 14 @MikeSchinkel
8 years ago

  • Resolution set to duplicate
  • Status changed from new to closed

Replying to westonruter:

Should we close this in favor of #12955? That seems to be where activity has continued.

Fine with me. So glad to see progress on this, thanks!

#16 @netweb
8 years ago

  • Milestone Awaiting Review deleted
Note: See TracTickets for help on using tickets.