Make WordPress Core

Opened 9 months ago

Closed 6 months ago

Last modified 6 months ago

#57923 closed defect (bug) (fixed)

Deprecated notice fired by wp_set_object_terms() with PHP 8.1

Reported by: chouby's profile Chouby Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.3 Priority: normal
Severity: normal Version:
Component: Taxonomy Keywords: php81 has-patch has-unit-tests commit
Focuses: Cc:

Description

It is common practice to pass null as second parameter of wp_set_object_terms() to remove all terms.

In such case, PHP throws this notice:

Deprecated: trim(): Passing null to parameter #1 ($string) of type string is deprecated in /app/polylang/tmp/wordpress/wp-includes/taxonomy.php on line 2751

Of course, this can be fixed by replacing null by array() as second param but I wonder if WordPress should not protect itself from this notice by replacing any empty $terms by array() before singular values are converted to an array.

Attachments (1)

57923.diff (1.2 KB) - added by SergeyBiryukov 9 months ago.

Download all attachments as: .zip

Change History (23)

#1 @audrasjb
9 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 6.3

Of course, this can be fixed by replacing null by array() as second param but I wonder if WordPress should not protect itself from this notice by replacing any empty $terms by array()

Agreed. Moving this for 6.3 consideration.

#2 follow-up: @costdev
9 months ago

replacing any empty $terms by array()

This makes sense. The docs for the $terms parameter state:

Passing an empty value will remove all related terms.

A thought: While this is documented as string|int|array, we might consider adding null to this list so that it's documented and our future selves know to protect BC for null. Not sure about that, but curious what others think.

#3 in reply to: ↑ description @SergeyBiryukov
9 months ago

  • Keywords 2nd-opinion added

Hi there, thanks for the ticket!

Replying to Chouby:

It is common practice to pass null as second parameter of wp_set_object_terms() to remove all terms.

Hmm, the $terms parameter is documented as string|int|array though, so as you've already noted, it seems like passing an empty array would be the correct way to remove all terms, and passing null is incorrect.

I think the notice is expected here, putting this guard in place to avoid a PHP notice when it's null is really just papering over the problem instead of fixing the root cause.

We could consider a _doing_it_wrong() notice here if $terms is not of the three supported types, however, as previously noted in comment:3:ticket:57580, ideally we should have a more structural plan on whether and how to validate the type of received parameters in core functions.

#4 in reply to: ↑ 2 @SergeyBiryukov
9 months ago

Replying to costdev:

The docs for the $terms parameter state:

Passing an empty value will remove all related terms.

We should probably improve that bit for clarity:

Passing an empty array will remove all related terms.

Last edited 9 months ago by SergeyBiryukov (previous) (diff)

#5 @audrasjb
9 months ago

Updating the docblock parameter would at least be a nice starting point 👍

#6 follow-up: @costdev
9 months ago

See: https://developer.wordpress.org/reference/functions/wp_set_object_terms/#more-information

For return:
(array) An empty array if the $terms argument was NULL or empty – successmessage for the removing of the term

So while the type isn't documented, the idea of passing $terms as null is mentioned here. 🤔

Question is, does "empty" refer to empty( $terms ) === true, or does it refer to null, '', and array()? 🤔

Last edited 9 months ago by costdev (previous) (diff)

#7 @audrasjb
9 months ago

It looks like this "More information" section was imported from the old Codex (which is fine). We can of course update this section, but I would recommend to also update the docblock, for people using other sources (like other websites crawling core docblocks, or for people reading core itself) :)

#8 @jrf
9 months ago

I agree with @SergeyBiryukov's comment https://core.trac.wordpress.org/ticket/57923#comment:3 - we should not be hiding errors which should be fixed instead.

#9 in reply to: ↑ 6 @peterwilsoncc
9 months ago

Replying to costdev:

Question is, does "empty" refer to empty( $terms ) === true, or does it refer to null, '', and array()?

I'm not sure what was meant back when when the docblock was written, but I think it's a reasonable interpretation that it means empty( $terms ) === true.

I do think that's how it has been interpreted in the official documentation that Colin refers to.

#10 @Chouby
9 months ago

I also noticed that the doc block doesn't explicitely accept null as parameter and that the parameter comment was ambiguous with this "empty value" which could in fact be interpreted as accepting null.

As a plugin developer, I don't believe that adding a _doing_it_wrong() would be helpful in this case. This just would add a notice on top of another notice.

In my opinion, there are only 2 alternatives. Either WordPress officially accepts null by updating the code and doc block. Or it is considered useless to officially accept it and the small adjustement in the doc block suggested by @costdev would look perfect to me.

For the records:

#11 @costdev
9 months ago

Thanks for the extra information @Chouby!

Given that:

  • The documentation in "More Information" explicitly states that null can be passed.
  • At least 74 that pass null for $terms, and the documentation currently mentions passing null.

I think that officially accepting null is the backward compatible approach here. The documentation, and loose checking, has somewhat backed us into a corner here.

Based on this, I think we have a few options:

  1. Implicitly accept null, by:
    1. Adding null !== $terms to the condition here.
      1. This would avoid the deprecation notice, as the foreach() uses (array) $terms, converting null to array() just for the foreach() loop.
  2. Or, explicitly accept null, by:
    1. Adding null to the list of types for $terms.
    2. Adding if ( empty( $terms ) ) { $terms = array(); } before line 2728.
      1. This would cover more values than null but, as far I know, none of those values are valid Term IDs/slugs, and it would be consistent with the current documentation: "Passing an empty value will remove all related terms."
  3. Or, a mixture of both:
    1. Adding null to the list of types for $terms.
    2. Adding null !== $terms to the condition here.
      1. This would prevent the deprecation notice.
      2. This would prevent changing the value of $terms passed by the set_object_terms action from array( null ) to array().
      3. This would mean a more accurate docblock for the function.
Version 6, edited 9 months ago by costdev (previous) (next) (diff)

#12 @SergeyBiryukov
9 months ago

  • Keywords has-patch added; needs-patch removed

Given that the "More information" section (at least in this case) is not the official DocBlock, but some user-editable notes previously imported from Codex which may or may not be up-to-date or fully accurate, I would suggest not to add null as an explicitly supported type, as I don't see a strong reason for that, the three existing types should be enough for any use case.

That said, adding the if ( empty( $terms ) ) { $terms = array(); } check would make the logic more consistent with wp_set_post_terms(), while also resolving the issue by supporting null implicitly.

So I think I would prefer something like 57923.diff here:

  • Make wp_set_object_terms() and wp_set_post_terms() handle null consistently. Even if it's not technically a supported type, it's a reasonable value to account for in this case.
  • Clarify the DocBlock to mention passing an empty array as the recommended way to remove all related terms, rather than an ambiguous empty value.
Last edited 9 months ago by SergeyBiryukov (previous) (diff)

#13 @costdev
9 months ago

With 57923.diff, should there be any concern that the set_object_terms action will now pass $terms as array() rather than array( null )?

If not, then 57923.diff looks good to me. Could do with a PHPUnit test to protect the behaviour when null is passed.

#14 @peterwilsoncc
9 months ago

57923.diff LGTM with the addition of some tests. As a nit, it could do with an elseif( ! is_array() )

This ticket was mentioned in PR #4603 on WordPress/wordpress-develop by @peterwilsoncc.


6 months ago
#15

  • Keywords has-unit-tests added

Prevents the throwing of a notice in wp_set_object_terms().

src changes props @SergeyBiryukov

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

#16 @peterwilsoncc
6 months ago

In the linked pull request:

  • changes from 57923.diff
  • added some tests to ensure empty values clear the terms
  • set up some terms to use shared fixtures, needed to do a little refactoring to convert the taxonomy property to a static

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


6 months ago

#18 @prashantbhivsane
6 months ago

  • Keywords needs-testing added; 2nd-opinion removed

This ticket was discussed during the bug scrub.

@peterwilsoncc has added a patch that needs to be tested.

Last edited 6 months ago by prashantbhivsane (previous) (diff)

@peterwilsoncc commented on PR #4603:


6 months ago
#19

Thanks @costdev , I've applied each of your suggestions.

#20 @costdev
6 months ago

  • Keywords commit added; needs-testing removed

All feedback on the PR has been addressed and an additional dataset has also been added to the tests. The tests achieve full line/branch coverage of the changes, and test all of PHP's possibly empty() values.

This one looks ready for commit to me.


Note: Removing needs-testing as I believe the PHPUnit tests cover this change.

#21 @peterwilsoncc
6 months ago

  • Owner set to peterwilsoncc
  • Resolution set to fixed
  • Status changed from new to closed

In 55921:

Taxonomy: Prevent deprecation notices clearing terms.

Prevents wp_set_object_terms() throwing a deprecation notice in PHP 8.1+ when passing an empty value as the second parameter to clear the terms.

Props audrasjb, chouby, costdev, jrf, peterwilsoncc, prashantbhivsane, sergeybiryukov.
Fixes #57923.

Note: See TracTickets for help on using tickets.