Feature/plugin denylist #24

NickGreen · 2024-02-17T21:15:23Z

Note: this PR is in progress, and shouldn't be merged before refactoring and testing. This PR is mainly for discussion and reviewing of the concept.

Synopsis

Uses a json file for centralized settings. This is "Step 1" of this rough idea: #16 (comment)

Reviewing

Have a look at the centralized_settings.json file to see what we are setting.
Look at how we are pulling in and parsing the JSON to see if that looks right
Look at each part of the code that references one of the centralized settings

class-plugin-autoupdate-filter.php

ahegyes · 2024-02-19T11:41:34Z

class-plugin-autoupdate-filter.php

+	public function auto_update_pause_all( $update, $item ) {
+
+		if ( isset( $this->settings['pause_all'] ) && true === $this->settings['pause_all'] ) {
+			return false;


This is definitely a nitpick but thought maybe worth bringing up. Based on the function's name, auto_update_pause_all I would expect a return value of true to mean that auto updates will be paused. Instead, it's a value of false that does that.

It's a small cognitive load but it always makes me stop and check the calling function to ensure that it's all correct and not a bug.

If the function was named should_auto_update_any, for example, then a return value of false pausing it all would be in tune and would make more sense.

Not a big deal in the grand scheme of things...

This is a very valid piece of feedback, and one that I spent time yesterday going back and forth on. Let's dig in a little bit:

I think that the UI being a "killswitch" is important. From a TAM's perspective, it needs to be a toggle that can be clicked which will stop all autoupdates, immediately. And I want the language around that to be very clear. You click it to stop autoupdates. (doesn't have to actually be named "killswitch" 😆 )

The auto_update_x filters need false for "don't update" and true for "update"

These two things are in a bit of conflict.

Maybe if the toggle were called "Allow any updates?" and the checked value could be "true." But the problem is that this could be confused with allowing all updates, which is way too confusing UI for my liking.

Or if we called it "Killswitch" with the checked value being "engaged." Then we avoid any comparison to true or on.

I don't know...

ahegyes · 2024-02-19T11:43:53Z

class-plugin-autoupdate-filter.php

+	 * Initialize WordPress hooks and load settings from JSON.
 	 */
 	public function init() {
+		$this->load_settings_from_json();


We need some sort of logic to stop the plugin from initializing here if loading the settings fails. Not likely with a local file, but definitely a bigger issue if we load them over HTTP later.

I haven't touched this one yet, because this seems like a design decision. If we can't reach the API for some reason, what would we want the default autoupdate behavior to be?

Maybe we would want all autoupdates to be paused until the API could be reached?

If we can't reach the API for some reason, what would we want the default autoupdate behavior to be?

I'd pause everything. If the API is down, we'll notice and get it fixed soon. Worst case scenario, auto-updates don't happen for a bit, and I think that's better than an update going through when it shouldn't have because the server was over-loaded or something...

ahegyes · 2024-02-19T11:45:10Z

class-plugin-autoupdate-filter.php

+			$json_content = file_get_contents( $json_path );
+			$settings     = json_decode( $json_content, true );
+			if ( is_array( $settings ) ) {
+				$this->settings = $settings;


Running this through wp_parse_args could simplify the code by not requiring isset checks throughout the file.

And it would be a self-documenting way of figuring out which options are supported.

Good suggestion! Will definitely do this when I get back to merging the local settings and centralized settings.

ahegyes · 2024-02-19T11:50:48Z

centralized_settings.json

+	"pause_periods": [
+	  {
+		"name": "woo_update_6_1",
+		"start": "2024-02-16 00:00:00",


Could I make an argument for using ISO 8601 as the date format? 😄 It's as unambiguous as it gets -- instantly recognizable, guaranteed to be parsed correctly by strtotime, and no question as to which timezone the string represents.

So, would that be changing 2024-02-16 00:00:00 to 2024-02-16T00:00:00+00:00 for example?

Or, setting them all to U.S. East coast timezone using the -04:00 suffix?

Yes, exactly! Sorry, I should've included those examples myself 😅 PHP calls this format ATOM which I think is pretty cool too;

If we get a timestamp from the setting, we can simply generate that string doing:

$time = new DateTime(); $time->setTimestamp( 1709548419 ); echo $time->format( DateTime::ATOM );

That would output 2024-03-04T10:33:39+00:00 right now.

ahegyes · 2024-02-19T12:07:17Z

centralized_settings.json

+	"pause_times": {
+	  "start": "10",
+	  "end": "23",
+	  "friday_end": "19"


I'm not sure that this is generic enough ... many countries have different weekend rules and thus a friday_end property doesn't always apply even though it fits our team's workflow: https://en.wikipedia.org/wiki/Workweek_and_weekend

Even the assumption that the weekend is adjacent days doesn't always hold: [...] Friday and Sunday (in [Brunei Darussalam](https://en.wikipedia.org/wiki/Brunei), [Aceh](https://en.wikipedia.org/wiki/Aceh) province (Indonesia) and state of [Sarawak](https://en.wikipedia.org/wiki/Sarawak) (Malaysia))

Also, I think that the name is confusing. At first, I think it meant times during which no updates took place but it means the exact opposite after inspecting the code, namely that updates happen only during this time.

I don't think accounting for how other countries do weekends is an important use of energy at this point, since probably only our team is using this at this time.

However, I agree the "pause times" is the wrong wording. It's backwards! It should probably be "update times."

ahegyes · 2024-02-19T12:15:48Z

class-plugin-autoupdate-filter.php

-		foreach ( $holidays as $holiday ) {
-			$start = $holiday['start'];
-			$end   = $holiday['end'];
+		$pause_periods = apply_filters( 'plugin_autoupdate_filter_holidays', $pause_periods ); // keep for backward compatibility


A use-case for https://developer.wordpress.org/reference/functions/apply_filters_deprecated/ maybe? 😄

Good tip! Will incorporate this one.

class-plugin-autoupdate-filter.php

ahegyes · 2024-02-19T13:06:25Z

What do you think about a JSON-schema like this:

{
    "pause_updates": false, // Global killswitch

    "updates_schedule": { // Work schedule of the agency, i.e. Team51 default availability
        "days": [ 1, 2, 3, 4, 5 ], // ISO 8601 day format where 1 is Monday and 7 is Sunday
        "hours": {
            "start": "10:00:00+00:00", // ISO 8601 time format for 10 AM UTC+0
            "end": { // Can be either a given time or an object specifying end time for each day
                "default": "23:00:00+00:00", 
                "5": "19:00:00+00:00" // Different end time for day 5, i.e. Friday
            }
        }
    },
    "pause_periods": [ // Array of start-end pairs with variables support
        {
            "name": "Christmas",
            "start":"Y-12-23T00:00:00+00:00", // ISO 8601 datetime format with support for dateTime variables like Y, m, d ... 
            "end": "Y-12-31T23:59:59+00:00"
        },
        {
            "name": "New Year",
            "start": "Y-01-01T00:00:00+00:00",
            "end": "Y-01-02T23:59:59+00:00"
        },
        {
            "name": "Wait on next WooCommerce release",
            "start": "2024-02-16T00:00:00+00:00",
            "end": "2024-02-20T23:59:59+00:00"
        }
    ],
    "pause_rules": { // Different custom rules for core, plugin, and theme updates.
        "core": [],
        "plugins": [
            {
                "slug": "*woocommerce*", // Support anything containing the slug woocommerce
                "start": "2024-02-16T00:00:00+00:00",
                "end": "2024-02-20T23:59:59+00:00"
            },
            {
                "slug": "woocommerce", // Matches woocommerce itself only
                "version": [ "8.6.0", ">=8.6.0 && <8.7", "8.6" ] // Skip updates if anything in this array matches. Support for simply boolean logic. 
            }
        ],
        "themes": []
    },

    "update_email": { // Custom config for the update email
        "core": {
            "to": [
                "concierge@wordpress.com"
            ]
        },
        "plugin_theme": {
            "to": [
                "concierge@wordpress.com"
            ]
        }
    },
    "debug_email": { // Custom config for the debug email
        "to": [
            "concierge@wordpress.com"
        ]
    }
}

I personally find the keys skip_exact_version, skip_version_higher_than_or_equal_to, and skip_version_lower_than a little verbose. If people generally prefer that though, then no objections from me, but we should at least add support for array values and for the other comparison operators like <= and >.

NickGreen · 2024-02-20T00:21:40Z

Thanks for taking a look! I will go in and address each of these in the morning :)

Question: what are your opinions on where these settings should live? My initial idea was to create a plugin on wpspecialprojects.com which has a settings page in wp-admin. Then we could either write the settings to a JSON file, or store it in the database and pull it out with an API call.

However, based on the suggested JSON format, the whole thing is potentially quite complicated for a GUI. And potentially too complicated for general TAM usage, if we are relying on direct editing of the JSON file.

ahegyes · 2024-02-20T11:17:06Z

what are your opinions on where these settings should live?

I don't think that a GUI would be too complicated necessarily. Actually, I think we might be able to build one relatively easily with MetaBox... We then compile the settings in a PHP array and output it as JSON.

I'm not sure if we should just output this on a REST API endpoint (wp-json/<something>) or create a custom endpoint added via a rewrite rule with the SHORTINIT constant set to true for performance; since we're returning basically a cacheable string, I think the former is enough and we let nginx caching worry about performance;

As for where it should live, what about OpsOasis? It's the central site/server we're building to turn the Team51 CLI into a thin client; everyone on the team will end up having an account there so it would make sense to be there especially if we might build a GUI; I made you an account now so you can check it out 😄

NickGreen added 2 commits February 17, 2024 10:45

Change "holidays" to "pause periods"

736298e

Pull settings from local JSON file

9aef97b

NickGreen mentioned this pull request Feb 17, 2024

Centralized "don't autoupdate" plugin list #16

Open

ahegyes reviewed Feb 19, 2024

View reviewed changes

class-plugin-autoupdate-filter.php Show resolved Hide resolved

ahegyes reviewed Feb 19, 2024

View reviewed changes

class-plugin-autoupdate-filter.php Show resolved Hide resolved

ahegyes reviewed Feb 19, 2024

View reviewed changes

class-plugin-autoupdate-filter.php Show resolved Hide resolved

NickGreen mentioned this pull request Feb 29, 2024

Feature/killswitch #26

Merged

Feature/plugin denylist #24

Are you sure you want to change the base?

Feature/plugin denylist #24

Uh oh!

Conversation

NickGreen commented Feb 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Synopsis

Reviewing

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NickGreen Feb 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NickGreen Feb 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ahegyes commented Feb 19, 2024

Uh oh!

NickGreen commented Feb 20, 2024

Uh oh!

ahegyes commented Feb 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

NickGreen commented Feb 17, 2024 •

edited

Loading

NickGreen Feb 29, 2024 •

edited

Loading

NickGreen Feb 29, 2024 •

edited

Loading