Make WordPress Core

Opened 13 years ago

Closed 11 years ago

Last modified 10 years ago

#17935 closed defect (bug) (wontfix)

Quick edit in admin bypasses custom_column filters in wp-admin/edit.php

Reported by: postpostmodern's profile postpostmodern Owned by: wonderboymusic's profile wonderboymusic
Milestone: Priority: normal
Severity: normal Version: 3.2
Component: Query Keywords: has-patch
Focuses: Cc:

Description

Calling get_post( integer ) bypasses all normal query filters, like posts_fields and posts_join. I ran into this problem when using Quick Edit on wp-admin/edit.php in conjunction with query filters in a plugin.

Example plugin:

/*
Plugin Name: get_posts bug report 
*/

add_filter( 'posts_fields', 'get_post_bug_posts_fields' );
add_filter( 'posts_request', 'get_post_bug_posts_request' );
add_filter( 'manage_posts_columns', 'get_post_bug_manage_posts_columns' );
add_filter( 'manage_posts_custom_column', 'get_post_bug_manage_posts_custom_column', 10, 2 );

function get_post_bug_posts_fields( $sql ){
	$sql .= ", 'ffffoooooooo' AS `foo`, 'baaaaaarrrr' AS `bar`";
	return $sql;
}

function get_post_bug_posts_request( $sql ){
	//var_dump( $sql );
	return $sql;
}

function get_post_bug_manage_posts_columns( $headings ){
	$a = array_slice( $headings, 0, 3 );
	$b = array_slice( $headings, 4 );
	
	$headings = array_merge( $a, array('foo'=>'Foo'), array('bar'=>'Bar'), $b );
	return $headings;
}

function get_post_bug_manage_posts_custom_column( $column_name, $post_id ){
	global $post;
	
	switch( $column_name ){
		case 'foo':
			echo $post->foo;
			break;
		
		case 'bar':
			echo $post->bar;
			break;
	}
}

On page load, the edit list table will load correctly with two custom columns, however saving the post from the Quick Edit interface will result in the two columns having no data.

Proposed fix:

wp-includes/post.php line 391

$_post = $wpdb->get_row($wpdb->prepare("SELECT * FROM $wpdb->posts WHERE ID = %d LIMIT 1", $post_id));

becomes

$_posts = query_posts( array('p' => $post_id, 
			     'post_type' => 'any', 
			     'post_status' => 'any', 
			     'posts_per_page' => 1) );
$_post = count( $_posts ) ? $_posts[0] : $wpdb->get_row( $wpdb->prepare("SELECT * FROM $wpdb->posts WHERE ID = %d LIMIT 1", $post_id) );

Attachments (2)

admin-ajax.diff (666 bytes) - added by postpostmodern 13 years ago.
Modifying inline-save action (Quick Edit) to use WP_Query rather than get_post()
query-ajax-list-table.diff (616 bytes) - added by wonderboymusic 11 years ago.

Download all attachments as: .zip

Change History (14)

#1 @scribu
13 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to invalid
  • Status changed from new to closed

get_posts() uses WP_Query, which calls all the filters you mentioned.

get_post() fetches a single post directly, so it doesn't make any sense for it to have those filters.

You will have to find another way to achieve what you want.

Last edited 13 years ago by scribu (previous) (diff)

#2 @scribu
13 years ago

Furthermore, adding those filters, without going through WP_Query would break a lot of plugins that expect WP_Query.

If it did go through WP_Query, it would have a significant overhead, which isn't warranted and would still probably break several plugins.

Last edited 13 years ago by scribu (previous) (diff)

#3 @postpostmodern
13 years ago

That's understandable that using WP_Query inside of get_post() can not be used because of ramifications with plugins and overhead.

However, I think that this bug in relation to the Quick Edit interface is still valid, here is my reasoning:

Wordpress provides APIs to select extra data along with my post, as well as APIs to display this in the edit table and sort the columns which this data belongs to. This data is automatically passed to the WP_Posts_List_Table class in wp-admin/edit.php via prepare_items(), which is using the WP_Query by way of wp_edit_posts_query().

The 'inline-save' case of wp-admin/admin-ajax.php is bypassing this logic by using get_post instead, and in doing so, loses functionality.

As it exists right now, it seems you are asking developers to make another database call inside of the manage_posts_custom_column filter to get the data they need. This is less than optimal, since adding extra database calls for each post will add to the overhead, not to mention losing the ability to sort the table on this custom column.

Modifying the 'inline-save' case in wp-admin/admin-ajax.php, line 1227 to the following would allow this extra flexibility with the custom tables, as well as not impacting functionality outside of Quick Edit:

$wp_list_table->display_rows( array( get_post( $_POST['post_ID'] ) ) );

to:

$posts = get_posts( array('p'=>$post_ID, 'post_type'=>'any', 'post_status'=>'any', 'suppress_filters'=>FALSE, 'posts_per_page'=>1) );
$post = count( $posts ) ? $posts[0] : get_post( $post_ID );
$wp_list_table->display_rows( array($post) );

