close

Make WordPress Core

Opened 5 years ago

Last modified 3 months ago

#52820 reviewing defect (bug)

Possible logic error in wp_trim_excerpt()

Reported by: themiked's profile theMikeD Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Formatting Keywords: has-patch has-unit-tests 2nd-opinion needs-refresh
Focuses: Cc:

Description

https://core.trac.wordpress.org/browser/tags/5.7/src/wp-includes/formatting.php#L3845

Reading this code, it looks like the wp_trim_excerpt filter's $raw_excerpt parameter will be empty if $text as supplied to the function is empty. This should be the value of $text at L3824 should it not? Or do I mis-understand?

Change History (14)

Image

This ticket was mentioned in PR #1126 on WordPress/wordpress-develop by donmhico.


5 years ago
#1

  • Keywords has-patch has-unit-tests added

This PR assigns the correct value for $raw_excerpt if the passed $text is empty. Right now $raw_excerpt is empty if $text is empty.

Trac ticket: https://core.trac.wordpress.org/ticket/52820

#2 Image @donmhico
5 years ago

Hello @theMikeD. Thanks for the ticket. Yes, I believe you're correct. $raw_excerpt passed to wp_trim_excerpt filter is empty if $text is empty. In my PR above, I placed $raw_excerpt = $text; below the apply_filter( 'the_content' ) because I still believe that $raw_excerpt should be processed by that filter. Others can chime in their thoughts regarding this.

#3 Image @SergeyBiryukov
5 years ago

  • Component changed from General to Posts, Post Types
  • Milestone changed from Awaiting Review to 5.8
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#4 Image @hellofromTonya
5 years ago

  • Milestone changed from 5.8 to 5.9

Today is 5.8 Beta 1. Ran out of time for this one to land. Punting to 5.9.

Image

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


4 years ago

#6 Image @audrasjb
4 years ago

  • Milestone changed from 5.9 to 6.0

We have some conflicts to resolve in the above PR. As today is 5.9 beta 1, I think it's safer to move this ticket to 6.0 to ensure it's properly reviewed and fixed.

Image

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


4 years ago

#9 in reply to: ↑ description ; follow-up: Image @jrf
4 years ago

Replying to theMikeD:

https://core.trac.wordpress.org/browser/tags/5.7/src/wp-includes/formatting.php#L3845

Reading this code, it looks like the wp_trim_excerpt filter's $raw_excerpt parameter will be empty if $text as supplied to the function is empty. This should be the value of $text at L3824 should it not? Or do I mis-understand?

I do agree there may be a logic error in this function, but I don't necessarily think it's in the filter. Maybe we should have good look at what this function is supposed to do ?

From the docblock:

3798 	 * Generates an excerpt from the content, if needed.
3799	 *
3800	 * Returns a maximum of 55 words with an ellipsis appended if necessary.
...
3808	 * @param string             $text Optional. The excerpt. If set to empty, an excerpt is generated.
3809	 * @param WP_Post|object|int $post Optional. WP_Post instance or Post ID/object. Default null.
3810	 * @return string The excerpt.

Looking at the code, the excerpt is only trimmed to 55 words when it has been auto-generated, not when it was provided via the $text parameter.

Is this really the intended behaviour ?

#10 Image @costdev
4 years ago

  • Keywords reporter-feedback added

As this is still being discussed and we're approaching RC1, I'm going to move this ticket to Future Release.

#11 Image @costdev
4 years ago

  • Milestone changed from 6.0 to Future Release

Image

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


3 months ago

#13 in reply to: ↑ 9 Image @SirLouen
3 months ago

Replying to jrf:

Looking at the code, the excerpt is only trimmed to 55 words when it has been auto-generated, not when it was provided via the $text parameter.

Is this really the intended behaviour ?

It appears that this concern was not addressed. I'm adding 2nd-opinion for this as it seems it's not a reporter trouble.

#14 Image @SirLouen
3 months ago

  • Component changed from Posts, Post Types to Formatting
  • Keywords 2nd-opinion needs-refresh added; reporter-feedback removed

Also last patch in PR2541 needs refresh.

Note: See TracTickets for help on using tickets.