Use of get_the_excerpt($post) is broken if post has no excerpt and you are inside a loop

[magicroundabout]

In changeset [36319] (and [36321]), a $post parameter was added to get_the_excerpt() in post-template.php. I was really excited when I discovered this but:

a) It's behaviour is inconsistent
b) It doesn't quite do what I'd hoped
c) It's broken in some circumstances

There are a few things wrong:

1) The behaviour is inconsistent depending on usage.

If you call get_the_excerpt() within the loop and if the post has no excerpt, then the post content will be stripped and truncated.

If you call it with the $post parameter and the post has no excerpt then nothing is returned.

Well...sometimes...

2) If you are inside a loop - say you're using get_the_excerpt($post) in a shortcode in a post - then you will actually get back the truncated content of the post from the loop that you are in.

This is because wp_trim_excerpt( $text ) does get_the_content('') if $text is empty.

I think that the fix for this is to add an optional $post parameter to wp_trim_excerpt() and process it accordingly.

We'll also need to update default-filters.php to pass the extra parameter on the get_the_excerpt filter hook.

This change has the added benefit that get_the_excerpt($post) works consistently, fetching and trimming the post content if the post has no defined excerpt. Hooray!! (Been wanting this for YEARS!)

I'm working on a patch. It would be good to write a test for this too but I have no idea how. Happy to take feedback.

[magicroundabout]

Attempt at patch

[magicroundabout]

Steps to reproduce:

• With a clean install of WordPress
• Add the test-excerpt.php template to the default theme directory
• Edit the "Sample page" to use the page template that I'm about to attach to this ticket
• View the page

You should see the latest post's title, but the sample page's excerpt.

With my patch you get the latest post's excerpt as expected.

[magicroundabout]

Sample theme template file for reproducing the issue

[magicroundabout]

Corrected patch created from root rather than from wp-includes (sorry - newbie error)

[achbed]

Should we (rather than target just this function) also apply the $post parameter to the get_the_content function?

[achbed]

Updated patch to push $post parameter through get_the_content

[achbed]

Proposed unit test

[achbed]

For now, I'm using the following as a half-baked workaround:

<?php
function filter_get_the_excerpt( $text = '', $post = null ) {
        if ( empty( $text ) ) {
                if ( ! empty( $post ) ) {
                        if ( ! ( $post instanceof \WP_Post ) ) {
                                $post = get_post( $post );
                        }
                        $text = $post->post_content;
                        $text = apply_filters( 'the_content', $text );
                        $text = str_replace(']]>', ']]&gt;', $text);

                        /**
                         * Filters the number of words in an excerpt.
                         *
                         * @since 2.7.0
                         *
                         * @param int $number The number of words. Default 55.
                         */
                        $excerpt_length = apply_filters( 'excerpt_length', 55 );
                        /**
                         * Filters the string in the "more" link displayed after a trimmed excerpt.
                         *
                         * @since 2.9.0
                         *
                         * @param string $more_string The string shown within the more link.
                         */
                        $excerpt_more = apply_filters( 'excerpt_more', ' ' . '[&hellip;]' );
                        $text = wp_trim_words( $text, $excerpt_length, $excerpt_more );
                }
        }
        return $text;
}
add_filter('get_the_excerpt','filter_get_the_excerpt', 1, 2);

[achbed]

Corrected proposed test

[achbed]

Updated patch to apply against revision 37881

[jorbin]

@swissspidy looks like two of your commits are a part of what's going on. Can you take a look?

[swissspidy]

On vacation but will have a look next week.

[swissspidy]

So the trouble began with #27246 after adding the optional $post parameter to get_the_excerpt(). A handful of tests where added for that function, but apparently the impact this change had on wp_trim_excerpt() went unnoticed until now.

wp_trim_excerpt() needs to be aware of the current post. Unfortunately, 36394.diff​ or 36934.3.patch​ are not enough, as get_the_content() relies on a bunch of global variables tied to the current post (especially$pages, $more and $numpages, see setup_postdata()) .

This means we need to use setup_postdata() to temporarily set these variables. 36934a.diff​ does this inside get_the_content() (introducing a new $post parameter) whereas 36934b.diff​ does this inside wp_trim_excerpt() (only adding the $post parameter there).

Both methods seem to work, but it needs tests.

I don't like how $more is dependant on get_queried_object_id(). This means the output ofget_the_excerpt() will be different depending on whether you're on that post or not (see current tests in the patches).

