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

Remove scheduled events upon deactivation or uninstall #6992

Closed
2 tasks done
jamesozzie opened this issue Apr 28, 2023 · 11 comments
Closed
2 tasks done

Remove scheduled events upon deactivation or uninstall #6992

jamesozzie opened this issue Apr 28, 2023 · 11 comments
Labels
P1 Medium priority PHP Team M Issues for Squad 2 Type: Enhancement Improvement of an existing feature Type: Support Support request

Comments

@jamesozzie
Copy link
Collaborator

jamesozzie commented Apr 28, 2023

Bug Description

As suggested by a user in the forums, consider allowing users to remove any cron events upon via a hook, or an option to remove such events upon uninstall or deactivation.

The cron event highlighted by the user is googlesitekit_cron_update_remote_features


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • All Site Kit cron events and schedules should be cleared on:
    • Site Kit Reset
    • Plugin deactivation
    • Plugin uninstall

Implementation Brief

  • Following events need to be cleared (these are present at the time of writing, check if there are new ones during execution):
    • OAuth_Client::CRON_REFRESH_PROFILE_DATA
    • Remote_Features_Cron:CRON_ACTION
    • Synchronize_AdSenseLinked::CRON_SYNCHRONIZE_ADSENSE_LINKED
    • Synchronize_AdsLinked:CRON_SYNCHRONIZE_ADS_LINKED
    • Synchronize_Property:CRON_SYNCHRONIZE_PROPERTY
  • Update includes/Core/Util/Uninstallation.php
    • In register method, hook into googlesitekit_deactivation and googlesitekit_reset and remove all listed CRON events. You can use wp_clear_scheduled_hook.
    • Extract the CRON cleanup logic in a new method and call it as callback in both hooks. And invoke it in the existing googlesitekit_uninstallation hook as well

Test Coverage

  • No updates needed

QA Brief

  1. Turn on the conversionReporting feature flag (as that includes one of the cron events).
  2. Set up Site Kit with the Analytics, Ads, and AdSense modules.
  3. Install & activate the WP Crontrol plugin.
  4. Go to "WordPress Dashboard -> Tools -> Cron Events" and search for "googlesitekit".
  5. You should be able to see some events from the Site Kit plugin here.
  6. Now, try deactivating, deleting, or resetting Site Kit.
  7. Perform step 4 again and verify that there are no Site Kit events.

Changelog entry

  • Remove scheduled events upon deactivation, reset or uninstall.
@jamesozzie jamesozzie added Type: Support Support request Type: Enhancement Improvement of an existing feature labels Apr 28, 2023
@jamesozzie
Copy link
Collaborator Author

@aaemnnosttv tagging you here for visibility.

@aaemnnosttv aaemnnosttv changed the title Allow removal of Cron Events upon deactivation or uninstall Remove scheduled events upon deactivation or uninstall May 8, 2023
@aaemnnosttv aaemnnosttv self-assigned this May 8, 2023
@aaemnnosttv aaemnnosttv added the P2 Low priority label May 8, 2023
@zutigrm zutigrm mentioned this issue Sep 11, 2023
18 tasks
@ivonac4
Copy link
Collaborator

ivonac4 commented Jan 8, 2024

@aaemnnosttv Are you still planning to work on this soon? If not, could you please unassign yourself so someone else can pick it up? Thank you!

@aaemnnosttv
Copy link
Collaborator

Closing in favor of #8988

@aaemnnosttv aaemnnosttv closed this as not planned Won't fix, can't repro, duplicate, stale Jul 8, 2024
@aaemnnosttv aaemnnosttv removed their assignment Jul 8, 2024
@webd-uk
Copy link

webd-uk commented Jul 9, 2024

I will comment on #8988 as well but I do not agree that this should be closed in favour of that issue.

As per WordPress WP Cron recommendations, tasks should be unscheduled when they are no-longer required and doing so on a deactivation hook (as recommended by WordPress) cannot have a detrimental effect because the plugin is no longer active!

Should the plugin be re-activated, the missing / required tasks will be added again by the plugin anyway.

@aaemnnosttv
Copy link
Collaborator

