WordPress.org

Make WordPress Core

Opened 19 months ago

Last modified 3 months ago

#36934 reviewing defect (bug)

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

Reported by: magicroundabout Owned by: swissspidy
Milestone: Future Release Priority: normal
Severity: normal Version: 4.5
Component: Formatting 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 (12)

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

Download all attachments as: .zip

Change History (60)

@magicroundabout
19 months ago

Attempt at patch

#1 @magicroundabout
19 months 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 19 months ago by magicroundabout (previous) (diff)

@magicroundabout
19 months ago

Sample theme template file for reproducing the issue

@magicroundabout
19 months ago

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

#2 @achbed
19 months ago

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

@achbed
19 months ago

Updated patch to push $post parameter through get_the_content

#3 @achbed
19 months ago

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

#4 @jorbin
19 months ago

  • Description modified (diff)

@achbed
19 months ago

Proposed unit test

#5 @achbed
19 months 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
19 months ago

Corrected proposed test

#6 @ocean90
19 months ago

  • Description modified (diff)

#7 @achbed
18 months ago

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

@achbed
18 months ago

Updated patch to apply against revision 37881

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


18 months ago

#9 @jorbin
18 months ago

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

#10 @swissspidy
18 months ago

On vacation but will have a look next week.

@swissspidy
18 months ago

@swissspidy
18 months ago

#11 @swissspidy
18 months 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
17 months 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
17 months 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
17 months 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
17 months 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
17 months 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
17 months 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
17 months 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
17 months 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.


17 months ago

#21 @theMikeD
17 months ago

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

#22 @swissspidy
17 months ago

#37519 was marked as a duplicate.

#23 @swissspidy
16 months ago

  • Milestone changed from Future Release to 4.7

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


16 months ago

#25 @swissspidy
16 months ago

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

@swissspidy
15 months ago

#26 follow-up: @swissspidy
15 months 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
15 months 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
15 months 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.


15 months ago

#30 @boonebgorges
15 months ago

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

#31 @boonebgorges
15 months ago

In 38679:

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

See #36934. Fixes #38196.

#32 @boonebgorges
15 months 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.


14 months ago

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


14 months ago

#35 @boonebgorges
14 months 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
10 months ago

#12976 was marked as a duplicate.

#38 @swissspidy
10 months ago

#39949 was marked as a duplicate.

@swissspidy
10 months ago

#39 @swissspidy
10 months 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 10 months ago by swissspidy (previous) (diff)

#40 @achbed
10 months 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
9 months 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 months ago

  • Milestone changed from Future Release to 4.8

#43 @swissspidy
7 months 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
6 months 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
5 months 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
5 months 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
5 months 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
3 months ago

  • Milestone changed from 4.9 to Future Release
Note: See TracTickets for help on using tickets.