Refactor un/installer class

[JDGrimes]

The un/installer class bundles all of the bootstrap for installation, uninstallation, and update, into a single class. It is growing to thousands of lines long, with hundreds of methods. We need to refactor this. Ultimately, I think the uninstall, install, and update portions should each be split into a separate helper class. Then we'll probably want to make each uninstall feature a separate class, which will allow for greater extensibility. We may also want to make it so that each update is a separate class as well.

During all of this, we will want to maintain back-compat of course, which probably won't be too tricky.

[JDGrimes]

Related: #264

[JDGrimes]

Let's see if we can take a whack at this in 2.3. Possibly without doing #264 yet, but preparing the way for it.

[JDGrimes]

I'm thinking that we'll probably want to introduce an installable class, that will keep track of the version info, network install status, etc. This will sort of be the core of the current un/installer class, which will likely be able to extend it. But the install, update, and uninstall features will be split out into separate helper classes that are utilized by or operate on the installable class.

[JDGrimes]

One thing I noticed while refactoring is that currently in unset_db_version() we actually unset all of the data for the entity, not just the version, except for when dealing with WordPoints itself. This has not caused us problems only because we only call this during uninstall. The behavior was actually what we wanted to accomplish in that case, but I'm going to change to avoid bugs in the future.

[JDGrimes]

I was going to have the base un/installer class extend the new installable class. However, many of the methods that we are now public were previously protected. As a result, any child classes that are overriding these methods will get a fatal error:

PHP Fatal error: Access level to WordPoints_Breaking_Updater::is_network_installed() must be public (as in class WordPoints_Un_Installer_Base) in /Users/johngrimes/git/wordpoints/src/classes/breakin...

Unfortunately, I don't think that there is any way to work around this. So instead of extending the new installable class, we'd have to either just utilize it internally, or leave the methods that were copied to it intact on the un/installer, and suffer the duplication. I'll try seeing if we can utilize the new installable class internally first though.

[JDGrimes]

I'm now working on splitting out an installer, and I was thinking that each part of the install routine would be split out into a class representing the subroutine. Currently installing database tables and custom caps is automatically handled, for example. So we'd have a subroutine that would handle each of those.

I was thinking that we'd grab the install info from the installable object, and then run the necessary subroutines, passing the needed data to each one. However, I was getting snagged on the question of what to do when we were given a subroutine slug by the installable that we didn't recognize. So instead, I'm now thinking that the installable object should return the already-constructed subroutine objects, and then we just have to loop over them as needed.

[JDGrimes]

I think that in the new installer there is no need for the bitmask return from the skip install check function as was introduced in https://github.com/WordPoints/wordpoints/commit/55267ab414e358c7941a705cf226ed3e965187bc. We don't need this because we can now check if an installable's routines need to run on each site or not, since we have the list of routines that need to run. Also, we're not expecting it to be overridden in sub-classes in that way anymore. If we do need to do that in the future, we can just introduce a second method to check if there is any install to run, while this one will still just check if the install should be skipped.

[JDGrimes]

I've just realized that when there is no per-site install routine to run, the IDs of the sites aren't added to the list of sites on which the entity is installed. This is true both in the new and old code.

Fortunately, this only affects the ranks component in core, since that is the only thing that doesn't have a per-site install routine. But we ought to fill in the site IDs on update.

We'll also need to modify the code in both the old and new implementations, because I don't think that we'll be able to have the old un/installer class extend the new installer. :-(

[JDGrimes]

This also made me realize that we realize that we ought to optimize the adding of these site IDs anyway, so that we don't have to switch to each site (which in this case we don't need to do otherwise). We should just add all of these site IDs at once, saving ourselves a bunch of option updates (and retrievals as well, I guess, for that matter).

[JDGrimes]

I'd already split out the database table creation code into its own class as part of the new installer design. But as I am writing the tests for it, I realized that it just assumed that the dbDelta() function will be loaded (that function is part of WordPress's update functions, and isn't normally loaded).

I don't really want to require_once the file within the class, for the sake of performance. It would be silly for that to run every single time on multisite (maybe negligible in comparison to database creation, but still). I guess though, that we could put it in the constructor, and since the class would only be constructed once in the new design, that wouldn't be a problem. So let's do that, even if it feels a little strange.

[JDGrimes]

On the other hand, I was wondering if we really need to use dbDelta() here at all. That function includes a lot of extra handling for updating a database table based on the supplied schema, if it already exists, and that isn't necessary if we are just creating the tables. So the only reason to use dbDelta() is if we also want to be able to use the same class for updates as well as install. And I guess that may make sense, so let's give ourselves that option.

[JDGrimes]

I'm thinking that we'll circle back and fix the site IDs issue after we get the updater split out. Then we can write the update code using the new API.

[JDGrimes]

