WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 4 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 21 months ago.
20635.3.diff (2.6 KB) - added by boonebgorges 4 months ago.
20635.diff (2.1 KB) - added by boonebgorges 4 months ago.
add term hierarchy loop detection to _pad_term_counts()
20635.3.2.diff (2.1 KB) - added by boonebgorges 4 months ago.

Download all attachments as: .zip

Change History (18)

comment:1 @nacin3 years ago

What's the goal here for 3.4?

comment:2 @jane3 years ago

  • Keywords reporter-feedback added

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

comment:3 @ryan3 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.

comment:4 @westi3 years ago

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

comment:5 @jkudish2 years ago

  • Cc jkudish added

comment:6 @jkudish2 years ago

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

@wonderboymusic21 months ago

comment:8 @wonderboymusic21 months ago

  • Milestone changed from Future Release to 3.7

fixed whitespace tab thing

comment:9 follow-up: @nacin20 months 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?

comment:10 @nacin19 months ago

  • Milestone changed from 3.7 to Future Release

Too many unanswered questions here.

comment:11 in reply to: ↑ 9 @boonebgorges4 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 4 months ago by boonebgorges (previous) (diff)

@boonebgorges4 months ago

@boonebgorges4 months ago

add term hierarchy loop detection to _pad_term_counts()

@boonebgorges4 months ago

comment:12 @boonebgorges4 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.

comment:13 @boonebgorges4 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.

comment:14 @boonebgorges4 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.