Make WordPress Core

Opened 6 years ago

Last modified 3 months ago

#43162 new enhancement

Deleting a site from a multisite network leaves orphaned database tables and files

Reported by: bluep92877's profile bluep92877 Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.0
Component: Networks and Sites Keywords: has-patch changes-requested
Focuses: multisite Cc:

Description

Looking at the table structure of a typical multisite install, I see that tables for individual sites within the network are prefixed with wp_{siteid}_ tablename. When I delete a site from the from the network, all standard WP tables for that particular site ID are deleted; however, several plugin tables are not.

As well, looking at the uploads/sites directory, the directory for that site (represented by its ID) is also still there along with all its files (media and other uploads).

an earlier ticket:30673 suggested that this should be caught when uninstalling a plugin from the master site. But, I'd have to disagree. Most uninstall scripts use a type of {TABLEPREFIX} before its table names. This would delete the tables for every site in the network, wouldn't it? and that's not what the goal would be.

Aside from the plugin issue are the media files that are also orphaned. Media file uploads is part of core, and that's not something a plugin author could approach.

When deleting a site from the network, this is irrevocable. Wordpress even create an extra confirmation page upon delete saying so just to make sure. If that's the case, and this is by design, what's the point of keeping these extra files and tables if they are never going to be used again?

Shouldn't all site specific db tables and all files in the uploads directory be deleted when a site is deleted? Or, perhaps as an enhancement, on the Delete Site confirmation page, could we have two checkboxes: "1) delete all tables associated with this site" and "2) delete all files associated with this site"

Currently I have a script setup to alert me as to when a site is deleted. I then open up a cli to manually delete the db tables for that install, and then its files from the uploads directory. This doesn't feel like the intended workflow to handle the orphaned elements of the site.

Attachments (3)

Screen Shot 2023-01-31 at 4.46.15 PM.png (19.4 KB) - added by carl-alberto 16 months ago.
muconfirm.png (19.4 KB) - added by carl-alberto 16 months ago.
4172.diff (3.7 KB) - added by carl-alberto 12 months ago.
Trying to add a patch

Download all attachments as: .zip

Change History (37)

#1 @mdifelice
6 years ago

Hello. I do think that when you uninstall a plugin it will only delete the tables that it created for that specific site. Of course it depends on the plugin, and whether the plugin was network activated or only activated on the specific site.

Either way, I think that it would be a good addition what you propose. About the tables, it would be nice if WP listed all those orphan tables before deleting the site and ask the user what to do with them: delete or keep them.

About the media files, I think that each site has a particular folder assigned to them, and simply deleting it it would remove all the associated media.

Another doubt that I have is whether WP really deletes a site or it keeps like in a hidden status. Because if that is the case the idea is that the site could be recover at any time and in those cases files and extra tables should be kept.

Regards.

#2 @johnjamesjacoby
6 years ago

  • Keywords 2nd-opinion added
  • Type changed from defect (bug) to enhancement
  • Version changed from 4.9.2 to 3.0

Hey @bluep92877, thanks for opening this issue up, and congrats on your first ticket here on WordPress Trac.

I think you've brought up good points, and these are all areas where clarification and improvements are necessary.

Re: #30673, I agree that "uninstall" in the context of a multisite installation implies that all relevant data is removed from all sites, which seems heavy handed under most circumstances, though I can imagine why that might be necessary.

Plugins can (and should) use the wpmu_drop_tables filter to add their custom database tables to the array of tables that are dropped when a site is deleted.

Database tables and Uploads are both pretty sacred, IMO. If we are deleting one, we can safely delete the other. Dropping large database tables can be costly on the database server in a similar fashion to unlinking a directory. It looks like wpmu_delete_blog() does try to delete both the database tables and the uploads directory. Maybe something is wrong with the $drop logic, or something else is in the way?

That said, permanent deletion is so scary; I'm always hesitant to give anyone that power (even super admins.) 1 click to ruin it all is a lot of responsibility, even 2 or 3 clicks with warning signs and notices.


I think erring on the side of caution here is OK. Only delete stuff if we are 100% certain it's OK to do so, and leave it untouched if there is any doubt. This might result in orphaned tables or files, but I think that's better than blindly deleting them without explicit say-so.

I guess, now that I type this, we could (should) do a quick check on that delete-site confirmation screen that says "Here are all of the database tables this site has, and it contains 2GB of uploads. Are you sure you want to delete all of this stuff?"

#3 follow-ups: @scamartist26
6 years ago

For what it's worth, we have this same issue on our multisite install. I do not see any reason to keep tables around at all. We created this little guy as a workaround:

<?php

/**
 * Cleanup orphaned tables during site deletion
 *
 * @param $blog_id
 * @param $drop
 */
