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

Optimize : Plugin uninstallation with data #6826

Closed
milindmore22 opened this issue Jan 10, 2022 · 12 comments · Fixed by #6840
Closed

Optimize : Plugin uninstallation with data #6826

milindmore22 opened this issue Jan 10, 2022 · 12 comments · Fixed by #6840
Labels
Bug Something isn't working Changelogged Whether the issue/PR has been added to release notes. P0 High priority
Milestone

Comments

@milindmore22
Copy link
Collaborator

milindmore22 commented Jan 10, 2022

Bug Description

We have been receiving support topics mentioning failing to delete data while uninstalling the plugin

  1. https://wordpress.org/support/topic/delete-amp-plugin/
  2. https://wordpress.org/support/topic/error-on-uninstall-amp/
  3. https://wordpress.org/support/topic/can-not-uninstall-amp/
  4. https://wordpress.org/support/topic/cant-delete-amp-plugin/
  5. https://wordpress.org/support/topic/amp-uninstall/
  6. https://wordpress.org/support/topic/does-not-allow-to-uninstall-amp/
  7. https://wordpress.org/support/topic/how-to-delete-the-amp-plugin/
  8. https://wordpress.org/support/topic/cant-uninstall-the-amp-plugin/

I am not able to reproduce the issue, but I doubt that cause maybe the timeout error failing to delete 1000 posts and taxonomies one by one, as in some cases there are plugins that have heavy processing or 50+ plugins on a site creating large number of invalidation record.

Please check the possibility of optimizing the uninstallation process (500 posts?), we can consider executing SQL queries, instead of using wp_delete_post and wp_delete_term

Expected Behaviour

Hassle-free Uninstallation process 💯

Screenshots

No response

PHP Version

7.4

Plugin Version

2.2.0

AMP plugin template mode

Transitional, Reader

WordPress Version

5.8.2

Site Health

ampwp-45d67ac7-6236-5036-b88d-8d797b7833cc

Gutenberg Version

12.2.0

OS(s) Affected

all

Browser(s) Affected

all

Device(s) Affected

all

Acceptance Criteria

No response

Implementation Brief

No response

QA Testing Instructions

No response

Demo

No response

Changelog Entry

No response

@milindmore22 milindmore22 added the Bug Something isn't working label Jan 10, 2022
@westonruter westonruter added the P0 High priority label Jan 10, 2022
@westonruter westonruter added this to the v2.3 milestone Jan 10, 2022
@westonruter
Copy link
Member

I wonder if there are plugins that are trying to purge some caches after each wp_delete_post() call. If every call is causing a the site's page cache to be purged, then that could be why cause of the problem.

Please check the possibility of optimizing the uninstallation process (500 posts?), we can consider executing SQL queries, instead of using wp_delete_post and wp_delete_term

A challenge here is that the object cache can be left populated with data, but better to have stale cache than having data permanently left in the DB.

We could also consider obtaining the current max_execution_time and then keep track of how long it is taking us to delete data. If we're approaching 50% of the total max_execution_time then we could bail running the wp_delete_post() and wp_delete_term() calls to instead.

Another option would be to obtain all the IDs that we need to delete, then to do a SQL batch DELETE, and finally pass each ID into clean_post_cache() and clean_term_cache().

@westonruter
Copy link
Member

Another thought is that we could call wp_suspend_cache_invalidation( true ) at the beginning before deleting everything, and then wp_suspend_cache_invalidation( false ) at the end. But this assumes other plugins are checking the $_wp_suspend_cache_invalidation when the delete_post and delete_term actions fire.

@westonruter
Copy link
Member

@milindmore22 Something to check is if users get the same error when they have the “uninstall at delete” toggle turned off in the settings. Is the issue the removal of data or that the plugin has a large number of files?

@milindmore22
Copy link
Collaborator Author

The user confirmed that by toggling off he is able to delete the AMP plugin.

@westonruter
Copy link
Member

@milindmore22 Have you been able to identify a common factor across users who have data deletion failures? Do they have a caching plugin in common, for example?

@milindmore22
Copy link
Collaborator Author

I have checked three site info, available with us, they have very few similarities

ampwp-45d67ac7-6236-5036-b88d-8d797b7833cc ampwp-71a5b4c4-ff59-5e3c-b3a9-5aa95ef33186 ampwp-6fa79782-72ca-53ba-8f6d-c3893dd569a5
WP Super Cache WP Rocket WP Super cache
Gutenberg with Classic 🤷🏼 Beaver Builder Elementor
------------- UpdraftPlus UpdraftPlus
WooCommerce ---------- WooCommerce
Rank Math SEO Rank Math SEO Rank Math SEO

I have tested, Rank Math SEO, WooCommerce, and Updraftplus plugins and didn't encounter any issue while deleting the AMP plugin.

@westonruter
Copy link
Member

westonruter commented Jan 13, 2022

Let's go ahead with just doing the SQL DELETE queries.

In looking at what WooCommerce does, they do SQL DELETE for posts and taxonomy terms, and then at the very end they do wp_cache_flush(). The flushing of the object cache at the end is the missing piece that had me hesitating.

@ghost
Copy link

ghost commented Jan 18, 2022

Hey, I am also facing this issue. Cannot delete the plugin until I turn off that delete all data button. But why I am writing here is because AMP left some children somewhere and cannot find where to remove it.

This is the Notice I get from RankMath : SEO Notice: A previously published term has been moved to trash. You may redirect https://example.com/?taxonomy=amp_validation_error&term=a6703459abe929936dadcce26e4f7fe4

Where do I find this to delete it? It really bothers me that notification.

Here is the Support UUID : ampwp-d7974cae-5b93-5bb9-9820-74df0166b945

Thank you in advance

@milindmore22
Copy link
Collaborator Author

Hello @rfseapp We are working on optimizing the delete data process, which is likely to release with 2.2.1, meanwhile, you can try and use the WP CLI commands or SQL queries mentioned in this reply (Please use those SQL queries at your own risk, also once you execute those queries please clear Site cache and Object Cache)

@westonruter
Copy link
Member

westonruter commented Jan 25, 2022

In addition to checking the uninstallation visually via the admin, here's a good way to QA the uninstallation by examining the result of the specific SQL DELETE queries being run:

wp db export - --extended-insert=FALSE  > /tmp/before-uninstall.sql
# Now do uninstall.
wp db export - --extended-insert=FALSE  > /tmp/after-uninstall.sql
diff /tmp/before-uninstall.sql /tmp/after-uninstall.sql  | cut -c1-200 | egrep '^<'

The resulting diff will show the rows that have been deleted from the database.

@delawski
Copy link
Collaborator

QA Passed

The AMP-related data is successfully removed after deleting the plugin.

I've set up a set of test posts and activated several plugins that cause AMP incompatibilities. Then, I've validated a bunch of posts.

The DB dump before uninstalling had over 2000 lines. The dump done after the uninstall had almost 1600 lines which means that AMP related data has been removed.

I've also reviewed the after-uninstall DB dump manually and could find a few remaining occurrences of amp_validated_url string. They were part of the rewrite rules and the admin screen layout metadata which I guess is expected.

@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Feb 3, 2022
@ocha1

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Changelogged Whether the issue/PR has been added to release notes. P0 High priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants