Deprecated notice fired by wp_set_object_terms() with PHP 8.1

[Chouby]

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.

[audrasjb]

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.

[costdev]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

[audrasjb]

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

[costdev]

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

[audrasjb]

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

[jrf]

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.

[peterwilsoncc]

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.

[Chouby]

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:

• I noticed that `wp_set_post_terms()` accepts null even if not documented.
• And there are ​at least 74 plugins which still use null to remove terms from an object.

[costdev]

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)
      2. This would prevent changing the value of $terms passed by the set_object_terms action from array( null ) to array().
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."
      2. This would change the value of $terms passed by the set_object_terms action from array( null ) to array().
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.

[SergeyBiryukov]

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.

[costdev]

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.

[peterwilsoncc]

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

Prevents the throwing of a notice in wp_set_object_terms().

src changes props @SergeyBiryukov

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

[peterwilsoncc]

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

[prashantbhivsane]

This ticket was discussed during the bug scrub.

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

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

[costdev]

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.

[peterwilsoncc]

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.

Merged in r55921 / ​https://github.com/WordPress/wordpress-develop/commit/6178829ab0594d3a541c406502ba31e05ddd82f3