WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 2 years ago

#40821 new enhancement

Add filter to `wpmu_delete_blog()` to overwrite ability to drop tables

Reported by: mermel Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.8
Component: Networks and Sites Keywords:
Focuses: multisite Cc:
PR Number:

Description

The function wpmu_delete_blog( $blog_id, $drop = false ) includes a flag to add the ability to drop tables. There are some cases where we may want to prevent accidental deletion of tables, even when that flag is set to true.

Adding a filter like: $drop = apply_filters( 'wpmu_allow_drop_tables', $drop ); will allow manual overwriting of the passed value.

Attachments (1)

ms.diff (362 bytes) - added by mermel 2 years ago.
Patch

Download all attachments as: .zip

Change History (5)

@mermel
2 years ago

Patch

#1 @johnjamesjacoby
2 years ago

Hi @mermel, thanks for the ticket and patch!

I, for one, think something like this is a great idea. There are a few historical things that make doing this a bit more complicated than your patch, but you're on the right track. See: #40280

Also, today, you *can* filter wpmu_drop_tables, but that will still delete the row from wp_blogs and media, so it does work differently than the proposed patch does (which is to short-circuit all deletion, including wp_blogs and media.)

You can expect this ticket to go through several days/weeks with of conversation, and iterations on the patch to happen while it's discussed more deeply. You're also welcome to join the #core-multisite channel in the WordPress Slack if you'd like to discuss this in real-time.

(Tangent: I always forget this function is admin-only.)

#2 follow-ups: @flixos90
2 years ago

I'm curious about a use-case for this. When would you want to deny dropping tables even though it is set? I'm not opposed to the filter, but would like to get some background.

Also, as a preliminary reminder, we should consider using the current naming conventions for sites, networks and functions when thinking about a name for the filter, especially as we're planning to work on new improved functions for the functionality (in #40364 and some not-yet-created tickets that were briefly discussed during the multisite chat recently). I don't think we should use the wpmu_ prefix for example. Might be weird to not have it there at the moment, but long-term it should make sense: If we have a function like wp_drop_site_tables() and the filter had a wpmu_ prefix, it would be worse in my opinion.

Also sorry for diving into detail too early. :)

#3 in reply to: ↑ 2 @jeremyfelt
2 years ago

  • Milestone changed from Awaiting Review to Future Release

Replying to flixos90:

I'm curious about a use-case for this. When would you want to deny dropping tables even though it is set? I'm not opposed to the filter, but would like to get some background.

We use the current wpmu_drop_tables filter so that site's tables are retained in the DB even when the site's record is deleted from wp_blogs. This is partially because the DB_USER does not have the capability to DROP TABLES and any attempt creates an unnecessary error log. It also makes data retention easier.

There's a good chance that I would also make use of a short circuit filter to avoid deleting uploads as well.

I agree that we can go with current naming. I'd also want this named to convey that more than the tables are being dropped/not dropped.

#4 in reply to: ↑ 2 @mermel
2 years ago

Replying to flixos90:

I'm curious about a use-case for this. When would you want to deny dropping tables even though it is set? I'm not opposed to the filter, but would like to get some background.

We have environments that we prevent certain queries, like those containing DROP TABLE.. to run. However we do still run the core tests in these environments. A few of the tests that run under multisite use wpmu_delete_blog( x, true ). Similarly with the deletion of the uploads directory, we would prefer to handle the cleanup of those files explicitly rather than risks the test unintentionally deleting customer data.

An example can be found here: https://core.trac.wordpress.org/browser/trunk/tests/phpunit/tests/user/getActiveBlogForUser.php#L54

This filter would prevent us from having to modify core files in our implementation only, therefore simplifying merges going forward.

Good point on the filter name, I agree it should be changed too :)

Note: See TracTickets for help on using tickets.