Make WordPress Core

Opened 14 years ago

Closed 11 years ago

Last modified 11 years ago

#15475 closed enhancement (fixed)

Proposed new function: wp_unset_object_terms

Reported by: simonwheatley's profile simonwheatley Owned by: markjaquith's profile markjaquith
Milestone: 3.6 Priority: normal
Severity: normal Version:
Component: Taxonomy Keywords: has-patch
Focuses: Cc:

Description

In order to remove a single term associated with an object, leaving it's other terms in the same taxonomy, you have to call wp_get_object_terms, remove the terms you don't require from the array, then call wp_set_object_terms. I propose a single function, wp_unset_object_terms, which does this, applying relevant actions as it does.

Use case: A "starring" system whereby a user can (un)star a post to (un)favourite it. Each user has their own term where their stars are stored. You can use wp_set_object_terms with the append parameter (thanks for pointing that out Nacin) to "star" posts, but there's no simple way to "unstar" (and be sure that appropriate actions are run).

The attached patch adds the proposed function. Feedback, obviously, is welcome.

Attachments (8)

wp_unset_object_terms-101118-1.diff (2.8 KB) - added by simonwheatley 14 years ago.
Adds wp_unset_object_terms
15475.diff (2.0 KB) - added by ericmann 13 years ago.
Patch to add wp_unset_object_terms()
15475-2.diff (2.0 KB) - added by ericmann 13 years ago.
Alternate patch uses wp_remove_object_terms()
15475-3.diff (5.1 KB) - added by maxcutler 12 years ago.
DRY patch for wp_remove_object_terms
15475-4.diff (4.7 KB) - added by ericmann 11 years ago.
Refresh patch and update docs
15475-5.diff (5.3 KB) - added by ericmann 11 years ago.
Rounds out tag manipulation by adding wp_add_object_terms()
15475.tests.diff (1.6 KB) - added by kovshenin 11 years ago.
15475.2.diff (845 bytes) - added by kovshenin 11 years ago.

Download all attachments as: .zip

Change History (34)

@simonwheatley
14 years ago

Adds wp_unset_object_terms

#1 @scribu
14 years ago

  • Keywords 3.2-early added; dev-feedback removed
  • Milestone changed from Awaiting Review to Future Release

#2 @nacin
14 years ago

Can you svn up before diffing so the last commit doesn't leak through?

#3 @scribu
14 years ago

Instead of a new function, maybe it would be better to add a $term_ids parameter to wp_delete_object_term_relationships() ?

#4 @trepmal
13 years ago

  • Cc trepmal@… added

#5 @ericmann
13 years ago

After some discussion on Twitter, it is apparent that this function will be needed sooner rather than later, though it might be a bit late for 3.2.

I'm attaching a patch (written primarily by @trepmal) that extends this functionality to WordPress as wp_unset_object_terms(). It accepts the same style of parameters as wp_set_object_terms() and has all the appropriate actions as well.

@ericmann
13 years ago

Patch to add wp_unset_object_terms()

#6 follow-up: @scribu
13 years ago

  • Keywords 3.3-early added; 3.2-early removed

I think it would be better called wp_remove_object_terms() to avoid confusion with it's counterpart.

#7 in reply to: ↑ 6 @ericmann
13 years ago

Replying to scribu:

I think it would be better called wp_remove_object_terms() to avoid confusion with it's counterpart.

Fair enough. That was my original inclination when I opened #17878, but it was closed as a dup so I just took the naming schema from this ticket. New alternate patch uses wp_remove_object_terms() instead.

@ericmann
13 years ago

Alternate patch uses wp_remove_object_terms()

#8 @ericmann
13 years ago

So what more needs to be done to this patch to make it commit ready? I know we're not pushing things until the upcoming scope discussion for 3.3 ... but I want to make sure this is ready to be on the table.

#9 @scribu
13 years ago

Coding standards:

Avoid intermediary variables:

$tt_id = $term_info['term_taxonomy_id']; 
$tt_ids[] = $tt_id;

Add missing whitespace:

