Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#49609 closed enhancement (duplicate)

Code simplification and bug prevention in get_the_content()

Reported by: dontdream's profile dontdream Owned by:
Milestone: Priority: normal
Severity: minor Version:
Component: Posts, Post Types Keywords: has-patch close
Focuses: Cc:


In the initial lines of get_the_content():

function get_the_content( $more_link_text = null, $strip_teaser = false, $post = null ) {
	global $page, $more, $preview, $pages, $multipage;

	$_post = get_post( $post );

	if ( ! ( $_post instanceof WP_Post ) ) {
		return '';

	if ( null === $post ) {
		$elements = compact( 'page', 'more', 'preview', 'pages', 'multipage' );
	} else {
		$elements = generate_postdata( $_post );

when the $post argument is null, two cases are possible:

1) get_post() finds a post
2) get_post() doesn't find a post

In case 2) get_the_content() will immediately return, so the remaining lines to set $elements can be simplified for clarity.

	$elements = generate_postdata( $_post );

See the attached patch.

Attachments (1)

49609.diff (1.7 KB) - added by dontdream 3 years ago.

Download all attachments as: .zip

Change History (6)

3 years ago

#1 @ocean90
3 years ago

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

This seems to be same as you have previously submitted in #49591.

Note that there are two similar named variables, $_post (with underscore) and $post. Depending on the context they are not the same. This was introduced in [44941].

#2 @dontdream
3 years ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

Hi ocean90,

Yes, the patch is practically the same but my explanation in #49591 was wrong, so I closed that ticket and submitted a new one - sorry for the confusion!

To rephrase my explanation above, let's focus on the lines:

1	if ( ! ( $_post instanceof WP_Post ) ) {
2		return '';
3	}
5	if ( null === $post ) {
6		$elements = compact( 'page', 'more', 'preview', 'pages', 'multipage' );
7	} else {
8		$elements = generate_postdata( $_post );
9	}

When the $post argument is null (line 5) the $_post instance (coming from the global $post) is also available (otherwise get_the_content() returns, line 2), so it's not necessary to build the $elements array with compact(), we can use generate_postdata( $_post ) as well.

Reopening this ticket for further consideration. Thank you!

#3 @ocean90
3 years ago

  • Keywords close added

If no post is passed to get_the_content() it assumes that it's used in a context where all globals have been set up already. That's why it's using a simple compact() for the existing globals instead of the heavy generate_postdata() function.

#4 @dontdream
3 years ago

  • Summary changed from Minor code cleanup to Code simplification and bug prevention in get_the_content()

Only two additional considerations:

First, I realized that "code cleanup" isn't a good choice of words, because it might suggest that the code isn't "clean" right now. Obviously that wasn't my intention, I should have said "code simplification" (my English is limited, sorry).

Second, get_the_content() assumes that, when the global $post is set, global $page, $more, $preview, $pages, $multipage are also correctly set, but that's not always the case. At least $pages is sometimes not set up correctly, as shown by a report I received a few days ago, see:

I haven't been able to reproduce the problem, but if we replace compact() with generate_postdata(), we guarantee that $elements is set correctly even when the $post argument is null.

To conclude, I think I've shared all the information I have and now I leave the final decision to you. In the meantime, I'm going to edit the ticket summary for clarity.

#5 @dontdream
3 years ago

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

Found in #47824:

get_the_content(), when called without an explicit post object, falls back to the $post global too, but makes an incorrect assumption that other globals (specifically $pages) are always set up in that case.

I'm going to mark the current ticket as a duplicate of #47824.

Note: See TracTickets for help on using tickets.