Add handling for module updates

[JDGrimes]

This will basically consist of a merge of the WordPoints.org module, except that there will be some changes we want to make in the process.

We did not do this before because we thought it might conflict with the WordPress.org plugin guidelines, but now I have asked about it and gotten the go-ahead. The WordPress.org plugin team published a clarification of the guideline in question, which makes everything much clearer.

So let's do this!

[JDGrimes]

One thing that I am questioning is the way that we register all of the channels (remote module servers) right up front. The channel registry doesn't seem to be really needed, because there is only one channel class that is used for all of the channels right now. So I think it would be better to just forego the channel registry. Instead, what we'll do is make the wordpoint_get_channel_for_module() function return the channel object directly, instead of just the slug URL. That way we could introduce a registry in the future if needed. But right now it would just be taking up memory for no reason that I can see right now.

[JDGrimes]

I'm not sure what we should call everything. We could just keep all of the naming from the module of course, but this is the opportunity to change if we want to.

The term "channel" is pretty clever, but maybe it is too clever. Something like "repository", "directory", or "server", would be more obvious. I think that in this case, "server" makes the most since. It is a remote service that is serving modules to the world. So yeah, let's go with that.

[JDGrimes]

That change supersedes #286, which superseded #106. While working on this I noticed that we didn't update the value of the new Channel: header with the value of the deprecated Update API: header when it was present. I don't see any comment on this, but I suppose that it was because the Update API: header didn't specify that a URL was expected, just a generic unique identifier. However, for the switch from Channel: to Server:, we can set the server manually when the module specifies a channel.

[JDGrimes]

So what we were previously calling module update APIs would now be called module server APIs. They will implement different interfaces based on whether the API supports updates, browsing the directory, searching, etc. Basically there will be separate interfaces for each feature. And for now we'll just be mostly concerned with the updates interface.

There is one other feature that we deal with though, and that is licenses. I started wondering if there should be an interface for that. Even before that, I was thinking that we might want to make the license handling code more generic, instead of having it be a part of one of the API objects. But the problem is that I'm not entirely sure which features are generic to licenses. That is, I don't really know how license systems work beside EDD Software Licenses. For example, in EDDSL the licenses have to be activated for each site, but for other systems maybe they wouldn't. But maybe we just need a licenses interface and an activatable licenses interface.

[JDGrimes]

I'm also crafting an expirable license interface, and a renewable license interface.

The problem with the renewable license interface though, is that licenses from EDDSL can't be renewed automatically: the user has to go to the site and purchase the renewal. So it is different than just activating/deactivating a license programmatically via the API.

I have thought that maybe I should just add two interfaces, one for renewable licenses, and one fro manually renewable licenses. But really, in most every case the license will require manual renewal: automatic renewal doesn't really make sense. Unless, perhaps, the user has a credit card on file or something, so renewal will automatically take place when they trigger it, but the payment isn't automatic, it has to be manually initiated by that trigger. Then it would be possible to display a renewal button in the admin that renewed the license through the API.

So really, I guess that we should try not to make assumptions about how renewal APIs might work. Separate renewal and manual renewal interfaces are probably the best way to go.

Maybe the name "manual" renew isn't the best term though, since there could be several different kinds. In this case we are thinking specifically of licenses needing to be renewed through visiting a website, so we could have the interface provide a method to get the URL to visit to begin the renewal process. But in other cases other action might be required instead.

So perhaps the best thing would be to just provide a "renewal URL" interface, so that it is explicit what information it should provide and how it would be used.

[JDGrimes]

And I don't think that it is necessary to include an automatic renewal interface at this time, since if we did we would also probably need to actually support licenses that use it, and that would take extra work that isn't needed for us at the moment.

[JDGrimes]

EDDSL doesn't actually specifically make the renewal link available, but we can use the homepage of the module for now.

[JDGrimes]

