Make WordPress Core

Opened 11 years ago

Closed 11 years ago

Last modified 4 years ago

#24330 closed defect (bug) (fixed)

When adding the post_formats_compat the_content filter a post ID argument should be included

Reported by: gcorne's profile gcorne Owned by:
Milestone: 3.6 Priority: normal
Severity: normal Version: 3.6
Component: Post Formats Keywords:
Focuses: Cc:

Description

The new post_formats_compat() function relies on knowing the $post_id to get meta data for the post format. By default, the function is bound as a the_content filter, which doesn't pass a $post_ID. One possible solution is to change the number of accepted arguments when adding the filter to 2, although this will require third-party developers that are calling apply_filters( 'the_content', $content ) with an unset $GLOBALS['post'] or the global set to a different post to pass in the post id: apply_filters( 'the_content', $content, $post_id ).

wp-includes/comment.php:1752 is an example from core of using the_content on content that is not pulled from the global $post.

For completeness, it may make sense to do the same with the prepend_attachment filter.

Attachments (15)

24330-01.patch (1.1 KB) - added by gcorne 11 years ago.
24330.diff (3.9 KB) - added by ryan 11 years ago.
Pass the post ID down the the_content() stack. Add phpdoc for the_content filter explaining portability issues of the ID arg
24330.2.diff (8.1 KB) - added by ryan 11 years ago.
Introduce wp_parse_post_content() and use it in get_the_content() to avoid global $pages
24330-ut.diff (737 bytes) - added by ryan 11 years ago.
Unit tests
24330.3.diff (8.3 KB) - added by DrewAPicture 11 years ago.
Better phpdoc, remove extra $page global in the else
24330.4.diff (10.3 KB) - added by ryan 11 years ago.
Avoid $format_pages. Avoid number of times content is copied around in wp_parse_post_content()
24330.5.diff (11.3 KB) - added by ryan 11 years ago.
Pass post ID to the_remaining_content()
24330.6.diff (11.1 KB) - added by ryan 11 years ago.
24330.7.diff (11.5 KB) - added by ryan 11 years ago.
Declare format_content and split_content as vars for WP_Post and provide phpdoc
24330-ut.2.diff (2.5 KB) - added by ryan 11 years ago.
24330.8.diff (11.6 KB) - added by ryan 11 years ago.
Use the id arg in get_the_password_form(). Props duck_.
24330.9.diff (11.7 KB) - added by ryan 11 years ago.
Indicate that post_password_required() and get_the_password_form() can accept an ID or WP_Post object.
24330.10.diff (12.1 KB) - added by ryan 11 years ago.
Re-add the filter in the_remaining_content() with the correct number of args. Props Aaron.
24330.patch (458 bytes) - added by johnjamesjacoby 11 years ago.
24330.11.diff (557 bytes) - added by ryan 11 years ago.

Download all attachments as: .zip

Change History (47)

@gcorne
11 years ago

#1 @apeatling
11 years ago

  • Cc apeatling@… added

#2 @ryan
11 years ago

  • Milestone changed from Awaiting Review to 3.6

#3 @markjaquith
11 years ago

WordPress core has, for years, relied on the convention that the_content filters are meant to be run inside a loop context with a valid global $post object. Multiple WordPress core functions already make that assumption, like gallery_shortcode() and prepend_attachment(), as well as all our embed handlers (which do caching by post ID). Any third party shortcode that touches post meta will be in the same boat, and won't work if $post isn't as expected.

It's not very clean, but it is expected and well-established behavior.

#4 @kovshenin
11 years ago

  • Cc kovshenin added

#5 @gcorne
11 years ago