While extracting the updater, I noticed that we required the network-active status of the entity to be passed in, just as we do for install. However, this seems unnecessary for updates, since at that point the entity has already been activated and so we can determine whether it is network active or not independently.

I was going to use the is_network_installed() method to check this, but of course that only means that the entity has been network active at some point, not that it is currently network active.

A little more investigation revealed, however, that we actually never pass a value in for the $network parameter of the update() method, and so it has always been internally defaulting to the value of is_wordpoints_network_active().

Now that I think about it, I recall that the logic behind this was that even when an extension isn't network active itself, it should still update all sites on the network where it is active at once, rather than one by one as the update routine is triggered on each. The reason is that we also have to think about site switching, which could cause data to be attempted to be pulled from another site on the network before the update routine has been triggered on it. If we still think that logic is valid, then perhaps we don't need to offer a $network parameter for the updater at all.

[JDGrimes]

A problem here though, is that we loop through all sites where the entity is installed, not just the sites where it is currently active. When it is network-activated, these two would be the same; there'd be no sites where the entity was installed but not active (nor should there be any sites where it is not installed, but active, either). But when per-site installed on multisite, the entity would end up having the update routine run for all of the sites where it was installed, even though it wouldn't necessarily still be active on all of them.

This wouldn't be a problem if we were running the update for each site separately, when the site was visited. We'd only run where we were active, because we'd only run for the current site; we wouldn't use the installed site IDs list at all.

As it stands, we don't actually have an easy way to get a list of all sites where the entity is active, so other than adding such a feature or changing this behavior, I see no other means to remedy this.

On the other hand, perhaps this behavior should really be considered correct. After all, if we don't run the updates per-site, then the update won't end up getting run on any site's that the entity is deactivated on, even if/when it is reactivated on that site. So if we don't run the update along with all of the others, it will never occur.

As such, I'm going to call this a "feature", and leave it as it is for now. If we find a compelling reason to change it in the future, we can do so.

Which also means that there's no reason to have a network argument for the updater; for now we will just have it match WordPoints's network activation status internally, as is the current behavior of the un/installer.

[JDGrimes]

For the updates, I am trying to decide whether to return all update routines and then decide which one to run in the updater, or how to design it. I think that it doesn't make much sense to load a bunch of update routines that don't even need to be run. So we should only construct the update routines that actually need to run.

I suppose that we could return the class names for the routines from the installable object, and not the constructed routines themselves, and then let the updater decide which ones to construct. But that doesn't allow us to pass any information to the objects when we construct them, which sound limiting. So I don't think that is really an option.

Which means that we need to return the update routines already constructed, and only those that actually need to run, for optimization. So we'll have had to already do the version checks.

[JDGrimes]

We currently do the version checks all in one place, and this avoidance in duplication is especially nice if we allow the to and from versions to be specified arbitrarily, since that contributes additional logic.

However, I'm now wondering whether the to and from checks are necessary at all. Since the installable knows its current code version and database version internally, there is no need for that information to be passed in. The fact that the un/installer didn't know that internally is why it was designed that way to start with—not because we wanted to be able to update from and to arbitrary versions. And really, I can't conceive of a situation where we would need to update from or to an arbitrary version. All of the updates needed really do need to be run, or else the database, etc., won't match the expectations of the code.

So passing in arbitrary to and from versions is now a useless feature that we don't need to carry over.

[JDGrimes]

In the installer, we automatically set the DB version, and we automatically remove it upon uninstallation. But we don't automatically update the version on update, that has to be done externally. This doesn't really make sense though, so I'm going to have the new updater update the installable's DB version automatically.

[JDGrimes]

On a similar note, when we detect that there are no per-site updates for a particular entity, we skip that part. But this means that the database version on that site is not updated. This is true even in the old code, I think. If the entity is not network wide, we use the version from the current site, and only update it for the current site. But as pointed out above, we still run the update for all sites where the entity is installed. Except that of course the version isn't updated for those sites.

I guess we need to either stop doing that, update the version for every site, or store the version network wide even when only per-site active.

[JDGrimes]

But as pointed out above, we still run the update for all sites where the entity is installed.

When WordPoints is network active. Otherwise we don't.

Also, I ended up mixing two separate issues above. The one is that in the old code when the entity is per-site installed and WordPoints is network active only the version for the current site is updated. (Though it should be noted that the update will be triggered to run on the other sites as well, and then the version will be updated there. But this in and of itself represents another issue, because the updates may not be designed to run more than once.) The other issue is that in the new design, when we take care of this internally, and so it is fixed in the most general case, we still have the same problem when we skip per-site updates altogether, because then we don't end up calling the per-site update method at all, which is where we update the version. To fix that we'd have to switch to each site just to update the version number, which seems silly.

[JDGrimes]

