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

Allow removing all data on uninstall #3210

Closed
swissspidy opened this issue Sep 9, 2019 · 29 comments · Fixed by #6422
Closed

Allow removing all data on uninstall #3210

swissspidy opened this issue Sep 9, 2019 · 29 comments · Fixed by #6422
Assignees
Labels
Changelogged Whether the issue/PR has been added to release notes. Enhancement New feature or improvement of an existing one Groomed P1 Medium priority WS:Core Work stream for Plugin core
Milestone

Comments

@swissspidy
Copy link
Collaborator

As reported in the forums: https://wordpress.org/support/topic/how-to-delete-plugin-including-data-left-behind/

When uninstalling the plugin, all the AMP options and custom post types are still in the database.

Suggestion:

  1. Add checkbox to AMP settings screen to enable data removal upon uninstall
  2. Use register_uninstall_hook or uninstall.php to remove all custom post types, taxonomies, and options upon uninstall.
@swissspidy swissspidy added the Enhancement New feature or improvement of an existing one label Sep 9, 2019
@westonruter
Copy link
Member

westonruter commented Sep 9, 2019

There's also the postmeta for controlling whether AMP is enabled for a given post.

Is the checkbox necessary? Doesn't uninstallation entail data removal? I'm not up on the best practices for plugin uninstallation.

@westonruter westonruter added the P2 Low priority label Sep 9, 2019
@swissspidy
Copy link
Collaborator Author

A checkbox is common in many plugins as it prevents accidental data removal, for example when one wants to update a plugin via uploading a ZIP file. Because https://core.trac.wordpress.org/ticket/9757 is not solved yet, it would erase all data when deleting the AMP plugin and uploading the ZIP file of an RC. Which would be a bit annoying.

@cryptochrome
Copy link

I can't comprehend why this wasn't built into the plugin from the get go. When you guys implement this, please also consider taking care of redirects (301) required after removing all AMP content. You don't want to leave your users with potentially thousands of 404's.

@westonruter
Copy link
Member

@cryptochrome What 404s are you referring to? Are you referring to /amp/ endpoint URLs? There would be no way for the AMP plugin to redirect those URLs if it is deactivated. WordPress has no persistent URL redirect system that it could write to upon deactivation.

The issue of /amp/ URLs 404ing will be mitigated by away from URL endpoints in favor of the ?amp=1 query var per #2204.

@westonruter
Copy link
Member

@cryptochrome To redirect /amp/ URLs to non-AMP URLs after plugin deactivation, you can use this plugin: https://gist.github.com/westonruter/b7eb9cc7648b9a8b94b0015c79c8702e

@westonruter
Copy link
Member

Something that comes to mind with this issue is that if someone wants to try a pre-release build of the AMP plugin, currently the way to do that is to download a pre-release ZIP, uninstall the AMP plugin, and install the pre-release AMP plugin to then activate. However, if the uninstall process deletes data, then this would not work anymore.

@swissspidy
Copy link
Collaborator Author

@westonruter That‘s why I mention the checkbox. Uninstall would only run if the user enables it.

@swissspidy
Copy link
Collaborator Author

Something that comes to mind with this issue is that if someone wants to try a pre-release build of the AMP plugin, currently the way to do that is to download a pre-release ZIP, uninstall the AMP plugin, and install the pre-release AMP plugin to then activate.

This is fixed in WordPress 5.5. Updating via upload is now possible.

Time to unblock?

@westonruter
Copy link
Member

Yes, let's do it in v2.1.

@westonruter westonruter added this to the v2.1 milestone Aug 4, 2020
@kmyram kmyram added the WS:Core Work stream for Plugin core label Aug 5, 2020
@kmyram
Copy link

kmyram commented Aug 18, 2020

This needs an Implementation Brief or similar to that effect

@westonruter
Copy link
Member

westonruter commented Aug 24, 2020

Implementation brief:

Add an uninstall.php which does what @swissspidy outlined in the support topic.

  1. Remove AMP settings data: wp option delete amp-options
  2. Remove all amp_validated_url posts: wp post delete $(wp post list --post_type='amp_validated_url' --format=ids)
  3. Remove all amp_validation_error terms: wp term list amp_validation_error --field=term_id | xargs wp term delete amp_validation_error

We could also delete all transients associated with the plugin, when an external object cache is not being used.

See also Uninstall Methods in the Plugin Handbook.

@westonruter westonruter added the P1 Medium priority label May 3, 2021
@amanijoseph87
Copy link

Can you help me how to remove this data from my site, i have try to delete some table on my database without success, is there anyway we can use while we wait..

@westonruter
Copy link
Member

@amanijoseph87 This plugin does not create any database tables, at least not yet. Are you sure you're using this AMP plugin?

@amanijoseph87
Copy link

Am using this plugin, even on board wizard display blank.

@westonruter
Copy link
Member

@amanijoseph87 OK, please open a support topic on the forum. Our team will be able to better help you there: https://wordpress.org/support/plugin/amp/#new-topic-0

@amanijoseph87
Copy link

And settings page display blank too nothing seems to work

@amanijoseph87
Copy link

@amanijoseph87
Copy link

Is there a way to reset this plugin as delete all data related to this plugin

@westonruter
Copy link
Member

That link takes me to a 404 page. Please open a topic here: https://wordpress.org/support/plugin/amp/#new-topic-0

We'll see it once you've posted it.

Otherwise, you can follow the manual steps outlined in #3210 (comment) until we add an uninstall.php.

@amanijoseph87
Copy link

( https://wordpress.org/support/?post_type=topic&p=14395574 ) Is held for moderation i don't know way

@westonruter
Copy link
Member

OK, we'll follow up with you there then.

@amanijoseph87
Copy link

Please do now i know you guys a busy and if its not now i can not reach you in other time

@westonruter
Copy link
Member

We'll continue this on the support forum.

@westonruter
Copy link
Member

⚠️⚠️⚠️ This will need careful QA since it involves deletion of data! ⚠️⚠️⚠️

See how I initially tested during review in #6422 (review).

@westonruter
Copy link
Member

Follow-up discussion on whether there should ba a mechanism for explicitly opting-in to deleting data: GoogleForCreators/web-stories-wp#8453

We could add a new option on the settings screen which is disabled by default to ”Delete all plugin data upon uninstallation“. This could be disabled by default if we want to be extra cautious. If a user uninstalls the plugin and finds the data wasn't removed, they could re-install and then enable that option and re-uninstall.

Nevertheless, the data being removed (e.g. validated URLs) is much less important than the data being stored by Web Stories.

@bartoszgadomski
Copy link
Contributor

QA passed:

Only AMP-related data has been removed from database; no AMP-related data left in database after removal. Attaching "before.sql" and "after.sql" DB dumps that I used to compare: db-dumps.zip

Screenshot 2021-12-8 at 16 30 16
Screenshot 2021-12-8 at 16 30 05
Screenshot 2021-12-8 at 16 29 41
Screenshot 2021-12-8 at 16 32 24
Screenshot 2021-12-8 at 16 32 55

@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes. Enhancement New feature or improvement of an existing one Groomed P1 Medium priority WS:Core Work stream for Plugin core
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants