Feature/killswitch

[NickGreen]

Synopsis

Simplified version of the centralized settings here: #24

The point of this is to get an MVP up and running, with the most important feature, and then we can discuss and add the "Swiss Army Knife" features later.

To Test

• This plugin is activated on opsoasis: https://github.com/a8cteam51/team51-autoupdate-settings
• The settings page is here, with a toggle switch: https://opsoasis.wpspecialprojects.com/wp-admin/tools.php?page=team51_autoupdate_settings_options
• The API endpoint is here: https://opsoasis.wpspecialprojects.com/wp-json/custom/v1/get_autoupdate_settings/

When that toggle switch is toggled on, then all autoupdates should stop happening, and a notice should appear on the top of the plugins page.

Details

• Caching: The settings are cached, causing the killswitch to not immediately take effect. This is likely because the settings are retrieved when the object is created. However, I also tried it by retrieving the settings every single time the update filters ran, and it still seemed to not take effect immediately. I can deactivate/reactivate the Plugin Autoupdate Filter plugin to get it to register changes to the settings. Any thoughts on how to do this better are encouraged.
• Naming of the killswitch: as is being discussed in the other PR, I'm still waffling over the naming of the killswitch being confusing with the boolean return for the autoupdate filter itself. (i.e. Killswitch is "on" and return is "false")

Any other feedback is helpful, with the goal of getting out a minimum viable product that includes the killswitch, and we can add more features later :)

[NickGreen]

Example of a similar UI feature in another plugin

[ahegyes]

The settings are cached, causing the killswitch to not immediately take effect.

Where are they cached exactly? Do you mean the nginx cache for https://opsoasis.wpspecialprojects.com/wp-json/custom/v1/get_autoupdate_settings/? For that we'd need to run wp_cache_flush on OpsOasis whenever a setting changes. Or maybe there is a more specific function provided by batcache to only flush that one URL...

However, I think we should also include a transient cache after retrieving the setting. Even if just a 5 minute one ... to prevent a high-traffic site potentially bringing down OpsOasis if something goes wrong with the caching ...

[ahegyes]

The setting name is extremely verbose 😅 I don't think we need the team51_autoupdate_settings_ prefix for every setting given that it would apply to all the settings and we're retrieving them from a special endpoint already mentioning this. Ultimately it doesn't matter but these very long names are likely to lead to bug introduced by typos 😕

[NickGreen]

@ahegyes

Where are they cached exactly?

The problem isn't on the opsoasis side of things; the wp-json endpoint is updating correctly and quickly each time.

The lag is happening on the site where the Plugin Autoupdate Filter code is running. I think what is happening is that when an instance of class Plugin_Autoupdate_Filter is initialized, that object persists for a while. Since the settings are pulled in when the object is created, then whatever settings were current at the time the object was created are used. Does that make sense?

[NickGreen]

@ahegyes

Updates

1. New endpoint is here: https://opsoasis.wpspecialprojects.com/wp-json/wpcomsp/autoupdate-plugin/v1/settings/
2. Error handling discussion in Feature/killswitch #26 (comment)
3. Caching discussion in Feature/killswitch #26 (comment)

[ahegyes]

The lag is happening on the site where the Plugin Autoupdate Filter code is running. I think what is happening is that when an instance of class Plugin_Autoupdate_Filter is initialized, that object persists for a while. Since the settings are pulled in when the object is created, then whatever settings were current at the time the object was created are used. Does that make sense?

Hmmm ... unfortunately, it does not make sense 😢 Unless cached in the object cache, PHP objects don't persist past their current request unlike objects running on a Java Tomcat or ASP.NET server.

I also checked the code of wp_remote_get to make sure that it doesn't do any caching and I couldn't find anything 😕

I haven't tested this to confirm but if you think the plugin is somehow using "old" settings even though OpsOasis has updated, then we need to investigate exactly why 😢 I still think some batcache feature would be the culprit...

I still think we should incorporate a ~5 minute transient ourselves for the settings so we only get a maximum of ~1000 requests every 5 minutes to OpsOasis (roughly the number of sites we run the plugin on) instead of any number of requests proportional to the number of visitors that our managed sites get, but that's a separate conversation 😄

[NickGreen]

@ahegyes

1. I've reworked the code to match your error handling paradigm
2. I figured out the caching problem - it was on opsoasis! I fixed it by clearing the object cache when that specific option is saved :)

The latest version of the settings plugin is on opsoasis and this branch of Plugin Autoupdate Filter can be loaded on any site to test.

[ahegyes]

@NickGreen

I figured out the caching problem - it was on opsoasis! I fixed it by clearing the object cache when that specific option is saved :)

That makes a lot of sense! Especially since my curl tests were showing that the endpoint was being cached ... glad this was figured out 😄

I've reworked the code to match your error handling paradigm

I'm still not a fan of the fact that error handling happens in two places (lines 85 and 26 when just 26 would be enough) nor of the fact that settings can still have two separate forms, but I agree that this is much better and that it works. Happy to approve it as-is.

Big fan of the renaming of the class methods though!

[NickGreen]

@ahegyes Thank you for your persistence, sorry to make you look through this so many times. I'm definitely slowly understanding your suggestions fully, without just copy/pasting them.

I've updated the code, and I'm hoping you'll have time to look at it again. No hurry, BTW :)

The only odd thing that I wasn't able to understand fully was how, when the JSON is empty, line 88 would not return an object, so starting on line 91, I had to set it as an object so that the function would return an object. Maybe that's not a big deal; it certainly works as-is.

[ahegyes]

@NickGreen Very big fan of the current iteration! 😄 I left a few comments but 👍 from me.

[NickGreen]

OK, I've tested this locally to ensure that autoupdates still do work as intended.

I've also loaded version 1.5.0 (the version with the killswitch functionality) on a single production site before we roll it out to all of them: https://americanperimetertrail.org/wp-admin/plugins.php

I'll check back in this week to ensure that it's not causing any problems, then will probably merge this to trunk, which will cause all of the existing copies of this plugin to autpopdate themselves.

This also means that we can no longer mess with the opsoasis killswitch without it affecting prod sites.

[ahegyes]

This also means that we can no longer mess with the opsoasis killswitch without it affecting prod sites.

The worst that can happen is that auto-updates will stop, right? 😅 Because the plugin won't be able to pull in the settings and will default to ... pausing, no?

[NickGreen]

Ha, that's true. I'll roll this out today 👍