[wp-trac] [WordPress Trac] #30335: Splitting shared terms on term update breaks backward compatibility when using an old term_id

[WordPress Trac]

#30335: Splitting shared terms on term update breaks backward compatibility when
using an old term_id
--------------------------+--------------------
 Reporter:  boonebgorges  |       Owner:
     Type:  defect (bug)  |      Status:  new
 Priority:  high          |   Milestone:  4.1
Component:  Taxonomy      |     Version:  trunk
 Severity:  major         |  Resolution:
 Keywords:  needs-patch   |     Focuses:
--------------------------+--------------------
Changes (by boonebgorges):

 * keywords:  has-patch 2nd-opinion => needs-patch


Comment:

 mboynes - Wow, thanks for your work on this.

 > I would propose tweaking this to store the old term ids in multiple
 options, one per taxonomy. Within each option, store an array of
 old_tem_id => new_term_id

 This is a much better idea than what I'd originally done. Big +1.

 > This is currently going to autoload the option. Do we want it to? I
 would say no

 I know I originally suggested this, but when I wrote my first patch I
 waffled. autoload is basically irrelevant for sites with a persistent
 cache, so let's weigh it for sites without. The case for autoload=no is
 that this is a value that won't be used often on most sites, and it adds
 some overhead to load it into memory on every pageload, especially if
 there are lots of split terms. The case for autoload=yes is that the
 memory overhead is small because split terms are likely to be pretty rare,
 even on very large sites; and those large sites are the ones most likely
 to have persistent caching; and on sites where there is no persistent
 caching, we don't want to cause an additional DB query potentially on
 every pageload (as will be required by, say, a nav item linking to a split
 term). On balance, the case for autoload=no seems less persuasive to me,
 but I'm happy to be swayed.

 > Should _get_split_term() be @private? Not that it's actually private,
 but why aren't plugins and themes welcome to use it?

 My thinking was that `_get_split_term()` - or at least the way we use it
 internally - is as a sort of magical maneuver that we don't really want
 plugin authors to attempt to duplicate. But now that I'm writing this, I'm
 reconsidering - there are legitimate instances where plugins would want to
 do what we're doing here to provide the same kind of backward
 compatibility. Let's switch it.

 > Should we try to update menu items when terms get split?

 Good call. I'd missed this. Yes, we should.

 > When a term is deleted, should we check if there's an old term ID linked
 to it and fire 'delete_term' for that term ID too?

 I would say no. In `_split_shared_term()`, I explicitly decided *not* to
 fire the term creation actions, because while a row is technically being
 inserted into the terms table, it's not the case that a "term" is being
 "created" in any real sense. I'd say that this case is similar. I'd even
 shy away from deleting the entry from the `_split_terms_$taxonomy` option,
 because then there'll be no record of what happened, and in any case it's
 harmless to have it there, since future term lookups will return false
 anyway. I'd suggest that we ought to handle cases of term deletion on a
 case-by-case basis, and encourage plugin authors to do the same, though
 tbh I think it's going to be extremely rare that it's necessary to do
 anything on term deletion, since fetching a deleted term is going to
 return empty whether it was split or not.

 > default_category, default_post_format, default_link_category: I think
 these should just get updated when a term splits if the old term id is a
 default.

 Yes. I made a note to do this when I wrote the first patch, but I
 neglected to check the note :) I'd say this is a case where we want to
 hook to 'split_shared_term'.

 > This was mentioned in the ticket, but should we add a check to
 wp_dropdown_categories() to see if the 'selected' arg is an old term?

 Sure, this is a pretty easy win.

 I will be able to work on an updated patch within the next few days, but
 if you beat me to it, then all the better :) Once you and I are pretty
 satisfied with it, I'd like to run what we've got by nacin and johnbillion
 to see what they think of the approach. Thanks for your help!!

--
Ticket URL: <https://core.trac.wordpress.org/ticket/30335#comment:18>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform