WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#35227 closed defect (bug) (fixed)

Add unregister_taxonomy()

Reported by: swissspidy Owned by: swissspidy
Milestone: 4.5 Priority: normal
Severity: normal Version: 2.3
Component: Taxonomy Keywords: has-unit-tests has-patch commit
Focuses: Cc:

Description

This is a follow-up to #11058.

While working on #14761 I stumbled upon the former ticket suggesting adding a unregister_taxonomy() function. It ended up with the addition of a new register_taxonomy_for_object_type() function.

Since _unregister_taxonomy() is used across all unit tests, I think we should add a real function for this for developers.

From Nacin's comment from three years ago:

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

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.

I'm gonna work on this together with #14761.

Attachments (3)

35227.diff (39.5 KB) - added by swissspidy 4 years ago.
35227.2.diff (5.3 KB) - added by swissspidy 4 years ago.
35227.3.diff (5.6 KB) - added by swissspidy 4 years ago.

Download all attachments as: .zip

Change History (12)

@swissspidy
4 years ago

#1 @swissspidy
4 years ago

  • Keywords has-patch has-unit-tests dev-feedback added; needs-patch needs-unit-tests removed

35227.diff adds the same remove_rewrite_tag() function as the patch in #14761 and does the following:

  • Properly removes the taxonomy from the $wp_taxonomies global
  • Removes rewrite tags and permastructs
  • Removes the callback for handling the meta box
  • Prevents unregistering built-in taxonomies

#2 @swissspidy
4 years ago

@boonebgorges You're the terms & taxonomy guy. Mind having a look at this one?

#3 follow-up: @boonebgorges
4 years ago

  • Keywords needs-patch added; early has-patch dev-feedback removed

@swissspidy A few thoughts, in no particular order:

  • Docblock description for WP_Rewrite::remove_rewrite_tag() is copied from add_rewrite_tag() and needs to be updated.
  • After unsetting array values in remove_rewrite_tag(), we could have gaps in the numerical array keys (ie: 0, 1, 3, 4). Does this matter? Worth resetting indexes using array_values() after unset()ing?
  • Personal preference, but I'd say that with a new function (and no backward compatibility concerns) we should be returning descriptive WP_Error objects on various failures in unregister_taxonomy().
  • Between this ticket and #14761, it's probably worth adding a WP::remove_query_arg() helper.
  • Similarly, I'd prefer a remove_permastruct() method to reaching into $wp_rewrite's public properties.
  • register_taxonomy() adds a specific callback to 'wp_ajax_add-' . $taxonomy, but when unregistering, you remove_all_actions(). Perhaps it's wiser to be conservative here and only undo the specific add_action() from register_taxonomy().
  • In the test suite, don't remove _unregister_taxonomy(). Just make it a wrapper. Plugins are using it.

Looks like you have pretty well covered the various pieces of register_taxonomy() aside from these issues.

@swissspidy
4 years ago

#4 in reply to: ↑ 3 @swissspidy
4 years ago

  • Keywords has-patch added; needs-patch removed

Replying to boonebgorges:

Thanks for your feedback so far! I incorporated it in the latest patch.

I created new tickets for adding the individual helper methods:

  • #35234 for WP::remove_query_var()
  • #35235 for remove_permastruct()
  • #35236 for remove_rewrite_tag()

35227.2.diff relies on these patches, making this one much more readable.

After unsetting array values in remove_rewrite_tag(), we could have gaps in the numerical array keys (ie: 0, 1, 3, 4). Does this matter? Worth resetting indexes using array_values() after unset()ing?

I see no harm in not re-indexing the arrays.

#5 @boonebgorges
4 years ago

35227.2.diff is looking good - so much lovelier with the new functions :) Once those other tickets are landed, I think this one is good to go.

@swissspidy
4 years ago

#6 @swissspidy
4 years ago

  • Keywords commit added

After the other tickets landed, I slightly updated the unit tests in 35227.3.diff to make them easier to read. Adds a new test_unregister_taxonomy_removes_permastruct() test.

#7 @swissspidy
4 years ago

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

In 36243:

Taxonomy: Introduce unregister_taxonomy().

This new function can be used to completely unregister non built-in taxonomies.

Fixes #35227.

#8 @johnbillion
4 years ago

In 36247:

Taxonomy: More tests for unregister_taxonomy().

See #35227

#9 @DrewAPicture
4 years ago

In 36961:

Docs: Improve the summary and return description in the DocBlock for unregister_taxonomy(), introduced in [36243].

See #35227. See #35986.

Note: See TracTickets for help on using tickets.