Thanks @webd-uk, I agree, this issue is still valid 👍

@aaemnnosttv aaemnnosttv reopened this Jul 10, 2024
@aaemnnosttv aaemnnosttv self-assigned this Jul 16, 2024
@aaemnnosttv aaemnnosttv added PHP P1 Medium priority and removed P2 Low priority labels Jul 16, 2024
@aaemnnosttv aaemnnosttv removed their assignment Jul 16, 2024
@zutigrm zutigrm self-assigned this Jul 29, 2024
@zutigrm zutigrm removed their assignment Aug 7, 2024
@eugene-manuilov eugene-manuilov self-assigned this Aug 7, 2024
@eugene-manuilov
Copy link
Collaborator

Update google-site-kit.php

  • Include register_uninstall_hook function, and re-use the googlesitekit_deactivate_plugin as callback. You can invoke this function bellow the register_deactivation_hook, since uninstalling and deactivating can share the same cleanup logic.

@zutigrm it should be done in the includes/Core/Util/Uninstallation.php file.

Update Google\Site_Kit\Plugin::register:

  • Hook into googlesitekit_deactivation and googlesitekit_reset and remove all listed CRON events.

The same here, we should do it in the Uninstallation class.

@zutigrm
Copy link
Collaborator

zutigrm commented Aug 8, 2024

@eugene-manuilov Thanks! Didn't see we have that file. IB Updated

@zutigrm zutigrm assigned eugene-manuilov and unassigned zutigrm Aug 8, 2024
@eugene-manuilov
Copy link
Collaborator

Thanks. IB ✔️

@eugene-manuilov eugene-manuilov removed their assignment Aug 8, 2024
@binnieshah binnieshah added the Next Up Issues to prioritize for definition label Aug 14, 2024
@ivonac4 ivonac4 added Team M Issues for Squad 2 and removed Next Up Issues to prioritize for definition labels Sep 2, 2024
@nfmohit nfmohit self-assigned this Sep 3, 2024
@nfmohit nfmohit removed their assignment Sep 5, 2024
@benbowler benbowler assigned benbowler and unassigned benbowler Sep 5, 2024
@techanvil techanvil self-assigned this Sep 9, 2024
techanvil added a commit that referenced this issue Sep 9, 2024
…ed-events

Clear all scheduled events on deactivation/reset.
@techanvil techanvil removed their assignment Sep 9, 2024
@kelvinballoo
Copy link
Collaborator

QA Update ⚠️

Hi @nfmohit , when I tested for the reset scenario (regardless if it's through the SK Admin Settings or the Tools > Available Tools menu), there is still one googlesitekit event left. ⚠️
Is that expected?

Screenshot and video attached below.

Screenshot 2024-09-12 at 13 14 57
reset.site.kit.cron.-.1.remaining.mov

Other than that, deactivation and deletion scenarios are working as expected. ✅

Plugin.deactivation.-.all.sk.events.gone.mov
Plugin.deletion.-.all.events.are.gone.mov

@nfmohit
Copy link
Collaborator

nfmohit commented Sep 12, 2024

Hi @kelvinballoo. This is expected because the googlesitekit_cron_update_remote_features event is cleared as expected, but as the Site Kit plugin is active, it re-schedules this event again. Thanks!

@nfmohit nfmohit removed their assignment Sep 12, 2024
@kelvinballoo
Copy link
Collaborator

QA Update ✅

Thanks for the confirmation @nfmohit.
This is good to be moved to approval in that case.

  • Resetting, deactivation and deletion scenarios are working as expected. ✅

Note that for 'Reset' scenario, there will be one event left and that is expected.

Reset (tested reset from SK admin settings and also Tools > Available Tools):
reset.site.kit.cron.-.1.remaining.mov

Deactivation:

Plugin.deactivation.-.all.sk.events.gone.mov

Deletion:

Plugin.deletion.-.all.events.are.gone.mov

@kelvinballoo kelvinballoo removed their assignment Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Medium priority PHP Team M Issues for Squad 2 Type: Enhancement Improvement of an existing feature Type: Support Support request
Projects
None yet
Development

No branches or pull requests