Ideally, these variables shouldn't be global at all, but instead be fetched via WP_Post methods. setup_postdata() could be simplified a lot.

[magicroundabout]

Ugh. What a mess I've found here. Thanks @swissspidy.

Do you need any more input from me on it? Or is it a case of deciding which of your patches is best, writing the tests and then committing the new code?

[peterwilsoncc]

@swissspidy I think this needs to avoid wp_reset_postdata() as it is possible the global post object isn't from the main query prior to this call. On first reading, 36934b.diff​ looks closer to the mark.

[achbed]

I tend to lean more towards adding the $post parameter to get_the_content() as it would bring the function more in line with similar functions (see get_the_title() and get_permalink()).

This does open a ton of worm cans as we need to ensure that we don't reset the query incorrectly and unexpectedly in a place it shouldn't normally happen. We would probably need to add a bunch of tests around this.

Given the scope of the issue and the timing of the 4.6 release, should we punt?

[magicroundabout]

I suspect that the answer to this is no, but given that I found a bug, and created a (really pretty simple) patch that fixed the bug for my use case (36394.2.diff), could we commit my simple patch for 4.6 (so I don't have to re-patch core when v4.6 comes out) and raise a new ticket for the rabbit hole of get_the_content( $post ) issues?

The downside of this is that wp_trim_excerpt() would now take a $post argument but wouldn't always get the expanded, filters-applied version of the content of that post before trimming it.

Thoughts?

[achbed]

Rather than patching core on my production boxen, I'm doing a "poor man's" filter instead on my live sites to paper over the issue. See comment 5 above.

I also tend to use a "fix once correctly" approach, rather than use a patch that may cause unknown collateral damage (more than already exists). Let's try to get this resolved permanently once.

[swissspidy]

@magicroundabout 36394.2.diff​ is not enough and shouldn't be committed for the reasons outlined above. It only solves half of the problems.

If this is not fixed properly, we might cause more harm than good, so we need to think this through thoroughly.

Let's try to get this resolved permanently once.

Absolutely. #10984 aims to make some changes to get_the_content() too, so we might need to do an overhaul of that function anyway. Being able to pass a specific $post to it makes sense.

Apart from that, 36934b.diff​ might be a solution, but without unit tests soon this won't make it into 4.6. I don't have much time at the moment, so some help would be appreciated. Otherwise this is a candidate for 4.7-early.

[magicroundabout]

Thanks @swissspidy and @achbed for your input. I totally get what you're saying. Let's do this properly in 4.7. I like the idea of having get_the_content( $post ) working properly too.

@achbed, I'd missed your workaround. That's neat. Well, it's a hack. But you know what I mean. Thanks!

You guys have a broader view of the WP codebase and potential other issues than I, so I'm going to leave it in your hands. Feel free to pull me in for help or testing if you need it. I'll keep a watch on the ticket.

[theMikeD]

Just ran into this too. Will help if I can.

[swissspidy]

#37519 was marked as a duplicate.

[swissspidy]

36934.diff​ is an improved version of 36934a.diff​.

Main changes:

• Adds an $args parameter to get_the_content() to make it more future proof.
• Gets rid of the $pages and $multipage globals by moving logic to WP_Post.
• Adds a test provided by @iandunn in #37519.

[achbed]

Replying to swissspidy:

• Gets rid of the $pages and $multipage globals by moving logic to WP_Post.

Should we have a quick scan done of the theme and plugin repository to see if this affects anyone doing_it_wrong()?

[swissspidy]

@achbed The globals aren't going away completely with that patch, get_the_content() just doesn't rely on them anymore. If anyone's doing global $multipage or global $pages, it will continue to work.

[boonebgorges]

In 38679:

Tests: Move get_the_excerpt() tests to their own file.

See #36934. Fixes #38196.

[boonebgorges]

I think that @swissspidy has correctly identified that we need a fix that's broader than what was originally suggested by @magicroundabout. But I don't think that setup_postdata() is smart enough to do what's being asked of it.

See 36934.2.diff​. I spent some time writing tests, to attempt to get my mind around what's happening in this, the very nether regions of The WordPress. The new test test_should_respect_pagination_of_inner_post() for get_the_content() is failing. Briefly, WP_Query::setup_postdata() can be passed a $post parameter. But checks like $this->get( 'page' ), $this->is_page(), and $this->is_single() assume that the $post is coming from the current WP_Query loop. If you are inside of a loop, and you pull up an unrelated $post and pass it to get_the_excerpt() - which is, if I understand it, the original idea in this ticket - the results will be affected by parameters of the loop. I've demonstrated this in the test by showing that the second page of the inner post is being fetched, because the page query_var of the outer loop is 2.

This suggests that a complete fix for this issue will mean abstracting get_the_content() even more. Perhaps the $args array should accept page, more, and preview, and only reference the globals (and thus setup_postdata() as fallbacks.

[boonebgorges]

I think this ticket touches too much at too deep a level to address at this point in the cycle. @swissspidy I'm going to assign this back to you for some Deep Thoughts when you've got time.

[swissspidy]

See also #21894.

[swissspidy]

#12976 was marked as a duplicate.

[swissspidy]

#39949 was marked as a duplicate.

[swissspidy]

36934.3.diff​ is a refreshed patch against trunk that applies cleanly and also makes all tests pass.

The main thing I changed was similar to the change in [30085]:

if ( ! $page || $post->ID !== get_queried_object_id() ) {
    $page = 1;
}

[achbed]

Is it a good thing or a bad thing that the only issue I have with this latest patch is the value for @since in the docblock in src/wp-includes/formatting.php?

Thanks for all the long hard looks @swissspidy ! This one's been a doozy because of all the core WP_Post and WP_Query changes required. I think it sets us all on a better footing though.

[magicroundabout]

Thanks, @swissspidy. Amazing work.

As raiser of this ticket, all I'm going to say is that I've tested the patch and that it fixes the issue that was originally raised. But as it fixes a load of other stuff it probably needs a more in-depth review which would be better done by someone who knows more about it then I.

So I'll leave you guys to it.

Thanks again.

[swissspidy]

@boonebgorges What do you think, can we get this into trunk as soon as 4.8 is branched?

[burlingtonbytes]

@boonebgorges and @swissspidy Now that 4.8 stable has been released, how close are we to getting this merged into trunk? We ran across this bug again today in a client project. Especially since we already have a viable patch, it would be great to see this make it into 4.8.1.

Let us know if we can help move this along.

[boonebgorges]

This feels like too big a change for a 4.8.1, which, as I understand, is now being conceived more in our previous tradition of regression-only releases.

I've reviewed the patch and it looks good so far as far as my hazy memory allows me to understand. @swissspidy If you are confident, let's put this into trunk for 4.9 so we can have time to suss out issues.

[swissspidy]

@boonebgorges Sounds good to me.

I also just found [24598] the other day which sheds some light on get_the_content() and all the global variables involved.

[swissspidy]

Unfortunately there are some failing tests :(

1) Tests_Query_SetupPostdata::test_page_from_wp_query
Failed asserting that 1 is identical to 78.

tests/phpunit/tests/query/setupPostdata.php:234

2) Tests_Query_SetupPostdata::test_secondary_query_page
Failed asserting that 1 is identical to 3.

tests/phpunit/tests/query/setupPostdata.php:256

3) Tests_Query_SetupPostdata::test_setup_postdata_loop
Failed asserting that '<p>global post</p>' is not equal to <string:<p>global post</p>>.

