WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#29614 closed defect (bug) (fixed)

wp_update_term() shouldn't allow a term to be assigned to a missing parent

Reported by: danielbachhuber Owned by: boonebgorges
Milestone: 4.1 Priority: normal
Severity: normal Version:
Component: Taxonomy Keywords: has-patch
Focuses: Cc:

Description

Easiest to mimic the new behavior established in #19205

Discovered in https://github.com/wp-cli/wp-cli/pull/1384

Attachments (2)

29614.diff (1.3 KB) - added by jesin 5 years ago.
Patch and unit test
29614.2.diff (1.9 KB) - added by jesin 5 years ago.
Includes 29614.diff and adds more assertions

Download all attachments as: .zip

Change History (10)

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.


5 years ago

@jesin
5 years ago

Patch and unit test

#2 @jesin
5 years ago

Patch 29614.diff includes a unit test that uses wp_update_term(). Does $this->factory->category->create() have a replacement for update?

#3 @nacin
5 years ago

The unit test factory is for initializing objects that may be needed during the test. An actual update should use wp_update_term().

The test should probably also assert first that changing its parent to a term that exists also works.

Also, we should probably assert that the category's parent changed to 0 after deletion, and that we're then trying to change it back.

@jesin
5 years ago

Includes 29614.diff and adds more assertions

#4 @jesin
5 years ago

Patch 29614.2.diff includes assertions for change to existing parent and deletion of a parent.

Without the patch the third assertion fails.

The term cache had to be cleared with clean_term_cache(). Do we have a method of getting terms without caching them?

#5 follow-up: @wonderboymusic
5 years ago

  • Keywords has-patch added; needs-patch removed

The only terms that get cleaned are those that are attached to posts <---- that is a bug. You should never have to call clean_term_cache() directly in app code or unit tests.

We need to figure out how to clean terms that are not attached to posts on update.

#6 @boonebgorges
5 years ago

  • Keywords needs-unit-tests removed

The only terms that get cleaned are those that are attached to posts <---- that is a bug.

Yes, but it's a red herring. The issue here is that wp_update_term() should fail when the parent doesn't exist, not necessarily that it should fail when the parent is *deleted*. So the proposed unit test is testing too many things. I'll open a separate ticket for it.

#7 @boonebgorges
5 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

In 29867:

Return an error when adding a term to a non-existent parent.

Parallels the logic of wp_insert_term(), introduced in [29196].

Props jesin.
Fixes #29614.

#8 in reply to: ↑ 5 @boonebgorges
5 years ago

Replying to wonderboymusic:

The only terms that get cleaned are those that are attached to posts <---- that is a bug. You should never have to call clean_term_cache() directly in app code or unit tests.

We need to figure out how to clean terms that are not attached to posts on update.

Following up: I don't see any evidence that being attached to posts has anything to with it. In this case, it has to do with clearing the cache for child terms when the parent is deleted. #29911.

Note: See TracTickets for help on using tickets.