foreach ( (array) $terms as $term) {
return new WP_Error('invalid_taxonomy', __('Invalid Taxonomy'));
wp_update_term_count($delete_terms, $taxonomy);

etc.

#10 @jeremyclarke
13 years ago

  • Cc jer@… added

Just want to +1 this idea. Very useful and necessary since wp_set_object_terms() is not very user-friendly with it's need of array_mapped integers etc.

Glad to see it's already almost done :)

#11 @danielbachhuber
13 years ago

  • Cc wordpress@… added

#12 @maxcutler
13 years ago

  • Cc max@… added

#13 @mikeschinkel
13 years ago

  • Cc mikeschinkel@… added

#14 @iandunn
12 years ago

  • Cc ian_dunn@… added

+1

#15 @ejdanderson
12 years ago

  • Cc ejdanderson@… added

#16 @nacin
12 years ago

Since we have wp_set_object_terms(), I would agree the counterpart should be wp_unset_object_terms().

Whitespace issues aside, I'm curious of some structural aspects.

The filter in wp_set_object_terms() (and wp_delete_object_term_relationships()) is called delete_term_relationships, not delete_term_relationship.

The code at large needs to be abstracted and, overall, DRY. You can see at how relatively concise wp_delete_object_term_relationships() is -- it uses wp_get_object_terms() and then does the simple delete. The patch tries to do too much by emulating wp_get_object_terms(), rather than calling it.

Likewise, I would think that we could probably get away with wp_delete_object_term_relationships() becoming a wrapper for wp_unset_object_terms(), and wp_set_object_terms() calling wp_unset_object_terms() when it is time to delete the terms that need to be deleted. Ideally, delete_term_relationships should only fire once, anywhere, not in three places. It demonstrates that our API is properly structured.

Since we are passing around term IDs and need TTIDs, then this full level of abstraction may not be necessary or efficient. But maybe someone can take a crack at it?

#17 @nacin
12 years ago

Since we have wp_set_object_terms(), I would agree the counterpart should be wp_unset_object_terms().

I am willing to change my mind on that. Since set() does do replacements, adds, and removals, I understand the desire for remove() rather than unset(). Likewise, if we are to add remove(), we should probably add add(), which would simply wrap set() with $append = true.

@maxcutler
12 years ago

DRY patch for wp_remove_object_terms

#18 @maxcutler
12 years ago

I added a fresh patch based on ericmann's but implementing the improvements recommended by nacin.

I had to add another DB query to wp_set_object_terms in the append=true case to get the term IDs from the tt IDs. The only reason is for back-compat because $old_tt_ids is used in the do_action at the end of the method (otherwise I would just change $old_tt_ids to $old_term_ids). There may be a better way to handle that, but I couldn't find any examples in taxonomy.php.

#19 @simonwheatley
11 years ago

  • Keywords dev-feedback added; 3.3-early removed

Tested patch: applies cleanly and wp_remove_object_terms does what it says on the tin.

@ericmann
11 years ago

Refresh patch and update docs

@ericmann
11 years ago

Rounds out tag manipulation by adding wp_add_object_terms()

#20 @markjaquith
11 years ago

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

In 23398:

Introduce wp_remove_object_terms() and wp_add_object_terms(). No more fetch-alter-update nonsense. Use wp_remove_object_terms() in a few places internally.

props simonwheatley, scribu, ericmann, maxcutler. fixes #15475

#21 @markoheijnen
11 years ago

  • Milestone changed from Future Release to 3.6

#22 @nacin
11 years ago

  • Keywords needs-unit-tests added; has-patch dev-feedback removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Could we have some unit tests for this?

@kovshenin
11 years ago

#23 @kovshenin
11 years ago

  • Cc kovshenin added
  • Keywords has-patch added; needs-unit-tests removed

Hey there! 15475.2.diff makes sure that false is returned if nothing has been deleted, 15475.tests.diff covers the two new functions.

#24 @SergeyBiryukov
11 years ago

In 1227/tests:

Tests for wp_add_object_terms() and wp_remove_object_terms(). props kovshenin. see #15475.

#25 @SergeyBiryukov
11 years ago

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

In 23552:

Make wp_remove_object_terms() return false if nothing has been deleted. props kovshenin. fixes #15475.

#26 @teauser
11 years ago

  • Cc martrober@… added
Note: See TracTickets for help on using tickets.