_pad_term_count get's stuck if there is a loop in the hierarchy

[westi]

Ideally it should be impossible for a loop in a taxonomy hierarchy to exist.

But sometimes they do.

If one does exist then _pad_term_counts() gets stuck :(

When one of these does exist the category widget breaks the front page of your site.

We currently hook wp_check_term_hierarchy_for_loops on wp_update_term_parent and I wonder if there are other places we should be doing this check too.

[nacin]

What's the goal here for 3.4?

[jane]

Given that we are about to go RC, I'm thinking this should be a 3.5 thing.

[ryan]

Aye. This feels punt to me since it doesn't look like a regression. Bring it back (with patch) if disagreed.

[SergeyBiryukov]

Related: #24461

[wonderboymusic]

fixed whitespace tab thing

[nacin]

How expensive is this? If done on the front page of a site when something is otherwise a read operation, are we looking at a race to run the wp_update_term() to fix the loop in the hierarchy?

westi, what did you mean by "stuck"? Infinite loop? Or something less bad?

[nacin]

Too many unanswered questions here.

[boonebgorges]

Replying to nacin:

How expensive is this? If done on the front page of a site when something is otherwise a read operation, are we looking at a race to run the wp_update_term() to fix the loop in the hierarchy?

westi, what did you mean by "stuck"? Infinite loop? Or something less bad?

Appears to be an infinite while loop.

wp_check_term_hierarchy_for_loops() is more than we need in this case. (a) It's probably overly expensive to break the hierarchy on a read operation like get_terms(), when all we really need here is to ensure that we don't loop infinitely; and (b) there's no need to recurse the hierarchy using get_term() (wp_get_term_taxonomy_parent_id()) because _pad_term_counts() does a single query to grab the whole hierarchy - that is, we already have all the data we need to detect a loop.

I propose that we keep track of already-identified $ancestors during the while loop, and just break out of it if we detect a loop. Worst case scenario with this fix is that, if the loop comprises only a subset of the terms identified in the get_terms() query, it's possible that some terms won't have their counts padded. See 20635.3.2.diff​ (note that the unit test won't outright fail without the fix, but you'll probably get a maximum execution time fatal error).

[boonebgorges]

add term hierarchy loop detection to _pad_term_counts()

[boonebgorges]

In 31206:

Bail out of hierarchy loops in _pad_term_counts().

Taxonomy hierarchy loops should not occur naturally, but when they do, the
logic of _pad_term_counts() could result in infinite loops, leading to
timeouts. We avoid this by breaking when a loop is detected.

Fixes #20635.

[boonebgorges]

[31206] breaks out of loops one level too early, so that padded counts are coming out as one less than they ought to be. Fix and amended tests are coming up.

[boonebgorges]

In 31248:

Better loop detection for _pad_term_counts().

The $ancestors check must be reset for each term in order for term counts
to be correct.

Fixes #20635.