Opened 8 years ago
Last modified 8 years ago
#40821 new enhancement
Add filter to `wpmu_delete_blog()` to overwrite ability to drop tables
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | normal | Version: | 4.8 |
Component: | Networks and Sites | Keywords: | |
Focuses: | multisite | Cc: |
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)
Change History (5)
#1
@
8 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:
↓ 3
↓ 4
@
8 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
@
8 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
@
8 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 :)
Patch