tests/phpunit/tests/query/setupPostdata.php:377

[magicroundabout]

I'm not sure if it's allowed to "bump" tickets here, but this didn't make it into 4.9 and it seems to have lost some traction.

I ran into this problem again today. I try to avoid using WP_Query loops to prevent stomping on my globals, so I make a lot of use of arrays of posts, and I really want to be able to properly do get_the_excerpt( $post ).

Can I try to get some renewed interest in getting this closed?

Thanks

[swissspidy]

36934.4.diff​ is a refreshed patch to get the ball rolling again.

I'd have to dig a bit into the history, but perhaps we should just change the failing tests I mentioned previously.

They fail because of the change I mentioned in https://core.trac.wordpress.org/ticket/36934#comment:39.

[swissspidy]

#44741 was marked as a duplicate.

[tofandel]

I would like to see this fixed as well, it's really an easy patch (Sorry for the duplicate I didn't find this thread)

So the new function should be in wp-includes/formatting.php

<?php
/**
 * Generates an excerpt from the content, if needed.
 *
 * The excerpt word amount will be 55 words and if the amount is greater than
 * that, then the string ' [&hellip;]' will be appended to the excerpt. If the string
 * is less than 55 words, then the content will be returned as is.
 *
 * The 55 word limit can be modified by plugins/themes using the {@see 'excerpt_length'} filter
 * The ' [&hellip;]' string can be modified by plugins/themes using the {@see 'excerpt_more'} filter
 *
 * @since 1.5.0
 *
 * @param string $text Optional. The excerpt. If set to empty, an excerpt is generated.
 * @param WP_Post|int|null $post
 * @return string The excerpt.
 */
