WordPress.org

Make WordPress Core

Opened 11 months ago

Closed 11 months ago

Last modified 9 months 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 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 months ago.
24330.diff (3.9 KB) - added by ryan 11 months 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 months 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 months ago.
Unit tests
24330.3.diff (8.3 KB) - added by DrewAPicture 11 months ago.
Better phpdoc, remove extra $page global in the else
24330.4.diff (10.3 KB) - added by ryan 11 months 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 months ago.
Pass post ID to the_remaining_content()
24330.6.diff (11.1 KB) - added by ryan 11 months ago.
24330.7.diff (11.5 KB) - added by ryan 11 months 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 months ago.
24330.8.diff (11.6 KB) - added by ryan 11 months ago.
Use the id arg in get_the_password_form(). Props duck_.
24330.9.diff (11.7 KB) - added by ryan 11 months 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 months 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 months ago.
24330.11.diff (557 bytes) - added by ryan 11 months ago.

Download all attachments as: .zip

Change History (46)

gcorne11 months ago

comment:1 apeatling11 months ago

  • Cc apeatling@… added

comment:2 ryan11 months ago

  • Milestone changed from Awaiting Review to 3.6

comment:3 markjaquith11 months 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.

comment:4 kovshenin11 months ago

  • Cc kovshenin added

comment:5 gcorne11 months 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?

Last edited 11 months ago by gcorne (previous) (diff)

ryan11 months ago

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

comment:6 ryan11 months ago

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

Version 0, edited 11 months ago by ryan (next)

ryan11 months ago

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

comment:7 ryan11 months 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.

ryan11 months ago

Unit tests

DrewAPicture11 months ago

Better phpdoc, remove extra $page global in the else

comment:8 DrewAPicture11 months ago

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

comment:9 ethitter11 months ago

  • Cc erick@… added

comment:10 ryan11 months 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?

comment:11 ryan11 months 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.

ryan11 months ago

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

ryan11 months ago

Pass post ID to the_remaining_content()

ryan11 months ago

ryan11 months ago

Declare format_content and split_content as vars for WP_Post and provide phpdoc

ryan11 months ago

ryan11 months ago

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

ryan11 months ago

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

comment:13 aaroncampbell11 months 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

ryan11 months ago

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

comment:14 ryan11 months 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

comment:16 follow-up: johnjamesjacoby11 months 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 months ago by johnjamesjacoby (previous) (diff)

johnjamesjacoby11 months ago

comment:17 johnjamesjacoby11 months 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.

comment:18 hissy11 months 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

comment:19 in reply to: ↑ 16 ; follow-up: aaroncampbell11 months 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.

comment:20 in reply to: ↑ 19 johnjamesjacoby11 months 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.

comment:21 ryan11 months ago

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

comment:22 ryan11 months 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 months ago by ryan (previous) (diff)

ryan11 months ago

comment:24 ryan11 months 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.

comment:25 markjaquith11 months ago

I agree that $post is more reliable.

comment:26 nacin11 months ago

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

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

comment:27 ryan11 months 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

comment:28 nacin11 months ago

In 24337:

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

comment:29 ryan11 months ago

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

comment:30 nacin9 months 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].

comment:31 ryan9 months ago

In 1314/tests:

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

Note: See TracTickets for help on using tickets.