WordPress.org

Make WordPress Core

Opened 3 months ago

Closed 3 months ago

#47867 closed defect (bug) (fixed)

wp_trim_excerpt and wp_trim_words don't validate the excerpt length (int)

Reported by: pikamander2 Owned by: SergeyBiryukov
Milestone: 5.3 Priority: normal
Severity: normal Version: 5.2.2
Component: Posts, Post Types Keywords: has-patch has-unit-tests
Focuses: Cc:
PR Number:

Description

We noticed that our error log was filled with messages like this:

PHP message: PHP Warning: A non-numeric value encountered in [...]/public_html/wp-includes/formatting.php on line 3770

The immediate culprit turned out to be Elementor Pro, which allows the user to input an excerpt length for its "Cards" element, but doesn't validate the value at all, so if the user's input is blank then it will set the excerpt length to a blank string value rather than 0. It does so via the excerpt_length filter.

I've submitted the fix to their support, but shouldn't WordPress's core be validating that value as well? I can't think of any possible reason why you would want WordPress to attempt to use a non-numeric value in those functions.

Here are the relevant lines in wp-includes\formatting.php that prompt the warning:

$excerpt_more = apply_filters( 'excerpt_more', ' ' . '[…]' );

$text         = wp_trim_words( $text, $excerpt_length, $excerpt_more );

//if...
$words_array = array_slice( $words_array[0], 0, $num_words + 1 );

//else...
$words_array = preg_split( "/[\n\r\t ]+/", $text, $num_words + 1, PREG_SPLIT_NO_EMPTY );

Basically, the WordPress core sets the default excerpt length to 55, then applies the filters, then doesn't check the resulting value to make sure that it's valid.

I think that the sanitization logic should look something like this:

  • If the resulting value is neither and integer nor a float, replace it with 0.
  • Else if the resulting value is a float, cast it to an integer
  • Else use the value without any modification (since it's an integer and therefore valid)

Assuming that that all sounds good, I'm not sure whether it would be more appropriate to put the check at the end of wp_trim_excerpt or at the start of wp_trim_words (or both?).

Attachments (2)

47867.diff (2.5 KB) - added by donmhico 3 months ago.
Patch + unit tests.
47867.2.diff (2.2 KB) - added by donmhico 3 months ago.
Set $num_words to 0 if non-numeric argument is passed.

Download all attachments as: .zip

Change History (8)

@donmhico
3 months ago

Patch + unit tests.

#1 @donmhico
3 months ago

  • Keywords has-patch has-unit-tests added

@pikamander2 - Welcome in the trac. Congratulations and thank you for your very first ticket.

I've attached a patched - 47867.diff - that should fix your concern. However instead of using 0 as length for non-numeric $num_words passed to wp_trim_words() (e.g. ' ', 'abc'), I think it's more reasonable to use the default 55 length.

Also, I didn't apply the fix in wp_trim_excerpt() since it's ultimately uses wp_trim_words().

#2 @pikamander2
3 months ago

Hi @donmhico, thanks for the quick fix.

My main concern with defaulting the value to 55 is that it changes a longstanding behavior. Any plugins or themes that are passing "" or null to that function are currently receiving zero words, but once the patch is applied they'll be receiving 55 words.

In the case of Elementor Pro, for example, many users' Post Cards will suddenly change from this:

https://i.imgur.com/qljC2Ec.png

to this:

https://i.imgur.com/SNnqn5g.png

And because it's an obscure issue, they're gonna have no idea what caused it or how to fix it. That's going to lead to a lot of frustration.

Granted, that's not necessarily WordPress's problem, and it might be better to just go ahead and "rip off the band-aid" so to speak, but it's definitely something to consider.

#3 @donmhico
3 months ago

Hey @pikamander2, I see your point. I supposed that if existing plugins / themes do want to use the default 55, they should have not passed anything as $num_words instead of passing empty string or null. And WP.org stays true to provide backward compatibility and not breaking any existing behaviour as much as possible.

I'll upload an updated patch shortly.

@donmhico
3 months ago

Set $num_words to 0 if non-numeric argument is passed.

#4 @SergeyBiryukov
3 months ago

  • Component changed from General to Posts, Post Types

#5 @SergeyBiryukov
3 months ago

  • Milestone changed from Awaiting Review to 5.3
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#6 @SergeyBiryukov
3 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 45796:

Posts, Post Types: In wp_trim_words() make sure the $num_words parameter is always an integer, as documented, to avoid a PHP warning.

Props donmhico, pikamander2.
Fixes #47867.

Note: See TracTickets for help on using tickets.