WordPress.org

Make WordPress Core

#39038 closed defect (bug) (wontfix)

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

Reported by: dd32 Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Taxonomy Keywords: has-patch needs-unit-tests
Focuses: Cc:

Description

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.

Attachments (1)

35614.diff (720 bytes) - added by dd32 19 months ago.
an idea @nacin was playing with on https://core.trac.wordpress.org/attachment/ticket/35614/35614.diff

Download all attachments as: .zip

Change History (17)

#1 @johnbillion
19 months ago

  • Summary changed from Pass the taxonomy context to singular term capabilities to Add support for non-split shared terms to singular term capabilities

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

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?

#2 @dd32
19 months ago

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.

#3 follow-up: @theMikeD
19 months ago

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?

Last edited 19 months ago by theMikeD (previous) (diff)

#4 in reply to: ↑ 3 @dd32
19 months ago

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

#5 @theMikeD
19 months ago

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?

#6 @johnbillion
19 months ago

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.

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


19 months ago

#8 @boonebgorges
19 months ago

  • Keywords needs-patch added

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().)

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


18 months ago

#10 @dd32
18 months ago

  • Milestone changed from 4.7.1 to 4.7.2

Bumping to 4.7.2 pending patch.

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


16 months ago

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


16 months ago

#13 @jnylen0
16 months ago

  • Milestone changed from 4.7.3 to 4.7.4

Bump.

#14 @theMikeD
16 months ago

  • Resolution set to invalid
  • Status changed from new to closed

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.

Last edited 16 months ago by theMikeD (previous) (diff)

#15 @theMikeD
16 months ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

#16 @johnbillion
16 months ago

  • Keywords has-patch needs-unit-tests added; needs-patch removed
  • Milestone 4.7.4 deleted
  • Resolution set to wontfix
  • Status changed from reopened to closed

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.

Note: See TracTickets for help on using tickets.