Make WordPress Core

Opened 8 years ago

Closed 6 years ago

Last modified 2 years ago

#36934 closed defect (bug) (duplicate)

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

Reported by: magicroundabout's profile magicroundabout Owned by: swissspidy's profile swissspidy
Milestone: Priority: normal
Severity: normal Version: 4.5
Component: Posts, Post Types Keywords: has-patch needs-testing has-unit-tests dev-feedback
Focuses: template Cc:

Description (last modified by ocean90)

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.

Attachments (13)

36394.diff (1.5 KB) - added by magicroundabout 8 years ago.
Attempt at patch
test-excerpt.php (331 bytes) - added by magicroundabout 8 years ago.
Sample theme template file for reproducing the issue
36394.2.diff (1.6 KB) - added by magicroundabout 8 years ago.
Corrected patch created from root rather than from wp-includes (sorry - newbie error)
36934.3.patch (2.5 KB) - added by achbed 8 years ago.
Updated patch to push $post parameter through get_the_content
36934.test.patch (486 bytes) - added by achbed 8 years ago.
Proposed unit test
36934.test.2.patch (490 bytes) - added by achbed 8 years ago.
Corrected proposed test
36934.test3.patch (490 bytes) - added by achbed 8 years ago.
Updated patch to apply against revision 37881
36934a.diff (4.5 KB) - added by swissspidy 8 years ago.
36934b.diff (3.1 KB) - added by swissspidy 8 years ago.
36934.diff (12.4 KB) - added by swissspidy 8 years ago.
36934.2.diff (17.0 KB) - added by boonebgorges 8 years ago.
36934.3.diff (17.8 KB) - added by swissspidy 8 years ago.
36934.4.diff (18.9 KB) - added by swissspidy 7 years ago.

Download all attachments as: .zip

Change History (80)

@magicroundabout
8 years ago

Attempt at patch

#1 @magicroundabout
8 years ago

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.

Last edited 8 years ago by magicroundabout (previous) (diff)

@magicroundabout
8 years ago

Sample theme template file for reproducing the issue

@magicroundabout
8 years ago

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

#2 @achbed
8 years ago

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

@achbed
8 years ago

Updated patch to push $post parameter through get_the_content

#3 @achbed
8 years ago

  • Component changed from General to Formatting
  • Keywords has-patch added

#4 @jorbin
8 years ago

  • Description modified (diff)

@achbed
8 years ago

Proposed unit test

#5 @achbed
8 years ago

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
8 years ago

Corrected proposed test

#6 @ocean90
8 years ago

  • Description modified (diff)

#7 @achbed
8 years ago

  • Keywords needs-testing added
  • Version changed from 4.5.2 to 4.5

@achbed
8 years ago

Updated patch to apply against revision 37881

This ticket was mentioned in Slack in #core by achbed. View the logs.


8 years ago

#9 @jorbin
8 years ago

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

#10 @swissspidy
8 years ago

On vacation but will have a look next week.

@swissspidy
8 years ago

@swissspidy
8 years ago

#11 @swissspidy
8 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to 4.6

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.

#12 @magicroundabout
8 years ago

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?

#13 @peterwilsoncc
8 years ago

@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.

#14 @achbed
8 years ago

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?

#15 @magicroundabout
8 years ago

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?

#16 @achbed
8 years ago

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.

#17 @swissspidy
8 years ago

@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.

#18 @magicroundabout
8 years ago

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.

#19 @swissspidy
8 years ago

  • Keywords 4.7-early added
  • Milestone changed from 4.6 to Future Release

This ticket was mentioned in Slack in #core-multisite by achbed. View the logs.


8 years ago

#21 @theMikeD
8 years ago

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

#22 @swissspidy
8 years ago

#37519 was marked as a duplicate.

#23 @swissspidy
8 years ago

  • Milestone changed from Future Release to 4.7

This ticket was mentioned in Slack in #core by helen. View the logs.


8 years ago

#25 @swissspidy
8 years ago

  • Owner set to swissspidy
  • Status changed from new to assigned

@swissspidy
8 years ago

#26 follow-up: @swissspidy
8 years ago

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.

#27 in reply to: ↑ 26 @achbed
8 years ago

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()?

#28 @swissspidy
8 years ago

@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.

This ticket was mentioned in Slack in #core by stevenkword. View the logs.


8 years ago

#30 @boonebgorges
8 years ago

  • Owner changed from swissspidy to boonebgorges
  • Status changed from assigned to reviewing

#31 @boonebgorges
8 years ago

In 38679:

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

See #36934. Fixes #38196.

@boonebgorges
8 years ago

#32 @boonebgorges
8 years ago

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.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


8 years ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


8 years ago

#35 @boonebgorges
8 years ago

  • Keywords has-unit-tests dev-feedback added; needs-unit-tests 4.7-early removed
  • Milestone changed from 4.7 to Future Release
  • Owner changed from boonebgorges to swissspidy

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.

#37 @swissspidy
8 years ago

#12976 was marked as a duplicate.

#38 @swissspidy
8 years ago

#39949 was marked as a duplicate.

@swissspidy
8 years ago

#39 @swissspidy
8 years ago

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;
}
Last edited 8 years ago by swissspidy (previous) (diff)

#40 @achbed
8 years ago

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.

#41 @magicroundabout
8 years ago

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.

#42 @swissspidy
8 years ago

  • Milestone changed from Future Release to 4.8

#43 @swissspidy
7 years ago

  • Milestone changed from 4.8 to 4.8.1

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

#44 @burlingtonbytes
7 years ago

@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.

#45 @boonebgorges
7 years ago

  • Milestone changed from 4.8.1 to 4.9

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.

#46 @swissspidy
7 years ago

@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.

#47 @swissspidy
7 years ago

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

#48 @swissspidy
7 years ago

  • Milestone changed from 4.9 to Future Release

#49 @magicroundabout
7 years ago

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
7 years ago

#50 @swissspidy
7 years ago

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.

#51 @swissspidy
6 years ago

#44741 was marked as a duplicate.

#52 @tofandel
6 years ago

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 );

#53 @swissspidy
6 years ago

@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.

#55 @magicroundabout
6 years ago

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.

#56 @swissspidy
6 years ago

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

#57 @spacedmonkey
6 years ago

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

#58 @swissspidy
6 years ago

  • Component changed from Formatting to Posts, Post Types

#59 @anonymized_11892634
6 years ago

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

#60 @swissspidy
6 years ago

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

#61 @magicroundabout
6 years ago

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! 😃

#62 @boonebgorges
6 years ago

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.

#63 @boonebgorges
6 years ago

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

#64 @swissspidy
6 years ago

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

#65 @spacedmonkey
6 years ago

  • Milestone Future Release deleted
  • Resolution set to duplicate
  • Status changed from reviewing to closed

Issue fixed in #42814

This ticket was mentioned in Slack in #core by boone. View the logs.


6 years ago

#67 @hellofromTonya
2 years ago

#45267 was marked as a duplicate.

Note: See TracTickets for help on using tickets.