Split into a separate issue, let's carry on. We can't really address the issue in the new design until we make some decisions from that ticket, so we'll just leave it with that bug in it for now.

[JDGrimes]

Our logic that skips the per-site update on large networks doesn't make much sense either, because it only does that when the entity is network installed (regardless of whether it is network active), but the updates will be run for all sites that it is installed on even when it is not network installed, no matter how many sites it is installed on. So the number of sites that it is installed on should probably be checked then too, and the process skipped if the number is too many.

[JDGrimes]

What we need to do there though may depend upon what we do in #706. So let's split that out as well.

[JDGrimes]

After digging a little deeper into https://github.com/WordPoints/wordpoints/issues/706#issuecomment-315077251, we've basically concluded that the fact that we run the update on all sites on the network is a bug that we accidentally introduced, that it is incorrect behavior, and should be fixed. So the behavior of our new updater can probably go ahead and reflect that decision.

[JDGrimes]

After all, if we don't run the updates per-site, then the update won't end up getting run on any site's that the entity is deactivated on, even if/when it is reactivated on that site. So if we don't run the update along with all of the others, it will never occur.

And as far as this goes, it is only true if we update the version number in the database on the site (which we haven't been doing anyway). Otherwise, once the entity is active the update check will automatically detect that the DB version is out of date and trigger the update.

[JDGrimes]

What about when network re-activating though, after having been per-site active? I guess that is an edge-case and likely wouldn't work properly either way, because even how the network portion of the update runs can be affected based on network activation status. Switching between one and the other will almost certainly cause breakage.

Related: #656.

[JDGrimes]

During update we should not be basing what we do on whether the entity is network installed, but on whether it is network active. The only time we have to worry about install status instead of activation status is during uninstall when we aren't active at all.

I just realized that from the start it was unnecessary to check the install status in do_per_site_update(), because the method is only called if the routine is being run network wide anyway. (And that would only ever happen when the entity wasn't network active due to the bug described above.) So we can actually remove that logic altogether, and just allow the network activation status to be passed in.

We could add an is_network_active() method to the installable interface instead of requiring that info to be passed into the updater, but due to the design of our other code I think having it passed in is just easier. We know that we already have that info at our disposal within the installables class, we just need to use it. But getting it from the installable object is not easy, because the slug of the installable may not be the same thing expected to be passed to the network activation check function (for extensions the basename path is expected, for example).

[JDGrimes]

When it comes to the uninstaller, I was going to put all of the bootstrap for constructing the uninstall routines in the installable bootstrap (this is similar to how it was all in the un/installer class before). But after I'd written it that way, I wasn't really satisfied. It was almost 1000 lines long, and it will grow in the future. It also seems like there is a fair amount of duplication and repetition that isn't easily reduced with that design. So I am splitting the logic out into uninstall routine factories for each different type of routine. This will also allow the factories to implement separate interfaces for each context (site, network, single), and that way we only have to construct the routines that we actually need, that is, for the contexts that actually need to run.

[JDGrimes]

I've realized though, that we don't make any difference between network-level things that should run only when the entity is network installed, and ones that only need to run on multisite. In the base un/installer, we never check the network install status in any of the uninstall bootstrap (except when checking if we should do the per-site install on large networks).

And I guess that means that in the same way, we also don't check if things that need to be uninstalled in site context should be uninstalled only when non-network installed or not. And really, I think that is the more likely to be problematic, since it means we may be running the routine on all sites unnecessarily.

For the most part though, I think that we've left it this way just because it isn't expensive enough to run the extra parts of uninstall (and is something that doesn't affect normal operation anyway). So we could just ignore this, at least for now. If we do see a need to fix it in the future, we can always implement detection then.

[JDGrimes]

The un/installer had similar bootstrap for running the install, uninstall, and update routines. There is one difference for uninstall, however: the install and update routines run the network portion first, then the per-site portion, on multisite, while uninstall does the per-site part before the network part. The base routine bootstrap that we extracted was based mainly on install, and so followed the pattern of doing the network part first. But I'm wondering if it will cause problems for the network part of uninstallation to run first when we extend that class.

Logically, the network uninstall won't depend on data from the individual sites, but the per-site install could rely on data from the network level, even just as part of the normal operation of the functions employed. So I'm thinking we'll likely run into issues if we change this order.

[JDGrimes]

Like with update and install, we skip uninstall if on a large network and network installed. The only difference is that if we aren't network installed, we check to see how many sites we are installed on, and skip if we are on too many.

I'm wondering though, why we should check if we are network installed at all. Shouldn't we just do the check on the number of sites we are installed on? Because unlike with install/update, just because we are network installed it doesn't mean that we were active and thus installed on every site on the network; many sites may have been added since we were last active, that we aren't actually installed on.

So I'm going to change this to go only by installed site count, at least in the new code.

[JDGrimes]

As noted in https://github.com/WordPoints/wordpoints/issues/704#issuecomment-318859561 though, it looks like we don't actually save the site IDs when network active at all. But that will likely change.

[JDGrimes]

In finalizing the design of how the installables API as a whole should work now, the primary factor is in regard to updates. Specifically, the check for whether updates need to be run occurs every single time the plugin code runs. So we want to make that check as unintensive as possible. In particular, I'd like to avoid loading all of the installable classes for all of the entities each time we just check if they need to run their update routine.

The minimal information needed to determine whether any updates should run is just the DB and code versions of each entity. Currently, these are both able to be retrieved via the installable objects, but that would mean we'd have to load the objects for the check. Instead, I suppose we need to supply this information along with the installable class name, so that it can be loaded.

However, even just getting the class name is problematic, because even when we know the extension namespace, we don't know for sure whether it has an installable class without checking if the class exists—which would of course cause the class to be autoloaded. The alternative is to check if the extension includes an /classes/installable.php file, which would indicate that the installable was indeed present. Although that still requires a filesystem check.

Possibly we'd want to pursue a design that could somehow avoid getting the installable class altogether until we knew that we needed to load it.

As far as getting the DB and code versions of each entity, I'm hesitant to manipulate the DB version outside of the installable object. Being supplied the code version from outside of the installable is fine though. So we could have the code version be passed into the update checker, as it is now in the installables class.

We can even use the code versions alone to check if any updates are needed, if we save the previous versions checked in the database for reference. Assuming an autoloaded option, this wouldn't even require an extra DB call, only a negligible amount of memory space. I guess that's essentially what the DB versions are, but they aren't stored in a manner conducive to this kind of check.

[JDGrimes]

It has just occurred to me that the install and uninstall methods aren't really needed as part of a central installables object, since we could probably just as easily run the install/uninstall routines outside of it. All that having them centralized does is force us to register the installable with the registry before calling the install() method. It may reduce duplication, but it could be designed better.

[JDGrimes]

I have been thinking about the fact that the installables app doesn't really need to be persistent, while the apps are stored in a persistent class registry. I was thinking that we could work around this by just deregistering and reregistering the app after using it. (We'd need to reregister it because in addition to the updates it also handles install on new site creation, and it is theoretically possible that both could occur in a single request.) But after further thought, I've decided that this probably isn't worth the minor memory gains. We'd have to trigger an action to have the installables registered JIT, but that would be adding additional memory consumption and overhead of its own. Since the option value is going to be cached regardless, I think the kind of memory gains we're talking about are negligible no matter which way we go here. So I'm just going to go with the basic app design, without any fancy non-persistence hacks. If there is a very compelling reason to change the design in the future, we'll do that.

[JDGrimes]

The maybe update method of the installables class used to check if WordPoints was installed yet, and if it wasn't, it would install it instead of running the update. For all other installables, however, it would just skip updating them if the DB version wasn't set.

This was done in part to address #349, which specifically related to WordPoints core. But as we port things over, I'm wondering if it would make sense to go ahead and just do this for every installable. It seems like a reasonable sanity check, and I can't really think of any issues it would cause. So I think I'll run with it.

[JDGrimes]

Our code for displaying admin notices for entities whose network install was skipped was also bundled in with the installables code, so I've decided to split it out. The only part of those methods that referenced the registered installables was a check to see if the installable in question was (still) network wide. Factoring this code out, I think that its OK to remove this check. This is such an edge-case anyway, and even if a user has network deactivated the installable, the error still holds, because the routine still won't run if they re network activate it in the future, and yet the notice won't be shown again at that time (because it will already be "installed"). So I'm just going to not worry about showing the notice then. It is dismissible anyway.

[JDGrimes]

While working on this, I started thinking about the fact that we only keep track of the installed site IDs for entities that currently have installable classes. I was wondering what would happen when an entity that doesn't have an installable class adds one later. This would mainly be extensions, since the plugin and core components all implement installable classes already.

Well, after further thought, I've concluded that everything would work correctly. Note first that the installed site IDs are currently only used during uninstall, to know where to run the uninstall routine. Which of course only needs to happen on the sites where the extension has actually been installed. Thus, there is no need to save the installed site IDs for where only the code was installed, it is about where the database features were installed.

Of course, when such an extension did add an installable class, it should also set the installed site ID as part of that initial update, using the updater that core offers for that purpose. This will add the IDs of the sites to the list, where this update that installed the extension actually ran, and thus where there was actually something to uninstall.

[JDGrimes]

The one final thing to complete here, is to use the list of installed site IDs even for network installed entities. We saved that for last to address it along with #704 and #703. As it is separate from the basic rewrite itself though, I guess we should open a dedicated issue for it.

[JDGrimes]

OK, I guess if anything else needs to be done here new issues should be opened.