add_action( 'delete_blog', 'delete_blog_43162', 10, 2 );
function delete_blog_43162( $blog_id, $drop ) {

    if ( true == $drop ) {

        /**
         * SELECT all tables relating to a specific blog id and add them to wpmu_drop_tables
         */
        global $wpdb;
        $prep_query = $wpdb->prepare( "SELECT table_name FROM information_schema.TABLES WHERE table_name LIKE %s;", $wpdb->esc_like( "{$wpdb->base_prefix}{$blog_id}_" ) . '%' );
        $table_list = $wpdb->get_results( $prep_query, ARRAY_A );

        add_filter( 'wpmu_drop_tables', function ( $filter_list ) use ( $table_list ) {

            foreach( $table_list as $index => $data ) {
                $filter_list[] = $data['table_name'];
            }

            return array_unique( $filter_list );

        });
    }
}

#4 in reply to: ↑ 3 @waynmeyer
4 years ago

  • Keywords needs-testing added

Replying to scamartist26:

For what it's worth, we have this same issue on our multisite install. I do not see any reason to keep tables around at all. We created this little guy as a workaround:

<?php

/**
 * Cleanup orphaned tables during site deletion
 *
 * @param $blog_id
 * @param $drop
 */
add_action( 'delete_blog', 'delete_blog_43162', 10, 2 );
function delete_blog_43162( $blog_id, $drop ) {

    if ( true == $drop ) {

        /**
         * SELECT all tables relating to a specific blog id and add them to wpmu_drop_tables
         */
        global $wpdb;
        $prep_query = $wpdb->prepare( "SELECT table_name FROM information_schema.TABLES WHERE table_name LIKE %s;", $wpdb->esc_like( "{$wpdb->base_prefix}{$blog_id}_" ) . '%' );
        $table_list = $wpdb->get_results( $prep_query, ARRAY_A );

        add_filter( 'wpmu_drop_tables', function ( $filter_list ) use ( $table_list ) {

            foreach( $table_list as $index => $data ) {
                $filter_list[] = $data['table_name'];
            }

            return array_unique( $filter_list );

        });
    }
}

This worked like a charm, please could we have this added to core its vital especially for multisite. Alternative would be as @mdifelice stated, simply ask an admin when they deleting the subsite if they would like to delete orphaned tables with a checkbox next to each table for custom selection, when an admin wishes to retain data then the choice should be available as to what tables are chosen by the admin.

Bigup to @scamartist26 for assisting with the snippet.

#5 in reply to: ↑ 3 @deepakdcc
3 years ago

Last edited 3 years ago by deepakdcc (previous) (diff)

#6 @carl-alberto
16 months ago

To add on as a temporary solution in deleting the corresponding subsite's uploads folder

<?php
add_action( 'wp_delete_site', 'wpmu_delete_blog_uploads_folder' );

/**
 * Deletes the corresponding uploads folder of a subsite.
 *
 * @return boolean Return true if successful.
 */
function wpmu_delete_blog_uploads_folder() {

                require_once ABSPATH . 'wp-admin/includes/class-wp-filesystem-base.php';
                require_once ABSPATH . 'wp-admin/includes/class-wp-filesystem-direct.php';

                $upload_dir = wp_upload_dir(); // This function already calls the subsite's upload folder path automatically.

                $fileSystemDirect = new WP_Filesystem_Direct( false );

                return $fileSystemDirect->rmdir( $upload_dir['basedir'], true );

}

It seems @johnjamesjacoby is right that there is a ready function wpmu_delete_blog which also calls the wp_delete_site which also calls wp_uninitialize_site and supposedly deletes the corresponding uploads folder. Those functions might need to be revisited as it seems to be logical to delete those along when a subsite is deleted, otherwise a busy multisite can easily be bloated with unnecessary files or possibly have something like a checkbox for confirmation like what @mdifelice proposed, something like the attached image and might contain warnings that it may take awhile for large db/uploads:

Last edited 16 months ago by carl-alberto (previous) (diff)

This ticket was mentioned in PR #3951 on WordPress/wordpress-develop by carl-alberto.


16 months ago
#7

  • Keywords has-patch added

Update the wp_uninitialize_site function to delete the uploads folder and custom db tables created by plugins

Trac ticket: https://core.trac.wordpress.org/ticket/43162

#8 @carl-alberto
16 months ago

  • Keywords has-patch removed

Found the function that is supposed to delete the database table as well as the uploads folder which seems not to be working anymore https://github.com/WordPress/wordpress-develop/blob/186abb264d6c97dc955ecc787f29c12be3ffb33c/src/wp-includes/ms-site.php#L828-L866

to get this issue moving, opened up a PR to use the built in WP FS system functions

https://github.com/WordPress/wordpress-develop/pull/3951

This ticket was mentioned in PR #4172 on WordPress/wordpress-develop by @carl-alberto.


15 months ago
#9

  • Keywords has-patch has-unit-tests added

Update the wp_uninitialize_site function to delete the uploads folder and custom db tables created by plugins

Trac ticket: https://core.trac.wordpress.org/ticket/43162

#10 @SergeyBiryukov
12 months ago

  • Milestone changed from Awaiting Review to 6.3

Found this ticket after a coding session with @aristath and @poena, where we noticed that wp_uninitialize_site() calls unlink() and rmdir() directly, instead of using the Filesystem API. Fixing that seems like a good first step here.

This particular code appears to be introduced in mu:1069.

