WordPress.org

Make WordPress Core

Opened 9 years ago

Closed 5 years ago

Last modified 2 years ago

#11058 closed enhancement (fixed)

Add unregister_taxonomy_for_object_type()

Reported by: scribu Owned by: nacin
Milestone: 3.7 Priority: normal
Severity: normal Version:
Component: Taxonomy Keywords: has-patch needs-testing needs-unit-tests
Focuses: Cc:

Description

There should be a function that unregisters a certain taxonomy after it has been registered.

Attachments (9)

unregister_taxonomy.diff (825 bytes) - added by scribu 9 years ago.
unregister_taxonomy.2.diff (1.1 KB) - added by scribu 8 years ago.
return bool, improve doc
unregister_taxonomy_for_object_type.patch (1.1 KB) - added by leewillis77 8 years ago.
I added a proposed implementation for unregister_taxonomy_for_object_type on #14482 (Which has been closed as a dupe of this bug). There was an issue with the patch I provided on that bug, so revised patch is attached here.
11058_unregister_taxonomy_from_object_type.diff (1021 bytes) - added by leewillis77 6 years ago.
Patch updated, and re-rolled against 3.4.1
11058.diff (1.7 KB) - added by wonderboymusic 5 years ago.
11058-2.diff (1.6 KB) - added by leewillis77 5 years ago.
Updated patch for unregister_taxonomy_from_object_type(). Removed get_post_type_object check as per conversation with nacin in IRC
11058-3.diff (1.9 KB) - added by leewillis77 5 years ago.
Updated patch which documents the need for the post_type checks for the benefit of our future selves.
11058-4.diff (4.5 KB) - added by leewillis77 5 years ago.
Adds initial unit tests
11058-5.diff (4.5 KB) - added by leewillis77 5 years ago.
Updated patch to remove use of rand_str in tests based on advice from tierra in IRC.

Download all attachments as: .zip

Change History (50)

#1 @scribu
9 years ago

  • Cc scribu@… added

#2 @mattrude
9 years ago

  • Cc m@… added

#3 follow-up: @hakre
8 years ago

Reviewed the patch, I think it should return true on success and false on failure. Can benefit from a better docblock.

#4 @hakre
8 years ago

  • Keywords reporter-feedback added

#5 in reply to: ↑ 3 @scribu
8 years ago

  • Keywords reporter-feedback removed

Replying to hakre:

Reviewed the patch, I think it should return true on success and false on failure. Can benefit from a better docblock.

Agree. Also, the changes done to the rewrite globals aren't reverted.

@scribu
8 years ago

return bool, improve doc

#6 @nacin
8 years ago

  • Owner changed from filosofo to dd32
  • Status changed from new to assigned

#7 @dd32
8 years ago

if ( empty($wp_taxonomies[$taxonomy]->object_type) ) 
 	222	                        unset($wp_taxonomies[$taxonomy]); 

I dont see the need to remove the taxonomy when it has no object types assoc. with it, I mean, With the functions to add a object type to a taxonomy after its registered, it makes sense that plugins could be removing, and then replacing the object types.

I'm +1 for this, so i'll get it in later sans that block, unless someone has a good arguement against that?

#8 @dd32
8 years ago

  • Keywords needs-patch added; has-patch removed

After looking at this closer, I think a single function is going the wrong way.

unregister_taxonomy() should remove the taxonomy all together, and cleanup any query vars which it may've registered

unregister_taxonomy_for_object_type() should remove a object type from a taxonomy.

While we're at it, the Documentation for register_taxonomy() probably needs updating, IT doesnt modify taxonomies, It mearly replaces them..

#9 @nacin
8 years ago

  • Type changed from enhancement to task (blessed)

#10 follow-up: @dd32
8 years ago

  • Milestone changed from 3.0 to 3.1
  • Type changed from task (blessed) to enhancement

Moving to 3.1 as this is an enhancement, and no ideal patch is available.

My outlined ideas there may require extra changes to query parsing as well.

What use-cases are there for unregistering taxonomies? Given that they need to be registered every page load..

#11 @scribu
8 years ago

The trouble was with built-in taxonomies. The best we can do currently is remove_action('init', 'create_builtin_taxonomies', 0); which I guess isn't so bad.

#12 @scribu
8 years ago

Related: #12629

#13 @kevinB
8 years ago

  • Cc kevinB added

@leewillis77
8 years ago