function wp_trim_excerpt( $text = '', $post = null ) {
        $raw_excerpt = $text;
        if ( '' == $text ) {
                $post = get_post( $post );
                if ( empty( $post ) ) {
                        $text = '';
                } else {
                        $text = $post->post_content;
                }

                $text = strip_shortcodes( $text );

                /** This filter is documented in wp-includes/post-template.php */
                $text = apply_filters( 'the_content', $text );
                $text = str_replace(']]>', ']]&gt;', $text);

                /**
                 * Filters the number of words in an excerpt.
                 *
                 * @since 2.7.0
                 *
                 * @param int $number The number of words. Default 55.
                 */
                $excerpt_length = apply_filters( 'excerpt_length', 55 );
                /**
                 * Filters the string in the "more" link displayed after a trimmed excerpt.
                 *
                 * @since 2.9.0
                 *
                 * @param string $more_string The string shown within the more link.
                 */
                $excerpt_more = apply_filters( 'excerpt_more', ' ' . '[&hellip;]' );
                $text = wp_trim_words( $text, $excerpt_length, $excerpt_more );
        }
        /**
         * Filters the trimmed excerpt string.
         *
         * @since 2.8.0
         *
         * @param string $text        The trimmed text.
         * @param string $raw_excerpt The text prior to trimming.
         */
        return apply_filters( 'wp_trim_excerpt', $text, $raw_excerpt );
}

And in wp-includes/default-filters.php the new filter should be:

add_filter( 'get_the_excerpt', 'wp_trim_excerpt', 10, 2 );

[swissspidy]

@tofandel Although it might seem like an easy fix at first glance, the issue is a bit more complex than that.

Looking at your code snippet, I can see that you replaced get_the_content() with $post->post_content. This alone is a regression as all the edge cases and filters from get_the_content() aren't being applied anymore.

That's why get_the_content() needs to support a $post parameter as well and why we need to get rid of all the global variables that function uses.

Please check out 36934.4.diff​ as the latest patch. It might not apply cleanly anymore as it's a bit older, but the idea is unchanged.

[SergeyBiryukov]

Related: #42814, #45267.

[magicroundabout]

Logging in to check progress of this. Seems to have died a death again. This is a core template tag that's been broken for three years. Can we get a milestone added or something and get it some focus post-5.0? Thanks.

[swissspidy]

@magicroundabout Check the related ticket for some new patches!:)

[spacedmonkey]

All those that are interested, I have uploaded a patch 42814.4.diff here. It is likely that this issue, will be resolved over there.

cc @magicroundabout

[anonymized_11892634]

Any change the patch will make it into an update soon?

[swissspidy]

@boonebgorges @magicroundabout Any chance you could give feedback on the latest patch on #42814 which should resolve this ticket?

[magicroundabout]

Busy today. But will look into it at some point. Great to hear this might finally be fixed. Thanks for all your input so far, @boonebgorges and @swissspidy - loving your work! 😃

[boonebgorges]

In 44941:

Posts: Avoid the use of globals in get_the_content() and related functions.

This changeset introduces $post parameters to get_the_content() and
wp_trim_excerpt(). When a $post object is passed to one of these functions,
the functions will operate on the data from that object, rather than from the
post globals ($authordata, $page, etc). This ensures that the functions work
in a predictable manner when used outside of the regular post loop.

The global-mismatch problem is surfaced in cases where get_the_excerpt() is
called outside of the post loop, on posts that don't have a defined excerpt. In
these cases, the post globals - used to generate a fallback excerpt - may refer
to the incorrect object, resulting in PHP notices or other unpredictable
behavior. See #36934 for a related issue.

Props spacedmonkey, kraftbj, Shital Patel.
Fixes #42814.

[boonebgorges]

See [44941]. It may be that [44941] fixes the current ticket. If anyone can confirm, we can mark as a duplicate.

[swissspidy]

@boonebgorges Haven't tested, but it probably does. [44941] contains my tests from this ticket here that cover this area.

[spacedmonkey]

Issue fixed in #42814

[hellofromTonya]

#45267 was marked as a duplicate.