WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 2 years ago

#34413 closed defect (bug) (fixed)

calling register_taxonomy() multiple times will overwrite existing object types

Reported by: BjornW Owned by: drewapicture
Milestone: 4.9 Priority: normal
Severity: normal Version: 2.6
Component: Taxonomy Keywords:
Focuses: docs Cc:
PR Number:

Description

Using register_taxonomy() with the same taxonomy, but different object types will result in the already registered object types to be overwritten.
The comment at the register_taxonomy() function state that it will modify (not overwrite existing registrated object types with this taxonomy), therefor the comment is wrong or the code needs to be adjusted.

Attachments (8)

example-plugins.zip (182 bytes) - added by BjornW 4 years ago.
Two plugins to demonstrate the overwriting of registered object types by using the register_taxonomy() twice
screenshot1.png (116.8 KB) - added by BjornW 4 years ago.
Screenshot 1 taxonomy 'beroepsgroep' added to Posts
screenshot2.png (115.6 KB) - added by BjornW 4 years ago.
Screenshot 2 taxonomy 'beroepsgroep' removed from Posts
screenshot3.png (114.4 KB) - added by BjornW 4 years ago.
Screenshot 2 taxonomy 'beroepsgroep' removed from Posts and now added to Pages
34413.1.diff (1.1 KB) - added by danielbachhuber 4 years ago.
Clarify register_taxonomy() documentation to respect actual behavior
add-extra-info-to-register-taxonomy-comment.diff (1.0 KB) - added by BjornW 4 years ago.
Change comment and warn for multiple calls of register_taxonomy()
34413.2.diff (1.7 KB) - added by SergeyBiryukov 4 years ago.
34413.3.diff (1.6 KB) - added by SergeyBiryukov 4 years ago.

Download all attachments as: .zip

Change History (25)

@BjornW
4 years ago

Two plugins to demonstrate the overwriting of registered object types by using the register_taxonomy() twice

@BjornW
4 years ago

Screenshot 1 taxonomy 'beroepsgroep' added to Posts

@BjornW
4 years ago

Screenshot 2 taxonomy 'beroepsgroep' removed from Posts

@BjornW
4 years ago

Screenshot 2 taxonomy 'beroepsgroep' removed from Posts and now added to Pages

@danielbachhuber
4 years ago

Clarify register_taxonomy() documentation to respect actual behavior

#1 @danielbachhuber
4 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.4
  • Owner set to drewapicture
  • Status changed from new to reviewing
  • Version changed from 4.3.1 to 2.6

This language has been in here since r8164

@BjornW
4 years ago

Change comment and warn for multiple calls of register_taxonomy()

#2 @BjornW
4 years ago

Whoops, didn't notice @danielbachhuber patch...

I've also added a patch which warns you what will happen if you call register_taxonomy multiple times for the same taxonomy with different object types. In the future it might be better to change this behavior by checking the existance of the taxonomy and using taxonomy_register_for_object_type to register nultiple object types to the same taxonomy.

#3 follow-up: @danielbachhuber
4 years ago

No worries. I do like how you've broken out the note as a separate line, which makes it more notable.

#4 follow-up: @SergeyBiryukov
4 years ago

Should we add a similar note to register_post_type()?

#5 in reply to: ↑ 3 @BjornW
4 years ago

Replying to danielbachhuber:

No worries. I do like how you've broken out the note as a separate line, which makes it more notable.

Thanks.

Although I think we also need to find a better solution for the future in dealing with this. It has been causing multiple headaches with plugins overwritting taxonomy registrations silently. At the very least it should trigger somekind of warning.

#6 in reply to: ↑ 4 @BjornW
4 years ago

Replying to SergeyBiryukov:

Should we add a similar note to register_post_type()?

Not sure what you mean? As in subsequently calling register_post_type()? Need to test that first to see if it has the same issue.

#7 @SergeyBiryukov
4 years ago

I think the current docs are somewhat accurate, overwriting a previously registered taxonomy counts as modifying.

Using register_taxonomy() with the same taxonomy, but different object types will result in the already registered object types to be overwritten.

Looks like register_post_type() uses register_taxonomy_for_object_type() and does not have the same issue when being subsequently called with different taxonomies.

We could use the same approach in register_taxonomy() too, see 34413.2.diff. Unit tests would be great.

#8 @SergeyBiryukov
4 years ago

  • Keywords needs-unit-tests added

#9 follow-up: @SergeyBiryukov
4 years ago

  • Keywords has-unit-tests commit added; needs-unit-tests removed

34413.2.diff didn't work as expected, 34413.3.diff does.

#10 in reply to: ↑ 9 @BjornW
4 years ago

Replying to SergeyBiryukov:

34413.2.diff didn't work as expected, 34413.3.diff does.

Great! So this is ready to be committed to core or does it need some more work? If so please let me know so I can help out with this :)

#11 @boonebgorges
4 years ago

  • Keywords has-unit-tests commit removed

34413.3.diff seems wrong to me. Either we should (a) bail out of register_taxonomy( 'foo' ) with an error if foo is already registered (so the first call to register_taxonomy() wins), or (b) we should overwrite the entire taxonomy each time (so the last one wins). Treating object_type differently from every other parameter is a recipe for confusion.

Even if we were going to allow subsequent calls to register_taxonomy() to update existing taxonomies, merging the object types doesn't seem right. Say I originally registered the taxonomy with object type 'post' and I want to change it to 'page' (only). The naive syntax (on the current proposal) would be to say register_taxonomy( 'foo', 'page' ). But with 34413.3.diff , there's no way to do this.

If you want to add additional object types to an existing taxonomy, use register_taxonomy_for_object_type().

I think the current docs are somewhat accurate, overwriting a previously registered taxonomy counts as modifying.

This seems like the crux of the problem: you can think of it this way, but it's not technically the case. I think we should make the docs clearer that what really happens here is an overwrite add-extra-info-to-register-taxonomy-comment.diff. (

If we want a function for updating taxonomies, that's fine, but it should be a separate function, maybe one that wraps register_taxonomy(). See wp_update_post().

#12 @danielbachhuber
4 years ago

I don't think we should change the behavior of the functions as a part of this ticket. I do think we should clarify the PHPdoc as a part of this ticket.

#13 @wonderboymusic
4 years ago

  • Milestone changed from 4.4 to Future Release

This ticket was mentioned in Slack in #core by wonderboymusic. View the logs.


4 years ago

#15 @swissspidy
3 years ago

  • Keywords needs-patch good-first-bug added; has-patch removed

I don't think we should change the behavior of the functions as a part of this ticket. I do think we should clarify the PHPdoc as a part of this ticket.

+1. 34413.3.diff isn't possible with WP_Taxonomy anyway. See also #31154 which is slightly related.

#16 @DrewAPicture
2 years ago

  • Keywords needs-patch good-first-bug removed
  • Milestone changed from Future Release to 4.9

#17 @DrewAPicture
2 years ago

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

In 41280:

Docs: Add a note to the register_taxonomy() DocBlock that the object types defined in $object_type when modifying an already-registered taxonomy will be overwritten.

Props danielbachhuber and BjornW for the initial patches.
Fixes #34413.

Note: See TracTickets for help on using tickets.