#4 @scribu
13 years ago

  • Milestone set to Awaiting Review
  • Resolution invalid deleted
  • Status changed from closed to reopened

Ok, I agree that this can be a problem. The proposed solution threw me off.

As it exists right now, it seems you are asking developers to make another database call inside of the manage_posts_custom_column filter to get the data they need.

What kind of data are you refering to?

$posts = get_posts( array('p'=>$post_ID, 'post_type'=>'any', 'post_status'=>'any', 'suppress_filters'=>FALSE, 'posts_per_page'=>1) );
$post = count( $posts ) ? $posts[0] : get_post( $post_ID );
$wp_list_table->display_rows( array($post) );

Why would you need to call both get_posts() and get_post()? The post either is there or it isn't.

Last edited 13 years ago by scribu (previous) (diff)

#5 @postpostmodern
13 years ago

Hi, thanks for taking another look at this. Im glad you took this ticket - having written http://scribu.net/wordpress/custom-sortable-columns.html, may I use that code for reference?

In your example, price_column_display() makes a call to get_post_meta() on every row.

However, if you had expected that every post on your site would have a price, and it you would always want to display it, you might to have wanted to always join the postmeta table and select this data along with the post.

function price_posts_fields( $sql ){
	global $wpdb;
	$sql .= ", {$wpdb->postmeta}.meta_value AS `price`";
	return $sql;
}
add_filter( 'posts_fields', 'price_posts_fields' );

function price_posts_join( $sql ){
	global $wpdb;
	$sql .= " LEFT JOIN {$wpdb->postmeta} 
				ON {$wpdb->posts}.ID = {$wpdb->postmeta}.post_id 
				AND {$wpdb->postmeta}.meta_key = 'price' ";
	return $sql;
}
add_filter( 'posts_join', 'price_posts_join' );

Then, price_column_display() becomes

function price_column_display( $column_name, $post_id ) {
	global $post;
	echo trim($post->price) ? $post->price : __( 'undefined', 'my-plugin' );
}

This has the benefit of having price as part of $post site wide. In my development, I am using comparable functions to join geolocation data to each post, so $post->latitude and $post->longitude is globally available, including for display in the Edit table.

As far as calling both get_posts() and get_post(), there seems to be a problem when using the Quick Edit on a draft, as far as I can tell, around line 2643 in wp-includes/query.php, $post_status_obj->protected is TRUE on drafts, and ends up returning an empty array. Perhaps there is another query variable I should be using?

#6 @postpostmodern
13 years ago

  • Summary changed from function get_post( integer ) bypasses posts_* filters to Quick edit in admin bypasses custom_column filters in wp-admin/edit.php

@postpostmodern
13 years ago

Modifying inline-save action (Quick Edit) to use WP_Query rather than get_post()

#7 @wonderboymusic
11 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to Future Release
  • Owner set to wonderboymusic
  • Status changed from reopened to accepted

My patch fixes the problem in the ticket, not 100% sure how crucial this is based on the use case

#8 @wonderboymusic
11 years ago

  • Milestone changed from Future Release to 3.6

#9 follow-up: @markjaquith
11 years ago

  • Milestone 3.6 deleted
  • Resolution set to wontfix
  • Status changed from accepted to closed

WONTFIX, because this assumption is not true:

Wordpress provides APIs to select extra data along with my post

It provides APIs to select extra data when querying multiple posts. But code should consider that sometimes posts are selected individually from the cache, and provide an appropriate "else" case that fetches any missing data.

#10 in reply to: ↑ 9 @postpostmodern
10 years ago

Replying to markjaquith:

WONTFIX, because this assumption is not true:

Wordpress provides APIs to select extra data along with my post

It provides APIs to select extra data when querying multiple posts. But code should consider that sometimes posts are selected individually from the cache, and provide an appropriate "else" case that fetches any missing data.

I don't understand the assertion that one table, in one place in the admin, should be populated by two different methods, depending on whether it is done from a normal page load, or by an ajax call - especially if there is a solution that can consolidate this logic and not require an 'else' clause.

Could you explain the benefit of having two sets of code populate this table? If so, is there a third or fourth way of populating this table that I should be aware of?

#11 follow-up: @nacin
10 years ago

Adding new fields into WP_Query for what you're trying to do isn't supported. It pollutes the cache and can cause all sorts of problems and inconsistencies. You're going to want to instead use get_post_meta(), not $post->price. Then you won't have a problem in either context.

#12 in reply to: ↑ 11 @postpostmodern
10 years ago

Replying to nacin:

Adding new fields into WP_Query for what you're trying to do isn't supported. It pollutes the cache and can cause all sorts of problems and inconsistencies. You're going to want to instead use get_post_meta(), not $post->price. Then you won't have a problem in either context.

I will have a problem if I wish to sort on price. I don't quite understand how this pollutes the cache or causes problems - the posts_fields, posts_join, posts_orderby etc. filters are all supported and documented. Would adding 'cache_results' => false to the query solve this?

Note: See TracTickets for help on using tickets.