WordPoints / wordpoints

Points plugin for WordPress
GNU General Public License v2.0
20 stars 15 forks source link

Un/Installer: better install bootstrap #307

Closed JDGrimes closed 9 years ago

JDGrimes commented 9 years ago

In #306, I'm working on introducing a better bootstrap for uninstallation. One thing that can be made simpler is un/installing capabilities; see https://github.com/WordPoints/wordpoints/issues/306#issuecomment-84033885. So, in that vein, I think we can improve the installation bootstrap as well.

Things we can do automatically:

I haven't deeply considered number 2 yet, but I think it is something that we could at least work toward.

JDGrimes commented 9 years ago

That takes care of capabilities.


Another thing that we could do is automatically create the tables. Kind of related to #308.

JDGrimes commented 9 years ago

I've got a bootstrap for table creation now. On to setting the version in the database.

Right now, the handling of component and module versions is done almost entirely outside of the un/installer classes. So it isn't going to be as simple as standardizing what we already do and moving it up to the base class. We will actually need to figure out the API, and I'm not at all sure that we are currently handling things correctly.

Core

First, let's look at how WordPoints itself does this. The version is set in the database when the plugin is installed by the un/installer class. This is part of the wordpoints_data option, which is a network option when WordPoints is network-activated, but a per-site option otherwise. This option is deleted when the plugin is uninstalled. It is also updated by wordpoints_update().

Other than the fact that perhaps updates should be more fully handled by the un/installer, it looks like core is doing everything right here.

Components

The base un/installer class provides a set_component_version() method, and this is used by both the ranks and points components to set the component version in install_network() and install_single(). This method will set the version in the wordpoints_data option, which obeys the same rules as above based on how WordPoints is activated. This is fine because components will always be activated with the same network-wide status as WordPoints itself. (But see https://github.com/WordPoints/wordpoints/issues/285#issuecomment-74964104.) The version is updated on update by WordPoints_Components::maybe_do_updates().

So, again this appears to be correct, except that we'd like to maybe bring the version updating code into the un/installer class.

However, I think there are issues when we come to non-core components, because they may be uninstalled separate from WordPoints itself, and when that happens the version will need to be removed from the database. As of now, non-core components would have to handle that manually. I think there is still room for debate over whether non-core components should even be a thing though.

Modules

Currently it is entirely left up to modules to ensure that they properly install and update using the un/installer. That needs to change. We already handle un/install, so it is only natural that we should handle these as well.

Most of the modules I've built don't require an un/installer, but WordPointsOrg does. It adds it's version to the wordpoints_data maybe-network option in install_network() and install_single(). It hasn't had to run any updates yet, so there is no code for that yet. The version is removed when the module is uninstalled.

This implementation is actually wrong. Whether the wordpoints_data option is network-wide is based on WordPoints's activation status. So when the WordPoints is network-active and the module is not, the version will be saved in the network-wide option, which will lead to issues when it comes time to update. When a module isn't network-active, it might have a different DB version on each site. However, with this implementation the version is only stored once, globally. So only one site will end up having it's database updated, because then the database version would get updated and so the rest of the sites would think that they were already up-to-date.

The solution is that modules need to store their versions in a regular option when they aren't network-active on multisite.

Uninstall is also done wrong, because the version could be in either the network-wide or regular option on multisite, and so should be removed from both.

JDGrimes commented 9 years ago

I think there are issues when we come to non-core components, because they may be uninstalled separate from WordPoints itself, and when that happens the version will need to be removed from the database.

Related to this is #264. If we were bundling the uninstall routines together, we could check if WordPoints itself was being uninstalled and act accordingly here. (Actually, we could detect WordPoints being uninstalled without bundling.) This would be useful for modules as well.

JDGrimes commented 9 years ago

To move the version update process into the un/installer API, we need to be able to do several things:

  1. We need to be able to get the database version of all installed and active entities.
  2. We need to be able to get the code version of all installed and active entities.

No. 1 isn't difficult, if they are all stored in wordpoints_data. No. 2 is more difficult. For WordPoints, we have a constant available, and we can get the version for components through the components API. For modules, though, there is no way to do it. We could make a part of the un/installer API, but we shouldn't have to load each un/install class for each module on every page load just to check if we need to update. We'd have to either require all un/installers to be registered (#264) or we'd have to introduce a new modules API (#263).

JDGrimes commented 9 years ago

We need to be able to get the code version of all installed and active entities.

I've opted to do that through a new modules API (#263).

JDGrimes commented 9 years ago

The solution is that modules need to store their versions in a regular option when they aren't network-active on multisite.

I just wanted to note that this will lead to wordpoints_data options on every site even when WordPoints isn't storing it's own version in them. I don't foresee this causing any problems though, and we already delete the wordpoints_data option from all sites on un/install anyway.

However, this does bring up a similar issue. When a module (or WordPoints for that matter) is toggled between network-wide and per-site activation, this option (or any other database features) isn't cleaned up. I'm not sure this actually causes any significant issues, and it is kind of an edge case, but I thought it should be noted.

JDGrimes commented 9 years ago

In the end the version updating code is a part of WordPoints_Installables instead of the un/installer class. I think that for now we'll leave it like this. It really makes the most sense this way, because we can't check whether an update is needed from within the un/installer anyway, without having to load every un/installer on every page.

JDGrimes commented 9 years ago

However, this does bring up a similar issue. When a module (or WordPoints for that matter) is toggled between network-wide and per-site activation, this option (or any other database features) isn't cleaned up. I'm not sure this actually causes any significant issues, and it is kind of an edge case, but I thought it should be noted.

That is something that I've left as an accepted edge-case. We could revisit it in the future if needed.

JDGrimes commented 9 years ago

Uninstall is also done wrong, because the version could be in either the network-wide or regular option on multisite, and so should be removed from both.

This is the last thing that remains to be done: remove the version on uninstall.

JDGrimes commented 9 years ago

Any new enhancements can be new tickets.