WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 16 months ago

#20635 closed defect (bug) (fixed)

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

Reported by: westi Owned by: westi
Milestone: 4.2 Priority: normal
Severity: normal Version:
Component: Taxonomy Keywords: westi-likes has-patch 2nd-opinion
Focuses: Cc:

Description

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.

Attachments (4)

20635.2.diff (1.4 KB) - added by wonderboymusic 3 years ago.
20635.3.diff (2.6 KB) - added by boonebgorges 16 months ago.
20635.diff (2.1 KB) - added by boonebgorges 16 months ago.
add term hierarchy loop detection to _pad_term_counts()
20635.3.2.diff (2.1 KB) - added by boonebgorges 16 months ago.

Download all attachments as: .zip

Change History (18)

#1 @nacin
4 years ago

What's the goal here for 3.4?

#2 @jane
4 years ago

  • Keywords reporter-feedback added

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

#3 @ryan
4 years ago

  • Milestone changed from 3.4 to Future Release

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

#4 @westi
4 years ago

  • Keywords westi-likes 3.5-early added; reporter-feedback removed
  • Owner set to westi
  • Status changed from new to accepted

#5 @jkudish
3 years ago

  • Cc jkudish added

#6 @jkudish
3 years ago

  • Keywords has-patch 3.6-early added; needs-patch 3.5-early removed

#8 @wonderboymusic
3 years ago

  • Milestone changed from Future Release to 3.7

fixed whitespace tab thing

#9 follow-up: @nacin
3 years ago

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?

#10 @nacin
3 years ago

  • Milestone changed from 3.7 to Future Release

Too many unanswered questions here.

#11 in reply to: ↑ 9 @boonebgorges
16 months ago

  • Keywords 2nd-opinion added; 3.6-early removed
  • Milestone changed from Future Release to 4.2

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

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

@boonebgorges
16 months ago

add term hierarchy loop detection to _pad_term_counts()

#12 @boonebgorges
16 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

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.

#13 @boonebgorges
16 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

[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.

#14 @boonebgorges
16 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

Note: See TracTickets for help on using tickets.