I agree that it is well established behavior, but I don't agree that it is expected. The usage in do_trackbacks() is evidence that it is easy to not be aware of the state needed to properly apply the_content. It is also unfortunate that we are adding *new* code that is relying on $GLOBALS['post']}}. At this point, isn't everyone in agreement that usage of the global {{{$post should be frowned upon if not deprecated?

Version 0, edited 11 years ago by gcorne (next)

@ryan
11 years ago

Pass the post ID down the the_content() stack. Add phpdoc for the_content filter explaining portability issues of the ID arg

#6 @ryan
11 years ago

I didn't using real phpdoc params to avoid possibly polluting the generated phpdoc with things it does not understand.

Last edited 11 years ago by ryan (previous) (diff)

@ryan
11 years ago

Introduce wp_parse_post_content() and use it in get_the_content() to avoid global $pages

#7 @ryan
11 years ago

Global $pages in setup_postdata() is a problem. Passing an ID makes no difference since get_the_content() goes to $pages for the content. 24330.2.diff works around this, but the surgery is not minor.

The new $format_pages variable is usually a straight up duplicate of $pages. Storing the content twice in globals could use up a lot of memory.

@ryan
11 years ago

Unit tests

@DrewAPicture
11 years ago

Better phpdoc, remove extra $page global in the else

#8 @DrewAPicture
11 years ago

24330.3.diff fleshes out the phpdoc for wp_parse_post_content(). I also removed the extra $page global in the else.

#9 @ethitter
11 years ago

  • Cc erick@… added

#10 @ryan
11 years ago

I'll use this ticket for some quick notes on the guts of post formats.

It'd be nice if $format_pages is only set when absolutely necessary, such as when $post->split_content is set. Even better would be for this global to not exist. We have enough problems with $pages as it is. Perhaps we run with the patch here after all and extend it to make the_content() and the_remaining_content() independent of the globals in setup_postdata().

$post->split_content and $post->format_content[ $cache_key ] are bummers. If we're going to decorate WP_Post with these perhaps they should be declared as vars and documented with phpdoc. Also, are these getting into the object cache? Should they?

#11 @ryan
11 years ago

The patch here plus abstracting some stuff out of get_the_content() as hinted at in the phpdoc for get_the_remaining_content() could allow us to avoid shipping with $format_pages. I would really like to avoid introducing that global.

$post->format_content[] could go into a non-persistent cache bucket with a key like $post_id:media:$type to avoid decorating the object.

@ryan
11 years ago

Avoid $format_pages. Avoid number of times content is copied around in wp_parse_post_content()

@ryan
11 years ago

Pass post ID to the_remaining_content()

@ryan
11 years ago

@ryan
11 years ago

Declare format_content and split_content as vars for WP_Post and provide phpdoc

@ryan
11 years ago

@ryan
11 years ago

Use the id arg in get_the_password_form(). Props duck_.

@ryan
11 years ago

Indicate that post_password_required() and get_the_password_form() can accept an ID or WP_Post object.

#13 @aaroncampbell
11 years ago

Seems to work as expected for me.

One thing I noticed: here you pass two parameters to post_formats_compat - http://core.trac.wordpress.org/attachment/ticket/24330/24330.9.diff#L9

But when you remove it and put it back you don't: http://core.trac.wordpress.org/attachment/ticket/24330/24330.9.diff#L234

@ryan
11 years ago

Re-add the filter in the_remaining_content() with the correct number of args. Props Aaron.

#14 @ryan
11 years ago

In 24301:

  • Introduce wp_parse_post_content() and use it in setup_postdata(), get_the_content(), and get_the_remaining_content().
  • Add a post ID argument to the_content(), get_the_content(), the_remaining_content(), and get_the_remaining_content().
  • Pass the post ID to the the_content filter.
  • Remove the format_pages global.
  • Declare format_content and split_content as vars in WP_Post.
  • phpdoc for the the_content filter that documents the new ID argument and denotes it as not-so-portable.

Props gcorne, DrewAPicture, duck_, aaroncampbell
see #24330

#16 follow-up: @johnjamesjacoby
11 years ago

Two things:

  • Missed the second argument in the remove_filter() call inside of the_remaining_content(). (Patch incoming)
  • r24301 broke some code we were using in a theme, that worked previously. Looks like this is a breaking change for loops like this:
    foreach( array( 8, 10, 12 ) as $id ) {
    	$page = get_post( $id );
    	if ( $page ) {
    		setup_postdata( $page );
    		the_content();
    		wp_reset_postdata();
    	}
    }
    
Last edited 11 years ago by johnjamesjacoby (previous) (diff)

#17 @johnjamesjacoby
11 years ago

Also, the use of extract() here is a time-bomb. It's not obvious (even with the inline doc in get_the_content()) that wp_parse_post_content() sets up would-be global variables.

#18 @hissy
11 years ago

Hi, I have one thing...

If global $page is not set, this code returns null. Is it correct?

get_the_content('',false,1);

It seems that $page should be tested.

	if ( !is_int($page) || $page > count( $pages ) ) // if the requested page doesn't exist
		$page = count( $pages ); // give them the highest numbered page that DOES exist

#19 in reply to: ↑ 16 ; follow-up: @aaroncampbell
11 years ago

Replying to johnjamesjacoby:

  • Missed the second argument in the remove_filter() call inside of

remove_filter() only takes 3 arguments, so I don't think 24330.patch is necessary.

#20 in reply to: ↑ 19 @johnjamesjacoby
11 years ago

Replying to aaroncampbell:

Replying to johnjamesjacoby:

  • Missed the second argument in the remove_filter() call inside of

remove_filter() only takes 3 arguments, so I don't think 24330.patch is necessary.

You're correct. Disregard that patch.

#21 @ryan
11 years ago

[UT1289] is a unit test for the loop problem found by johnjamesjacoby.

#22 @ryan
11 years ago

setup_postdata() sets global $id but not global $post. This mismatch is the reason the new code breaks for loops that don't set global post to match the post passed to setup_postdata(). In get_the_content(), "if ( $post->ID != $GLOBALSid? )" is true in this case and wp_parse_post_content() is run on get_post( 0 ), which is the global post rather than the post that has its content sitting in the pages global. It was sheer accident that these loops ever worked, but worked they did so I'll see what can be done for back compat.

Last edited 11 years ago by ryan (previous) (diff)

@ryan
11 years ago

#24 @ryan
11 years ago

Patch checks the global post rather than the global post ID when checking for a mismatch between the requested post and the global post. The global post is more reliable and proper than the global ID for this situation.

#25 @markjaquith
11 years ago

I agree that $post is more reliable.

#26 @nacin
11 years ago

I thought we eliminated global $id everywhere. Guess we missed this one?

One suggestion, can we use get_post()->ID?

#27 @ryan
11 years ago

In 24336:

Use the global post rather than the global post ID. They don't always match, and the global post is more canonical.

see #24330

#28 @nacin
11 years ago

In 24337:

Remove use of global $id from comment_form() and the media list table. see #24330.

#29 @ryan
11 years ago

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

#30 @nacin
11 years ago

In 24598:

Remove wp_parse_post_content(), get_paged_content(), paginate_content() from 3.6, and remove the new $id parameters for get_the_content() and the_content().

The content parsing functions are good abstractions, but are no longer needed by core and are too closely tied to legacy globals, rather than paving a new path.

For get_the_content() and the_content(), this only worsens the function prototype. It muddies theme-specific display (more links, etc) with filtered content. apply_filters( 'the_content', $post->post_content ) is sufficient practice for now.

see #24330, [24301]. see #23625, [23804].

#31 @ryan
11 years ago

In 1314/tests:

Remove test_the_content_with_id(). ID supports was removed in [24598]. see #24330

#32 @SergeyBiryukov
4 years ago

In 48113:

Posts, Post Types: Simplify test_setup_postdata_loop().

The important part here is calling the_content() after setting up post data for another post without updating global $post.

The foreach() loop is not necessary.

Follow-up to [UT1289].

See #47824, #24330.

Note: See TracTickets for help on using tickets.