Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Uninstall distributor #540

Open
wants to merge 60 commits into
base: develop
Choose a base branch
from
Open

Fix: Uninstall distributor #540

wants to merge 60 commits into from

Conversation

dinhtungdu
Copy link
Contributor

@dinhtungdu dinhtungdu commented Mar 10, 2020

Description of the Change

  • Add an uninstall.php script to delete distributor data from the database.
  • Add DT_REMOVE_ALL_DATA constant to prevent accidental data deletion.
  • Deletes entries from the options table.
  • Do not delete posts and meta other than Subscriptions.

A modal will display when deactivating the plugin. This modal does not display in sub-sites.

image

Benefits

Delete distributor data on uninstalling.

Possible Drawbacks

n/a

Verification Process

Check the database manually.

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Applicable Issues

Closes #349

Changelog Entry

  • Added - Remove data on uninstall.

@jeffpaul jeffpaul added this to the 2.0.0 milestone Mar 10, 2020
@jeffpaul jeffpaul added the type:enhancement New feature or request. label Mar 10, 2020
@jeffpaul jeffpaul modified the milestones: 2.0.0, 2.1.0 Mar 26, 2020
@jeffpaul jeffpaul added the needs:discussion This requires discussion to determine next steps. label Mar 26, 2020
@jeffpaul
Copy link
Member

Moving this to 2.1.0 as we still need to discuss what items we can/should safely remove during uninstall.

@jeffpaul jeffpaul requested review from a team and iamdharmesh and removed request for a team June 7, 2023 15:59
@ravinderk
Copy link
Contributor

@peterwilsoncc, Can you provide a list of data we should remove when someone installs a plugin?

cc: @dkotter @jeffpaul

@ravinderk ravinderk self-assigned this Aug 4, 2023
@ravinderk ravinderk marked this pull request as ready for review August 4, 2023 12:48
@ravinderk ravinderk requested a review from a team as a code owner August 4, 2023 12:48
@ravinderk ravinderk requested review from peterwilsoncc and removed request for iamdharmesh and a team August 4, 2023 12:49
Copy link
Collaborator

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The data deletion function will leave a bunch of caches in place for posts and post meta that doesn't exist, so we'll need to get the post IDs to be deleted, delete them and flush the caches. This is because the individual posts & post meta don't use the last changed time.

As distributor makes use of site options and site transients, we'll also need to delete the options differently depending on the type of install. Since WordPress 6.6 the autoloading of options depends on the options size, so we'll need to check for additional values to determine if the option is cached in all options or as an individual option.

As my suggested changes are quite a lot, I've put together a gist with the items split out in to multiple functions to allow for the cache to be deleted.

https://gist.github.com/peterwilsoncc/286025a3ca49b90c4d4f2e06741396a2

Warning

Please review my code carefully and check the queries generated.

Much of the code was written by Copilot so could do with some tidy-up, for one. Secondly, it uses some WPDB methods that are quite rare so it would be good to have a logic check

If you think deleting the options via the option ID will be faster (it's the primary key, then feel free to update the code to do so.

includes/bootstrap.php Outdated Show resolved Hide resolved
includes/bootstrap.php Outdated Show resolved Hide resolved
includes/bootstrap.php Outdated Show resolved Hide resolved
includes/bootstrap.php Outdated Show resolved Hide resolved
uninstall.php Outdated Show resolved Hide resolved
uninstall.php Show resolved Hide resolved
uninstall.php Outdated
);

// Flush the options cache.
wp_cache_delete_multiple( $options_to_delete, 'options' );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is being called with the option ID but the cache uses the option_name

uninstall.php Outdated
);

// Flush the site options cache.
wp_cache_delete_multiple( $sitemeta_to_delete, 'site-options' );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is using the option ID for the cache names, but it's cached using the pattern $network_id:$option_name

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is using meta_id from the sitemeta table, do you mean we should pass $network_id:{sitemeta.meta_key}? Where we can use sitemeta.site_id as a $network_id. So something like this?:

wp_cache_delete_multiple( [Array of {sitemeta.site_id:sitemeta.meta_key}], 'site-options' );

uninstall.php Outdated Show resolved Hide resolved
uninstall.php Outdated Show resolved Hide resolved
@peterwilsoncc
Copy link
Collaborator

@jeffpaul This still needs a little more from the previous review: fixing the options cache IDs, site options and adding the line phpcs notes for each line.

@faisal-alvi
Copy link
Member

Correct @peterwilsoncc This was dropped to pick some other work. I will drop a comment once this is ready for review. Thanks!

@faisal-alvi
Copy link
Member

@peterwilsoncc I've added a few commits and included a couple of questions for confirmation. Could you please take a look at #540 (comment) and #540 (comment)?

@faisal-alvi
Copy link
Member

@peterwilsoncc I’m now retrieving both the option IDs and names. IDs to delete the records, and names to clear the caches.

For multisite, I’m using $network_id: sitemeta.meta_key for cache deletion. Let me know if any adjustments are needed.

Copy link
Collaborator

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good but some odd behaviour when I try to delete the plugin

  • the dist/css file can't be deleted.
  • The database queries on a single site were failing. Not sure why.

To test I've been running the release script and unzipping and activating a duplicate copy to make sure I don't delete the git repo.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File needs to be added to this block of code

patterns: [
{ from: 'readme.txt', to: './' },
{ from: 'README.md', to: './' },
{ from: 'CHANGELOG.md', to: './' },
{ from: 'composer.json', to: './' },
{ from: 'distributor.php', to: './' },
{ from: '.github/workflows/*', to: './' },
{ from: '.gitattributes', to: './' },
{ from: 'assets/img/*', to: './' },
{ from: 'dist/**/*', to: './' },
{ from: 'includes/**/*', to: './' },
{ from: 'lang/**/*', to: './' },
{ from: 'templates/**/*', to: './' },
{
from: 'vendor/yahnis-elsts/plugin-update-checker/**/*',
to: './',
},
],
} ),

@faisal-alvi
Copy link
Member

Hey @peterwilsoncc,

I tested again on both a multisite network and a single site, but I didn’t encounter any odd behavior when deleting the plugin.

  • The dist/css files were successfully deleted on my end (see the below GIF).
  • The database queries were functioning properly, and all entries in the options table were being deleted.

all-files-deleted-distributor

I also ran the release script and used the zip to test.

Can you please check again? Or @qasumitbagthariya if you have some time to test this, that would be great.

Copy link
Collaborator

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@faisal-alvi Looks like it was file permission error between my local and virtual environments. Once I ran chmod it worked as expected.

@jeffpaul
Copy link
Member

jeffpaul commented Dec 2, 2024

@qasumitbagthariya over to you for QA

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:feedback This requires reporter feedback to better understand the request. type:enhancement New feature or request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uninstall distributor (completely)
6 participants