WordPress.org

Make WordPress Core

#22448 closed defect (bug) (fixed)

WP_Post conversion overwrites object changes on the_posts filter

Reported by: ntm Owned by: ryan
Milestone: 3.5 Priority: high
Severity: normal Version: 3.5
Component: Query Keywords: has-patch commit
Focuses: Cc:

Description

I started testing with WP 3.5 beta 3 and the plugin I maintain (podPress). The plugin extends the array $this->posts via the filter the_posts (wp-includes/query.php - function get_posts()). It adds keys and values. But the array_map() command which has been added with the Changeset 22011

$this->posts = array_map( 'get_post', $this->posts );

sets the $this->posts array back to the content it had contained before the filter. It retrogrades the_posts filter in my case and the content of $this->posts after array_map() seems to be the same as before the filter.

Is it possible to move the array_map() command some lines up or the filter down?

(Changeset 22011 was added during the discussion of #21309)

Attachments (4)

22448.diff (483 bytes) - added by kovshenin 17 months ago.
22448.unit-tests.diff (1.5 KB) - added by kovshenin 17 months ago.
22448.2.diff (421 bytes) - added by ryan 17 months ago.
22448.3.diff (739 bytes) - added by scribu 17 months ago.

Download all attachments as: .zip

Change History (23)

comment:1 nacin17 months ago

Could you describe what you mean by "The plugin extends the array $this->posts via the filter the_posts"? Some code would be great. I don't see how this sets the posts back, unless the objects themselves are being changed.

comment:2 scribu17 months ago

  • Cc scribu added

comment:3 scribu17 months ago

  • Keywords reporter-feedback added

comment:4 ntm17 months ago

The plugin use the filter hook

add_filter('the_posts', array(&$podPress, 'the_posts'));

and calls the function

function the_posts($input) {
	if ( FALSE === is_admin() && !$this->settings['compatibilityChecks']['themeTested'] ) {
		$this->settings['compatibilityChecks']['themeTested'] = true;
		podPress_update_option('podPress_config', $this->settings);
	}
	if(!is_array($input)) {
		return $input;
	}
	foreach($input as $key=>$value) {
		$input[$key] = $this->addPostData($value);
	}
	return $input;
}

which adds some values.
After applying the filter the data of a post array ($this->posts) looks like

array (
  0 => 
  WP_Post::__set_state(array(
     'ID' => '6',
     'post_author' => '1',
     'post_date' => '2012-11-14 16:05:24',
     'post_date_gmt' => '2012-11-14 15:05:24',
     'post_content' => 'test podcast',
     'post_title' => 'wp35_ep',
     'post_excerpt' => '',
     'post_status' => 'publish',
     'comment_status' => 'open',
     'ping_status' => 'open',
     'post_password' => '',
     'post_name' => 'wp35_ep',
     'to_ping' => '',
     'pinged' => '',
     'post_modified' => '2012-11-14 16:05:24',
     'post_modified_gmt' => '2012-11-14 15:05:24',
     'post_content_filtered' => '',
     'post_parent' => '0',
     'guid' => 'http://127.0.0.1/wp35en/?p=6',
     'menu_order' => '0',
     'post_type' => 'post',
     'post_mime_type' => '',
     'comment_count' => '0',
     'filter' => NULL,
     'podPressMedia' => 
    array (
      0 => 
      array (
        'URI' => 'http://127.0.0.1/wp33en/wp-content/uploads/2012/10/FF102312.mp3',
        'title' => '',
        'type' => 'audio_mp3',
        'size' => 76292598,
        'duration' => '158:57',
        'previewImage' => 'http://127.0.0.1/wp35en/wp-content/plugins/podpress/images/vpreview_center.png',
        'dimensionW' => 0,
        'dimensionH' => 0,
        'rss' => 'on',
        'atom' => 'on',
        'content_level' => 'free',
        'ext' => 'mp3',
        'mimetype' => 'audio/mpeg',
        'authorized' => true,
      ),
    ),
     'podPressPostSpecific' => 
    array (
      'itunes:subtitle' => '##PostExcerpt##',
      'itunes:summary' => '##PostExcerpt##',
      'itunes:keywords' => '##WordPressCats##',
      'itunes:author' => '##Global##',
      'itunes:explicit' => 'Default',
      'itunes:block' => 'Default',
    ),
  )),
...

The filter data from the wp_postmeta table. The data is used for multiple purposes by the plugin e.g to add additional elements to the RSS and ATOM feeds or to the posts on web page.
The functions which should do this use the global variable $post. But the new line of code reduces the data to the default key+value pairs.

After the new array_map(...) command the array is this (again):

array (
  0 => 
  WP_Post::__set_state(array(
     'ID' => '6',
     'post_author' => '1',
     'post_date' => '2012-11-14 16:05:24',
     'post_date_gmt' => '2012-11-14 15:05:24',
     'post_content' => 'test podcast',
     'post_title' => 'wp35_ep',
     'post_excerpt' => '',
     'post_status' => 'publish',
     'comment_status' => 'open',
     'ping_status' => 'open',
     'post_password' => '',
     'post_name' => 'wp35_ep',
     'to_ping' => '',
     'pinged' => '',
     'post_modified' => '2012-11-14 16:05:24',
     'post_modified_gmt' => '2012-11-14 15:05:24',
     'post_content_filtered' => '',
     'post_parent' => '0',
     'guid' => 'http://127.0.0.1/wp35en/?p=6',
     'menu_order' => '0',
     'post_type' => 'post',
     'post_mime_type' => '',
     'comment_count' => '0',
     'filter' => NULL,
  )),
...

This is also what the array is before the filter hook.

comment:5 Viper007Bond17 months ago

Modifying the post object also won't play nice with caching. We ran into this issue a lot with WordPress.com VIP code once we merged WP_Post into WP.com. We came to the conclusion that modifying the post object to add additional fields wasn't a good idea (and that's probably correct) however it's certainly a regression of sorts.

comment:6 scribu17 months ago

  • Keywords reporter-feedback removed
  • Milestone changed from Awaiting Review to 3.5

comment:7 nacin17 months ago

  • Keywords needs-unit-tests added

comment:8 nacin17 months ago

  • Summary changed from Changeset 22011 corrupts the effect of the the_posts filters to WP_Post conversion overwrites object changes on the_posts filter

comment:9 nacin17 months ago

  • Owner set to ryan
  • Priority changed from normal to high
  • Status changed from new to assigned

kovshenin17 months ago

comment:10 kovshenin17 months ago

  • Cc kovshenin added
  • Keywords has-patch needs-testing added

Howdy! I spent some time figuring out how all of this works, doesn't seem to be caused by [22011] from what I see. The problem is because of the behavior of get_post, which is supposed to retrieve a fresh post from cache if filter is (but was not) raw.

The behavior was copied into WP_Post but with the current implementation of WP_Query, the cache exists for all posts, before get_instance is called, so the sanitize_post inside get_instance is never called (within a WP_Query scenario). The 3.4 scenario used to explicitly call sanitize_post, but 3.5 relies on get_post instead. So in 3.4, after a successful query, every post in the posts array will have its filter set to raw, whereas 3.5 keeps it blank, causing WP_Post::filter to call get_instance over and over again (because the cached version has no filter either).

I'm not sure this is the only problem, and I'm pretty sure 22448.diff is not the best solution, but it does make sure that when retrieved, a fresh post is always sanitized, meaning WP_Post::filter can not run the get_instance routine unless the filter has been explicitly changed to something other than raw. 22448.unit-tests.diff adds some tests, covers adding more posts via the_posts filter, and adding custom data using the_posts filter, as initially described in this ticket.

phpunit --group post
phpunit --group query
phpunit --group 22448

Hope this helps :)

comment:11 ntm17 months ago

The modifications suggested in 22448.diff solving the problem. The $this->posts array which gets modified via the the_posts filter hook stays the same passing the line $this->posts = array_map( 'get_post', $this->posts );
in the /wp-includes/query.php file.

Thank you kovshenin!

ryan17 months ago

comment:12 follow-ups: ryan17 months ago

Moved to elseif to avoid the empty check after hitting the DB. Removed wp_cache_set() since we try to avoid using it (we always try to add instead).

comment:13 in reply to: ↑ 12 ntm17 months ago

Replying to ryan:
That works too.

comment:14 in reply to: ↑ 12 kovshenin17 months ago

Replying to ryan: That makes total sense, and passes the unit tests too :)

comment:15 scribu17 months ago

  • Keywords needs-unit-tests removed

comment:16 ryan17 months ago

  • Keywords commit added

comment:17 nacin17 months ago

At a glance, makes sense and looks good. Let me know if I am needed to take off my shoes and dive in to satisfy the "needs-testing" keyword.

scribu17 months ago

comment:18 scribu17 months ago

  • Keywords needs-testing removed

It took me a while to figure out where $_post was coming from in the elseif block. 22448.3.diff just makes the call to wp_cache_get() more explicit.

Other than that, looks good.

Last edited 17 months ago by scribu (previous) (diff)

comment:19 ryan17 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 22635:

Always return a raw filtered post object from WP_Post::get_instance().

Props kovshenin, scribu, ntm
fixes #22448

Note: See TracTickets for help on using tickets.