Add support for non-split shared terms to singular term capabilities

[dd32]

As reported in #35614 by @kevinB, the edit_term, delete_term and assign_term singular capabilities only pass the term_id and not the taxonomy. For compat with installations where shared terms aren't split, we should specify the taxonomy, or trigger splitting.

As implemented in 4.7 RC, these meta capability checks accept a term_id with no taxonomy specified. This is an issue for legacy installations that have term_id != term_taxonomy_id. Taxonomy determination is now slated to fall to WP_Term::get_instance(), which will try to disambiguate but may return WP_Error.
An obvious option would to require the additional taxonomy argument following term_id. This would cause a fourth item in the $args array passed into the 'map_meta_cap' filter - a break with tradition, but not a problem for API-compliant plugins in my view.

[dd32]

an idea @nacin was playing with on https://core.trac.wordpress.org/attachment/ticket/35614/35614.diff

[johnbillion]

There are a couple of other places in core where the optional $taxonomy parameter isn't passed to get_term():

• https://core.trac.wordpress.org/browser/trunk/src/wp-admin/edit-tags.php?rev=39308#L138
• https://core.trac.wordpress.org/browser/trunk/src/wp-includes/capabilities.php?rev=39308#L277

Do these situations need to handle non-split shared terms too? If so, do we need a generic handler? Or can we assume that all calls to get_term() cannot be considered reliable for non-split shared terms when the $taxonomy parameter isn't specified?

[dd32]

Or can we assume that all calls to get_term() cannot be considered reliable for non-split shared terms when the $taxonomy parameter isn't specified?

Exactly that. When terms are not split, calling get_term() without a $taxonomy param will fail hard when you attempt to fetch it.

I'm almost fine with saying that terms should be split at this point, I'm not sure how far ahead we can support them not being split.
One option that @nacin suggested, was when we encounter a non-split term in an auth-situation was to cause it to be split.. something that shouldn't be hit very often.

[theMikeD]

I thought term splitting was automatic. Is there an actual case when someone would be using 4.7 and not have had term splitting performed?

[dd32]

Replying to theMikeD:

I thought term splitting was automatic. Is there an actual case when someone would be using 4.7 and not have had term splitting performed?

Yep. Splitting is automatic, but it may be disabled, have failed, or run into a race condition

[theMikeD]

OK...but if we have to assume that term splitting failed when we do stuff like this, that seems to be the same thing as assuming code splitting didn't happen, which begs the question...why was it done in the first place?

[johnbillion]

Agreed. The $taxonomy parameter was made optional in [34997] as part of the term splitting work. If we can't guarantee that shared terms are split, then this parameter should not be optional.

[boonebgorges]

I'm with @dd32 that we are probably far enough along that we can assume, throughout most of the taxonomy API, that terms have been split. Auth checks are a case where we should be extra careful, but we're erring on the side of caution here (shared terms will get 'do_not_allow').

Something like 35614.diff​ seems OK, but it could result in weird behavior due to latency or race conditions when the same cap-check happens more than once on a page: the first time it could fail, triggering a split, while on a subsequent call it might succeed. This could have consequences that are difficult to predict, though it'd only be a one-time occurrence for a given term, so maybe it's an OK consequence.

(If we want to go this direction, the patch will need to be written in a way that fetches all terms sharing the term_id, so that we have a tt_id to pass to _split_shared_term().)

[dd32]

Bumping to 4.7.2 pending patch.

[jnylen0]

Bump.

[theMikeD]

IMHO this is a not a defect and should not be classified as a bug. Further, since this is working as designed, it's not even a valid request and should closed as invalid.

ETA: Oops, did not mean to set those, i was using the dropdown to see what the correct terms are. I'll change it back.

[johnbillion]

As per my previous comments, if core cannot assume that terms have been split then the $taxonomy parameter for term functions should not be optional. Thus, this is invalid unless the assumption of split terms needs to be revisited.