WordPoints / wordpoints

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

Installables app: Don't run install instead of update if no DB version set #716

Closed JDGrimes closed 7 years ago

JDGrimes commented 7 years ago

Via https://github.com/WordPoints/wordpoints/issues/404#issuecomment-320335283:

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.

However, in https://github.com/WordPoints/wordpoints/issues/706#issuecomment-323449416, we did discover an issue it would cause:

This means that when an installable first adds an installable object, it doesn't need to perform an update, it can just add an install routine, and that will get run by the installables app. This design may make things difficult, however, if the update was doing something that was only needed as an update (to correct a bug in the db, for example), and not during install.

We also observed that this design means that just loading an installable once will accidentally install it.

So we thought that we should revisit this.

JDGrimes commented 7 years ago

I think we have to consider which situation we are more likely to be dealing with here. On the one hand, we know that this was needed because of a bug in core, but it is something that probably isn't likely to surface again, and might not really be an issue for extensions or components anyway (it specifically related to registering the plugin activation hook with the wrong file name). On the other hand, adding an installable to an existing extension is totally expected, so it is important that it work well. It should probably take higher priority then trying to avoid hypothetical bugs.

Of course, that is assuming that there is another way to correct the buggy behavior. I actually was wondering if we have to correct it at all, but was a bug in 2.0.0 that was fixed in 2.0.2, and 2.0 is still 8.4% of our current install base. So there probably are some installs out there where WordPoints wasn't installed properly. And thus we still need to address that in the code. I wonder if we could do it as part of an update though, instead of baking it into the installables API. All we would have to do is detect that the DB version wasn't set in our core installable, and if so, run install. Or, we could just limit this check to core again. Or, we could just call core's activation function in some other hook as needed. So we do have some other options at our disposal here.

When we add an installable class to an extension, we probably wouldn't expect the install routine to be run; after all, install takes place on activation. So that behavior would be unexpected. Also, when we were adding it for an update, and not install, we would expect that update to be run. Thus it not running would also be unexpected behavior. (And note that this would occur even if we added several updates, like if we were migrating old update code to the installable.)

So it seems that we really ought to change this behavior.

JDGrimes commented 7 years ago

And it wouldn't just be the first update that was skipped in this way, because the code might receive several updates while inactive. In that case later updates would also be skipped, which would be totally unexpected.