Don't return if WP_Error object return by wp_insert_term() from foreach() loop in wp_set_object_terms()

[chandrapatel]

Recently, I'm trying to restrict tag creation for some user roles. I tried with using roles and capabilities but it's not possible. So I hook function to pre_insert_term filter which returns WP_Error object if condition match.

add_action( 'pre_insert_term', 'prevent_terms', 1, 2 );

function prevent_terms ( $term, $taxonomy ) {

	if ( 'post_tag' !== $taxonomy ) {
		return $term;
	}
        
         // Only administrator can create tag.
	$allowed_roles = array( 'administrator' );

	$user = wp_get_current_user();

	if ( ! empty( $user->roles ) && empty( array_intersect( $allowed_roles, $user->roles ) ) ) {

		return new WP_Error( 'term_addition_blocked', 'You are not allowed to create new tags.' );

	}

	return $term;

}

Now, a restricted user goes to the edit post and trying to assign existing tags to the post and its work fine. But when a user adds a tag which does not exist along with existing tags then existing tags also not assigned to that post. Because wp_set_object_terms() function will create new tag if it's not exists and I've return WP_Error object using pre_insert_term hook. So once it get WP_Error object then it returns from the wp_set_object_terms() function and it's also not execute for other terms.

See https://core.trac.wordpress.org/browser/tags/4.8/src/wp-includes/taxonomy.php#L2237

To complete my functionality I hook into admin_init action and remove new tags from $_POST so that remaining existing tags can assign to the post.

I believe, instead of returning, it should continue execution for other terms. Also, I'm not sure whether it's valid use case or not.

So main question is, do we need to return if there is WP_Error while creating term or don't return and continue execution for other terms.

I hope, I'm able to explain it properly. :)

[chandrapatel]

Initial patch

[boonebgorges]

Hi @chandrapatel - Thanks for the detailed description of the issue. I agree that the error handling in wp_set_object_terms() is less than ideal. The function should either: (a) skip failures but process all other terms, or (b) bail from the entire function if there are any errors. Currently it does a mix of the two.

I think that it's important to retain the WP_Error. Silently skipping failed terms, without providing feedback to the developer, makes it potentially difficult to debug problems. This is especially true since we *currently* return an error object; plugins may be expecting it.

We currently don't have a great way of handling return values in mixed-success situations. We can only return one thing: in this case, it's either an array of $tt_ids or a WP_Error object. If we allowed the function to process partially, it's not clear to me how we would provide this information to developers. I think this is probably the main reason why we ought to keep something like the current (imperfect) behavior. Do you have ideas about how this error reporting can be improved without losing important feedback for developers?

[pbiron]

Replying to boonebgorges:

We currently don't have a great way of handling return values in mixed-success situations. We can only return one thing: in this case, it's either an array of $tt_ids or a WP_Error object. If we allowed the function to process partially, it's not clear to me how we would provide this information to developers. I think this is probably the main reason why we ought to keep something like the current (imperfect) behavior. Do you have ideas about how this error reporting can be improved without losing important feedback for developers?

I just bumped into this problem today, altho for a slightly different reason than the OP.

In my case, term_exists() ( on line 2286) was returning false but the wp_insert_term() (on line 2290) was returning a WP_Error that the term already existed...because the new $term->name was getting "santizied" into a string that was the same as an existing $term->name (due to illegal UTF-8 encoding of the string).

The following is just off the top of my head (i.e., I have't thought thru all of the implications), but...

Since a single WP_Error object can contain more than 1 error (via WP_Error:add()), how about WP_Error::add()'ing error that occurs in the loop...and then doing 1 final WP_Error::add() with the $tt_ids?

[boonebgorges]

@pbiron This is a good thought.

It's worth noting that WP_Error objects only allow a single instance of an error code. So if multiple terms in the loop return the same error code, they'll overwrite the previous error codes. Instead, we might need something that intelligently assembles these codes. For example: if more than one call to wp_insert_term() returns an 'invalid_term_id' error, we will need to assemble all the term data into an array, add add_data() all of those to the wp_set_object_terms() error object under the 'invalid_term_id' slot in the error object. So this will take some retooling of the way the $terms loop works.

[pbiron]

So this will take some retooling of the way the $terms loop works.

Agreed.

Also, I think the hardest part might be writing good documentation so that callers understand that just because wp_set_object_terms() returns a WP_Error does NOT mean that NO terms were set.

Unfortunately, I don't have the bandwidth at the moment to work on this :-(

[chandrapatel]

Hello @boonebgorges and @pbiron

As per your suggestion, I've tried following solution.

We can add multiple messages for the same error code. So we can use that instead of add_data(). Also, we can prepend term name to the error message so that developer can know which term get which error. We can use add_data() to add $tt_ids if few terms set to object successfully. See the following example.

WP_Error Object
(
    [errors] => Array
        (
            [term_addition_blocked] => Array
                (
                    [0] => Tag 3: You are not allowed to create new tags.
                    [1] => Tag 4: You are not allowed to create new tags.
                )

        )

    [error_data] => Array
        (
            [affected_terms_tt_ids] => Array
                (
                    [0] => 2
                    [1] => 3
                )

        )

)

If there are no errors then only $tt_ids will return.

Array
(
    [0] => 2
    [1] => 3
)

Also, return types are same array|WP_Error which don't break the error handling code. I don't know other use cases for backward compatibility.

Please let me know your thoughts.

[chandrapatel]

Added patch as per the solution I've thought in comment #6