I added a proposed implementation for unregister_taxonomy_for_object_type on #14482 (Which has been closed as a dupe of this bug). There was an issue with the patch I provided on that bug, so revised patch is attached here.

#14 @leewillis77
8 years ago

  • Cc leewillis77 added

#15 @nacin
8 years ago

  • Milestone changed from Awaiting Triage to Future Release

#16 in reply to: ↑ 10 @tamlyn
7 years ago

Replying to dd32:

What use-cases are there for unregistering taxonomies? Given that they need to be registered every page load..

I want to unregister a taxonomy registered by a plugin, without modifying the plugin code.

#17 @jkudish
7 years ago

  • Cc jkudish added

what's the status of this? would be great to see it in 3.2.

I've been using @leewillis77 implementation as a function and it works flawlessly.

#18 @jkudish
7 years ago

  • Keywords has-patch added; needs-patch removed

#19 @doolin
7 years ago

  • Cc doolin added

#20 @dd32
6 years ago

  • Owner dd32 deleted

Can someone with more recent Taxonomy experience comment here? I can't think of anything this would break, although if it's being used to remove a taxonomy that a plugin adds, it could cause issues for rewrite rules perhaps?

#21 @leewillis77
6 years ago

There are two distinct patches on this bug. One implements unregister_taxonomy(), and mine implements unregister_taxonomy_for_object_type(). The second one is on here because the original bug was closed as a dupe of this, but it seems that it's not, and there doesn't seem to be traction where there are two different things going on (For example disassociating a taxonomy from an object type shouldn't affect rewrite rules AFAIK?).

Should I log that separately? Re-Open the original?

#22 @jkudish
6 years ago

To re-iterate, I am using the patch by leewilliis77 on many live sites and it works without problem and doesn't break anything. We've even included it in this plugin: http://wordpress.org/extend/plugins/sld-custom-content-and-taxonomies/ for others to use. I still think this should make it to core.

#23 @dd32
6 years ago

(For example disassociating a taxonomy from an object type shouldn't affect rewrite rules AFAIK?

That should be fine, I can't think of any case that would break.

Should I log that separately? Re-Open the original?

I wouldn't bother, both of these functions go hand in hand.

#24 @Chouby
6 years ago

  • Cc Chouby added

@leewillis77
6 years ago

Patch updated, and re-rolled against 3.4.1

#25 @jakemgold
6 years ago

  • Cc jake@… added

Agreed. This should definitely be in core.

#26 follow-up: @nacin
6 years ago

A unregister_taxonomy() function needs to un-roll quite a bit:

  • Remove the query var from the WP class, if one was registered.
  • Remove any rewrite tags and permastructs.
  • Remove the callback for handling the meta box.

A separate unregister_taxonomy_for_object_type() function needs to remove items from object_type.

An unregister_taxonomy() function should disallow the unregistration of 'post_tag' and 'category' until core does not assume they exist in many, many locations. The same block might also need to be in place in unregister_taxonomy_for_object_type() for the 'post' object type until core no longer breaks under any existing assumptions as well.

#27 in reply to: ↑ 26 @jakemgold
6 years ago

'unregister_taxonomy' is unquestionably a hot mess (and what we should probably strive for is an early hook in register_taxonomy that can prevent taxonomy registration, particularly for something like posts, rather than unloading it all after the fact).

Not sure that this applies to unregistering the taxonomy for an object type, however, even for "core" taxonomies against post. I've used this somewhat extensively, even before this patch, and never encountered any serious glitches, and none that would count as a legitimate "bug." There's a certain... "weirdness" to lingering references to Categories and Post Types in the admin ("Right Now", permalink settings, etc) when no objects are actually using them (probably because we ideally want to get to unregister_taxonomy...), but nothing breaks, since any "retrieval" of individual object terms - even core ones - all goes through the abstracted taxonomy functions, and the taxonomy still exists, protecting against any direct taxonomy references (e.g. core category / tag archives).

Any idea where there might be something more subtle lurking that would break?

#28 @jakemgold
6 years ago

I should add that one of the peculiar behaviors - core taxonomy and objects or not - when unregistering a taxonomy against an object is that objects previously assigned terms in those taxonomy retain those terms.

So even in ideal circumstances when you register a custom post type and taxonomy, assign some post objects some terms in that taxonomy, than unregister the taxonomy against the post type object (but don't remove the taxonomy), queries against those terms still return posts assigned terms against that taxonomy (without any GUI to unassign them).

If we care about this (pretty questionable path), we'd need to modify queries against taxonomy terms to limit its results to post types assigned those taxonomies... I wonder if we should do this anyways...

#29 @leewillis77
6 years ago

In reply to: "The same block might also need to be in place in unregister_taxonomy_for_object_type() for the 'post' object type until core no longer breaks under any existing assumptions as well."

Just to echo Jake's comments for what it's worth - I've been using the patch for unregister_taxonomy_for_object_type() on several sites, and haven't seen anything "break". I haven't particularly noticed any weirdness either. Happy to test specific scenarios if you have something in mind?

#30 follow-up: @SergeyBiryukov
6 years ago

Replying to jakemgold:

So even in ideal circumstances when you register a custom post type and taxonomy, assign some post objects some terms in that taxonomy, than unregister the taxonomy against the post type object (but don't remove the taxonomy), queries against those terms still return posts assigned terms against that taxonomy (without any GUI to unassign them).

Related: #21290

Replying to leewillis77:

I've been using the patch for unregister_taxonomy_for_object_type() on several sites, and haven't seen anything "break". I haven't particularly noticed any weirdness either. Happy to test specific scenarios if you have something in mind?

Related: #19590

#31 in reply to: ↑ 30 @leewillis77
6 years ago

Replying to SergeyBiryukov:

Replying to leewillis77:

I've been using the patch for unregister_taxonomy_for_object_type() on several sites, and haven't seen anything "break". I haven't particularly noticed any weirdness either. Happy to test specific scenarios if you have something in mind?

Related: #19590

#19590 relates to unregistering taxonomies, and would be an issue for unregister_taxonomy(), but shouldn't affect the implementation of unregister_taxonomy_for_object_type(). Indeed, having unregister_taxonomy_for_object_type() available provides a way for people to "hide" the taxonomy without having to deal with any issues in core that would be exposed by removing the taxonomy entirely.

#32 @sscovil
5 years ago

  • Cc sscovil@… added

#33 @kovshenin
5 years ago

  • Cc kovshenin added

@wonderboymusic
5 years ago

#34 @wonderboymusic
5 years ago

rehab'd whitespace

#35 @wonderboymusic
5 years ago

#12629 was marked as a duplicate.

@leewillis77
5 years ago

Updated patch for unregister_taxonomy_from_object_type(). Removed get_post_type_object check as per conversation with nacin in IRC

#36 @nacin
5 years ago

Updated patch for unregister_taxonomy_from_object_type(). Removed get_post_type_object check as per conversation with nacin in IRC

Per #25082, let's keep the get_post_type_object() check after all.

@leewillis77
5 years ago

Updated patch which documents the need for the post_type checks for the benefit of our future selves.

#37 @nacin
5 years ago

  • Keywords needs-testing needs-unit-tests added
  • Milestone changed from Future Release to 3.7

Could use some thorough testing as well as some unit tests.

It would be good to also test unregistering categories and tags from posts, though if that didn't work fully, I wouldn't find it to be a blocker for 3.7. It would still be good to find those holes.

@leewillis77
5 years ago

Adds initial unit tests

#38 @leewillis77
5 years ago

I am using this on at least one production site where a core taxonomy (category) is unregistered from page, and post_tag is unregistered from post without any significant issues, as per http://core.trac.wordpress.org/ticket/11058#comment:28

That's not to say it couldn't use broader testing, but there seem to be a few people here also using it as-is.

I've updated the patch to include some basic unit tests for discussion - are they sufficient - or would you expect the tests to be validating the contents of $wp_taxonomies as well as checking True/False return vals?

@leewillis77
5 years ago

Updated patch to remove use of rand_str in tests based on advice from tierra in IRC.

#39 @nacin
5 years ago

  • Summary changed from Add unregister_taxonomy() to Add unregister_taxonomy_for_object_type()

#40 follow-up: @nacin
5 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from assigned to closed

In 25596:

Introduce register_taxonomy_for_object_type().

props leewillis77.
fixes #11058.

#41 in reply to: ↑ 40 @lkraav
2 years ago

Replying to nacin:

In 25596:

Introduce register_taxonomy_for_object_type().

Pardon the gravedigging, @nacin but shouldn't the comment say *un*register_taxonomy_for_object_type()? I don't have comment edit permissions to fix myself.

Note: See TracTickets for help on using tickets.