Last edited 12 months ago by SergeyBiryukov (previous) (diff)

@carl-alberto
12 months ago

Trying to add a patch

#11 @carl-alberto
12 months ago

Patch would be using the Filesystem API to delete the associated folder related to the subsite being deleted, deletes all the related tables for that subsite and it also contains the unit test. Hopefully I did it correctly :)

This ticket was mentioned in Slack in #core by carl-alberto. View the logs.


12 months ago

This ticket was mentioned in PR #4534 on WordPress/wordpress-develop by @carl-alberto.


12 months ago
#13

Patch would be using the Filesystem API to delete the associated folder related to the subsite being deleted, deletes all the related tables for that subsite and it also contains the unit test

Trac ticket: https://core.trac.wordpress.org/ticket/43162

This ticket was mentioned in PR #4537 on WordPress/wordpress-develop by @carl-alberto.


12 months ago
#14

fixing the orphaned uploads folder

https://core.trac.wordpress.org/ticket/43162

This ticket was mentioned in PR #4537 on WordPress/wordpress-develop by @carl-alberto.


12 months ago
#15

fixing the orphaned uploads folder

https://core.trac.wordpress.org/ticket/43162

This ticket was mentioned in PR #4538 on WordPress/wordpress-develop by @carl-alberto.


12 months ago
#16

fixing the failing php unit test for multisite

https://core.trac.wordpress.org/ticket/43162

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


11 months ago

#18 @spacedmonkey
11 months ago

  • Milestone changed from 6.3 to 6.4

As it is unclear which PR we should be reviewing here, I am going to punt to 6.4, when we have more time to look through code submitted.

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


9 months ago

#20 @oglekler
9 months ago

  • Keywords needs-patch added; 2nd-opinion needs-testing has-patch has-unit-tests removed

Hi @carl-alberto, this ticket was discussed during the bug scrub, and we noticed many PRs and most are closed and it is confusing, please, if you can, make a new PR with all changes including Unit tests. And if you don't have a capacity to work on it now, please, tell like it is, possibly someone else will be able to pick up the work. Thank you!

Add props to @mukesh27

Last edited 9 months ago by oglekler (previous) (diff)

This ticket was mentioned in PR #5066 on WordPress/wordpress-develop by @carl-alberto.


9 months ago
#21

  • Keywords has-patch added; needs-patch removed

When you delete a WP mutisite subsite, it orphans the DB and relative uploads folder. Becomes problematic on huge multisites leaving a lot of db tables and wasted space

Trac ticket: https://core.trac.wordpress.org/ticket/43162

#22 @carl-alberto
9 months ago

  • Keywords needs-patch added; has-patch removed

Thanks for the follow-up @oglekler, I have added another attempt on this patch in this PR https://github.com/WordPress/wordpress-develop/pull/5066

#23 @oglekler
9 months ago

  • Keywords has-patch needs-testing added; needs-patch removed

This ticket was mentioned in Slack in #core-test by oglekler. View the logs.


9 months ago

#25 @carl-alberto
9 months ago

Not sure if I made it correctly but I replaced the deletion method to use the FilesystemAPI, when I tested the deletion of WP multisite, it now deletes the orphaned folders but unit tests fails so it might need to be updated too?

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


8 months ago

This ticket was mentioned in Slack in #core-test by oglekler. View the logs.


8 months ago

#28 @oglekler
8 months ago

@carl-alberto yes, it looks like it, tests are failing to find directories what are searching for.

#29 @oglekler
8 months ago

  • Keywords changes-requested added; needs-testing removed
  • Milestone changed from 6.4 to 6.5

Because the patch needs a bit of work, and we are in two days before the Beta 1, I am moving this ticket into 6.5. It can go in trunk as soon as the patch will be ready and trunk open again after 6.4 will be branched out.

#30 @carl-alberto
8 months ago

ok thanks! I may be needing help with the unit test for this one if that is the case for the 6.5 release

#31 @janthiel
6 months ago

Hey there, just came along this ticket by chance.

I would like to add one suggestion for the sake of current work on the sqlite front: https://make.wordpress.org/core/2022/09/12/lets-make-wordpress-officially-support-sqlite/

Although I am not affiliated with the sqlite work or Plugin in any way ;-).

Could you please make the table selection query filterable? There is no information_scheme table in sqlite. So the hard coded MySQL based query would break WP on any db where the information_scheme table does not exist for whatever reasons.

Luckily easy to fix with a filter.

Unrelated to that: Thank you very much for picking up this issue and providing a solution. Highly appreciated!

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


3 months ago

#33 @audrasjb
3 months ago

  • Milestone changed from 6.5 to Future Release

As per today's bug scrub:
Since the ticket wasn't updated since 6.4, it won't be ready to ship in 6.5.
Moving to Future Release as beta 1 is tomorrow. Feel free to move it to 6.6 once a new patch is proposed.

#34 @christopherplus
3 months ago

I was ready to ask if this will be included in 6.5 version as it's an important bug. Hope to see it in the 6.6 or even in a 6.5.* release.

Note: See TracTickets for help on using tickets.