Make WordPress Core

Opened 8 years ago

Closed 7 years ago

#20651 closed enhancement (wontfix)

Inconsistent MS API: delete_site_option action hook is only called AFTER deletion, unlike delete_option

Reported by: danielnashnz Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.3.2
Component: Multisite Keywords: dev-feedback has-patch
Focuses: Cc:


Unlike the delete_option action hook, the delete_site_option action hook is only called AFTER deletion of the row from the sitemeta table - making it difficult to capture the expiring value (for instance), unless you know its precise option key name ( and hook it using pre_delete_site_option_{$option} ).

Suggest introducing a pre-deletion, non-specific action hook call named "pre_delete_option" for options and "pre_delete_site_option" for site options, for consistency.

Attachments (4)

20651.diff (1.2 KB) - added by scribu 8 years ago.
20651.2.diff (1.2 KB) - added by SergeyBiryukov 8 years ago.
20651.3.diff (1.2 KB) - added by jeremyfelt 7 years ago.
20651.4.diff (437 bytes) - added by jeremyfelt 7 years ago.

Download all attachments as: .zip

Change History (16)

#1 @SergeyBiryukov
8 years ago

  • Component changed from General to Multisite
  • Keywords dev-feedback added; delete_site_option MS removed

8 years ago

#2 @scribu
8 years ago

  • Keywords has-patch added

What a mess. 20651.diff:

  • add 'pre_delete_site_option' and call it only if the option exists
  • add 'pre_delete_option_{$option}'

#3 @scribu
8 years ago

Similar efforts: [13148]

#4 @danielnashnz
8 years ago

To keep it consistent with delete_option and delete_site_option, shouldn't $option be passed as well in the do_action calls?

#5 @scribu
8 years ago

Obviously, 'pre_delete_site_option' is useless if we don't pass the $option. Sergey already submitted a fixed patch.

#6 @DrewAPicture
8 years ago

  • Cc xoodrew@… added

7 years ago

#7 @jeremyfelt
7 years ago

  • Milestone changed from Awaiting Review to 3.7

20651.3.diff is a refresh of Sergey's patch for current trunk. Moving to 3.7 to add the action or close the ticket.

#8 follow-up: @nacin
7 years ago

Option 1:

  • I wonder if we could just break things slightly here, move the delete_site_option hook to be a pre-delete hook, introduce a new deleted_site_option hook, and just ignore that pre_delete_site_option_$option exists.

Option 2:

  • This isn't just in delete_site_option(). The same pattern exists in the other site_option() functions. While this pattern differs from the regular option() functions, it is at least consistent across the site_option API. Maybe wontfix? If/when we rename these to network_option we could use the other hook pattern.

7 years ago

#9 @jeremyfelt
7 years ago

Beyond pattern, the weird parts here are:

  1. pre_site_delete_option_$option runs even if delete_site_option() is run in single site mode.
  2. The value of $option is required in advance to use pre_site_delete_option_$option

I think a patch is worth it to fix at least the pass of $option to the action, which is the original description of this ticket. See 20651.4.diff

In the long run, I think Nacin's option 2 is probably best. In some ways I find the pattern of the hooks in the site_option functions to be more intuitive.

#10 @nacin
7 years ago

20651.4.diff is OK. Again, though, add_site_option() and update_site_option() similarly follow the existing pattern and should also thus get new pre_*_site_option, $option hooks.

#11 in reply to: ↑ 8 @jeremyfelt
7 years ago

Replying to nacin:

Maybe wontfix? If/when we rename these to network_option we could use the other hook pattern.

I'm leaning this way now. We're adding at least 3 hooks (if not another for get_site_option()), that will end up being misnamed in the future.

#12 @jeremyfelt
7 years ago

  • Milestone 3.7 deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Closing as wontfix due to varying parts of the discussion. If we add a new pre_site_delete_option filter, then we would want to make sure the pattern is consistent across all *_site_option() functions. In the future, we'd like to have *_network_option() and we would be stuck with maintaining more confusing names.

Note: See TracTickets for help on using tickets.