When checking for updates, we now would just be getting the latest version via the API object, and then externally seeing whether that version was newer. Previously that was internal to the API object. The benefit of that was that it allowed APIs to use whatever kind of versioning system that they wanted to. If we do the version check externally, the APIs have to use a versioning system that will work properly with our check. So I guess the question is whether we want to support versioning schemes that do not work with version_compare().

And perhaps there would also be other assumptions inherent in the external checking.

As var as version_compare() itself goes though, I doubt that we really want to encourage non-semantic and strange versioning schemes for modules. Semantic versioning is pretty easy for users to understand, and any system that isn't even remotely similar to traditional version numbers will likely only cause confusion. So I'd say that if version_compare() couldn't process it, it is probably already broken.

[JDGrimes]

I was wondering why we check both the list of plugins that were checked before and the list of updates that were found, when checking if any plugins have changed. How could we have added potential updates to the list for modules that weren't checked? Our logic currently doesn't really make this possible, unless something else messes with the database directly. So at the most this seems like a sanity check for an edge-case.

This logic originated in wp_update_plugins(), via WP@19683. However, a look over WP#18876 did not reveal a particular reason for this dual check.

So I think I'll just remove that second part, and leave just the comparison with the modules that were previously checked for updates.

[JDGrimes]

Cron and Multisite

We are going to add a cron schedule to run the update check every so often. Of course, when the plugin is deactivated, the cron schedule needs to be deleted. This got me wondering how this would work on multisite, and whether we needed to remove the schedule for each site when the plugin was deactivated on multisite. Of course, that only applies when WordPoints is network-active, when it is per-site active, the point is moot.

The cron schedule is stored in a regular option, so it is different per-site on multisite. So when the plugin had been network-active, the schedule would need to be deleted for each site on deactivation.

However, this got me thinking that maybe on multisite the schedule only really needs to be registered once anyway. The update info itself will be stored in a network transient, so it will be common to the entire network. It doesn't seem then that there is really any reason for each site on a network to be checking for updates, since the same modules are common to the entire network and the update info is stored for the entire network. It makes no sense for 1000 sites on a network to be checking when only one check needs to be performed.

This leads to a new revelation though: that each site could have different modules active on it, which could potentially include code that affects the update routine (registering APIs, adding proper handling for some servers, etc.). So the updates for some modules might only be detected when the check ran in the context of that particular site. And then what about when there were several such modules active on different sites? This would essentially lead to conflicts where the list of updates would be getting overwritten every day by each of the sites on the network.

The only way to solve this would be to perform the update check in a special context where all of the modules are loaded.

[JDGrimes]

The only way to solve this would be to perform the update check in a special context where all of the modules are loaded.

While looking for more information about the original code on trac, I stumbled across WP#22129. That is when I realized that doing something like this would allow completely inactive modules to check for updates as well. Possibly a useful feature even not on multisite, though it is something that WordPress decided not to support for plugins.

The main issue with that kind of special context is that modules might end up accidentally behaving as if they are active even when they aren't. I don't think that is a particularly large concern for us at this point, because there is not so much legacy code out there, like there would have been thousands of plugins.

But really, we could make this explicit by loading only a particular file from the module, like update.php (similar to how uninstall.php works), only if the file was present.

[JDGrimes]

Turns out that the update checks are only hooked up on the main site on multisite, see WP@15591. So we will do the same with modules. It makes sense that only network-active plugins or those active on the main site should affect the update routine, or at least for other modules. We can revisit the possibility of update checks for inactive modules later.

[JDGrimes]

We should probably add a note to the readme about privacy. Something like:

Privacy Policy

WordPoints does not communicate with any remote services by default. However, when you install modules from WordPoints.org or other servers, WordPoints may communicate with those services in order to provide you updates for the module. Check the privacy policy of the module server in question to learn more about what information is shared with it, though generally only the ID and version of the module will be sent to the update service.

[JDGrimes]

As far as cron goes, we still have an issue though, in that WordPoints is not always network active on multisite. When it is, we can just schedule the updates once in the context of the main site/network admin. But when WordPoints is only active per-site, we have a problem, because the plugin may not be active on the main site. (WordPress core doesn't have this problem, of course, because it is always "network active" on multisite.)

Maybe we could bypass the whole issue by just hooking into plugins cron schedule instead? But that is still only executed in context of the main site, so it doesn't help us.

One way that we could actually run the update check in the context of the main site even when WordPoints wasn't active on it (and even when it wasn't active at all, really), would be to create a mu-plugin for that purpose when WordPoints is installed. But that seems like overkill for this.

I guess maybe the moral of the story is that modules are network-wide things, and thus WordPoints really always needs to be network-active on multisite for them to be handled sanely. Perhaps we should work toward that instead of trying to accommodate this here?

[JDGrimes]

I've created https://github.com/WordPoints/wordpoints/issues/656, but it is a ways off from being possible to implement, or even a final decision on whether that is the direction to go.

In the mean time, I think it is still important to provide module updates to any installs that have WordPoints enabled per-site on multisite instead of network active. Possibly the best approach for that is just to go ahead and let the cron hook be registered on every site in that case.

[JDGrimes]

On a different note, while I was working on the expiration code for the EDDSL API I realized that the expiration date is returned in the timezone of the remote site, although we have no way of knowing what that is. I brought this up with the folks at EDD, suggesting that maybe the time returned should be UTC, or an expiration_gmt field should be added, and Pippin said that they'd add something like that in a future version. Until then, we're just treating it as if it is UTC even though it isn't.

[JDGrimes]

One thing that I realized about the cron checks was that the checks run twice a day, and the cache validity is 12 hours. This means that when cron updates the cache, it will be valid for another twelve hours, probably expiring just after the check would have run the second time. Cron schedules don't always run on time, of course, and the checks can take a while, so it is likely that in many cases the check won't run.

In WordPress core's wp_update_plugins() function, the cache expiry is automatically set to 0 seconds when cron is being run, but that logic was removed from the function we use in the module. Instead, we introduced a parameter that allowed the acceptable cache age to be indicated from the outside. We can utilize this when scheduling our cron hook, by specifying the timeout as an arg for the hook.

However, I'm not sure that we want to use 0 seconds. Does it really make sense to always run the check on the cron schedule, even when it has just been run a little while before? It isn't as if the schedule runs on some super special timeline that users will expect updates to be retrieved at.

I suppose though, that really what it means is that the cache can never get much older than twelve hours when using the 0 second age requirement. If we make the requirement 6 hours for example, as a compromise, the cache can get up to 18 hours old before the next cron check is run.

But if our concern is just making sure that we check at least every 12 hours, then why use what are essentially hacks, instead of just scheduling one-time cron events? We could schedule the event as a one-time deal, and automatically clear the schedule and schedule a new event at the end of the update check. So we'd know that after the update check, a new check would always get run about 12 hours after. Though slightly more complex, I think that makes a lot more sense.

[JDGrimes]

Installer and Upgrader

While working on copying over the module upgrader, I realized that it also includes the install() method. This is because the Plugin_Upgrader class, which is what it was based on acts as both an upgrader and and installer.

The interesting thing is that we actually already have an installer in WordPoints. We didn't include the upgrade methods as part of it though, because we didn't support upgrading modules in core.

I'm not entirely sure why we added the install methods on the upgrader, especially since we already extend the upgrader from it. But I suppose that it was because there were some small features that the core installer did not support, and we were anticipating installing modules directly from remote servers, although we never actually created that feature. Some related functionality then also had to be copied over due to some things in the installer being private. Other functionality was duplicated more-or-less unchanged, and perhaps unnecessarily.

The point is though, that we have an installer with a few small things missing, and an upgrader that extends it and also duplicates some functionality.

What I'm trying to decide is how to best remove the duplication. Should we just move all of the functionality to the upgrader, as it is with the plugin upgrader? Or, should we pull up the things that we need to the installer?

If we did the former, we could deprecate the installer. But although this is consistent with the way the WordPress core installers work, I think it is kind of confusing. There is an upgrader, but no installer. I think since we already have the installer, it might make the most sense to move the shared functionality up to it, and have the upgrader extend it if needed. I'm honestly not 100% sure that they even have to extend one another, but we'll see.

[JDGrimes]

This also makes it much easier to see which changes have been made the the installer functionality.

The check_package() and module_info() functions are needed by both, so it makes sense for the upgrader to extend the installer.

[JDGrimes]

In the function that displays the update row in the modules list table we check if the module has a server with an API that supports updates before displaying the row. But I'm not sure that there is really much sense in that. That will only be invoked if a module has an update listed, which implies that updates are supported. It would only be an edge-case where for some reason an update was added to the list and then support for updates was removed that this sanity check would kick in. I think though, that since we know at that point that an update may be available, that we should probably do the user the courtesy of telling them. Then again, I suppose that if the server has changed, it could be that the user changed it manually, and installing the update would override that. So perhaps it is best to leave the sanity check in place.

[JDGrimes]

It is true, however, that we don't actually check if the server or API have changed, only if there still is one. But I guess perhaps that is a more distant edge case.

[JDGrimes]

Also, we can't even show the changelog without the API, and we provide a link to that in the update row. So this really makes sense.

[JDGrimes]

In the module there was a function, wordpoints_get_installed_translations(). It would get any module translations installed in the translations directory, and load them. However, it was unused, so wasn't actually doing anything. And as we don't currently install any translations there, it wouldn't be doing anything anyway. However, there is the likelihood that we will do so in the future, and the potential that other module server APIs might want to use it, so perhaps we should copy it over anyway. There is also the possibility that users might want to place their own custom module translations in the directory, rather than into the modules themselves where they would be overwritten on update.

[JDGrimes]

Actually though, this isn't what loads the textdomains. It just gets a list of the files. The function in WordPress that it is based on is only used when deleting plugins/themes to remove their translations with them, and when sending the list of translations to WordPress.org on update check.

We could do the same when we delete modules, of course, but really there is no need to get all of the translation files for all modules at that time, only for the module(s) being deleted. So I'm thinking that maybe we should just hold off doing anything with this until we begin actually providing translations for modules, and storing them there.

Basically, the implementation was incomplete, and it is kind of a separate issue from our main goal here.

For posterity, here is what the function looked like:

/**
 * Get installed translations for WordPoints extensions.
 *
 * Currently the only translations are for WordPoints Modules.
 *
 * @since 2.4.0
 *
 * @param string $type The type of extension to retrieve translations for.
 *
 * @return array An array of language data.
 */
function wordpoints_get_installed_translations( $type ) {

    if ( 'modules' !== $type ) {
        return array();
    }

    $dir = "/wordpoints-{$type}";

    if ( ! is_dir( WP_LANG_DIR ) || ! is_dir( WP_LANG_DIR . $dir ) ) {
        return array();
    }

    $files = scandir( WP_LANG_DIR . $dir );

    if ( empty( $files ) ) {
        return array();
    }

    $language_data = array();

    foreach ( $files as $file ) {

        if ( '.' === $file[0] || is_dir( $file ) || substr( $file, -3 ) !== '.po' ) {
            continue;
        }

        if ( ! preg_match( '/(?:(.+)-)?([A-Za-z_]{2,6}).po/', $file, $match ) ) {
            continue;
        }

        list( , $textdomain, $language ) = $match;

        if ( '' === $textdomain ) {
            $textdomain = 'default';
        }

        $language_data[ $textdomain ][ $language ] = wp_get_pomo_file_data( WP_LANG_DIR . "$dir/$file" );
    }

    return $language_data;
}

[JDGrimes]

For the EDD SL API, we had to decide what to do in the activatable() method of the license object when the activation limit for a license had been reached. We left it so that it didn't take that into account, thinking we'd come back to it later. It is later, and I'm leaning toward leaving it like that. The license is activatable, it just can't be activated on that particular site without first deactivating it on another one. We could add additional logic to take this into account and say that the license key is not activatable when the limit has been reached, but I don't see much value in that at this point.

[JDGrimes]

I've added the code to deactivate the WordPoints.org module when we update. A notice like this will be shown to the user:

[JDGrimes]

Currently we only run the module update checks on the cron schedule. However, WordPress runs the plugin update checks on admin_init (checking the transient and only rerunning the checks every 12 hours), and on the plugins and update screens with an hour timeout, and on the updates screen with a minute timeout, and on upgrader_process_complete with a 0 timeout.

I suppose we really ought to do all of this with our module update checks as well. Though I really don't see the purpose of hooking to admin_init, except with cron is not running properly, perhaps.

[JDGrimes]

We had wondered before whether the updates module server API interface should include the get_latest_version() method. It seemed that perhaps it should be possible to determine the latest version of a module even if updates weren't provided via the API. Upon further reflection, this seems like it probably would be a good idea. It is similar to how we have the basic renewable interface, but a manually renewable interface extends it, and if renewals could be performed via the API somehow, that would be another interface extending it. So basically we should have a basic interface for APIs that provide update information, and then other interfaces extending that for when the updates need to be installed manually by visiting a link and downloading them, or when they can be installed automatically.

What got me thinking more about this was that WordPress actually provides special handling for plugins that require manual updates.

[JDGrimes]

While working on this I wanted to see what the updates row would look like now, since I had to make some changes. So I installed a module that needed an update from WordPoints.org. And nothing happened. So I did some digging and found that the update check was returning a 403 Forbidden response.

Sorry, there was an error. Please be sure JavaScript and Cookies are enabled in your browser and try again.

I thought at first that this was caused by Cloudflare, but further digging led me to realize that it was actually coming from the WP-SpamShield plugin that I've recently installed there. The issue was that that plugin has a setting that filters all form submissions, and since the module version check is a POST request, it was filtering that as well (or maybe it would have even for a GET request). Anyway, I just created a custom plugin to bypass the bot checks for the EDD SL API, and all is working.

Interestingly, I also discovered that SSL was being used, whereas previously we thought that WordPoints.org could not be accessed from some sites over SSL.

Oh, and here is what the update row looked like:

[JDGrimes]

We periodically check for module updates, which causes a refresh of most of the module info, because of how the server API objects are designed internally. However, the module license info is handled via the module license objects, which are separate, and therefore the module license status and expiration date, etc., are not refreshed. The cached versions continue to be used indefinitely.

I had noted down earlier that we probably want to expire the module expiration date cache automatically when the expiration date is reached. That is probably a good idea. However, I think that it makes sense to refresh the license status info periodically, similar to the way that we check for updates. Possibly it should even be done on the same schedule, as otherwise the modules that use licenses won't be able to really run the check, if the license is no longer usable.

Of course, we would just refresh this from within the server API object when we do the version check, but the license information is not returned by the version check request.

[JDGrimes]

Does it actually do any good to check the license statuses before we run the update check though? I mean, if the licenses are bad, there isn't much that we can do about it. We could possibly skip the update check for the modules that don't have good licenses, but other than that there is really no benefit that I can see.

I guess it is kind of like this though. We were checking for updates, and we would have shown them to the user if there were any. But now we don't really know whether there were any updates or not (depending on the API), because the license was invalid. So we ought to tell them that, instead of letting them think that there aren't any updates.

In general though, it seems like almost an edge-case that a license would become invalid. Most of the time licenses are just going to expire eventually. So checking the statuses every 12 hours seems unnecessary.

Instead, let's re-check the license status on-demand when the latest version request fails.