WordPoints / wordpoints

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

Points component fails to remove points user meta on uninstall if network activated #395

Closed JDGrimes closed 8 years ago

JDGrimes commented 8 years ago

While working on https://github.com/JDGrimes/wp-plugin-uninstall-tester/issues/2 I discovered that the points component doesn't uninstall itself properly on multisite. When running the uninstall tests with WordPoints network-activated, this manifests itself with the following failure:

1) WordPoints_Uninstall_Test::test_uninstall
Failed asserting that 'wordpoints' prefix does not exist in `wptests_usermeta`.`meta_key`.
The following rows were found:
        wordpoints_points-points
.

/Users/johngrimes/git/wp-plugin-uninstall-tester/includes/wp-plugin-uninstall-unittestcase.php:343
/Users/johngrimes/git/wordpoints/tests/phpunit/tests/uninstall.php:169

The problem is that we add these options to the list dynamically, based on the list of existing points types. This usually works. However, when WordPoints was network-activated, the list of points types is stored in a network option. Thus wordpoints_get_points_types() retrieves the option through wordpoints_get_network_option(), which uses is_wordpoints_network_active() to check whether to get a site option or a regular option. But when the uninstall is run, WordPoints isn't active at all, so is_wordpoints_network_active() will return false, resulting in wordpoints_get_points_types() ultimately looking for the list of points types in a regular option instead of in the network option that it is actually in. No regular option will exist, and thus our list of points types will be empty, with the result that no meta keys for the points types will be added to the list to be uninstalled.

One way that we could address this would be to just always remove all wordpoints_points-% meta keys. Although of course that won't address the underlying issue of is_wordpoints_network_active() not working during uninstall. Perhaps that is really for a separate ticket though.

JDGrimes commented 8 years ago

Our current logic also doesn't take into account the possibility that WordPoints may have been activated both ways, with points types created at the site level as well. Though that is more of an edge-case.

Note also that this could cause some points types to be stored in the regular option and then to be uninstalled even though any points types which were only created when the plugin was network activated wouldn't have their user meta removed.

I suppose that all of this is an argument in favor of our proposed solution above, of just using a wildcard to delete these.

JDGrimes commented 8 years ago

I'm not entirely sure why we didn't just use a wildcard in the first place. I suppose that we just thought that the meta key prefix was too generic. Although of course it is prefixed with wordpoints_, so what is the worst that can happen if we delete it when WordPoints is being uninstalled? I suppose that we just figured that it was pretty easy to get an exact list, so we decided to just go ahead and do that. But now that we see it isn't so easy, we might as well just go with the wildcard.

JDGrimes commented 8 years ago

Well, actually, the reason that we didn't use a wildcard is because we only support wildcards for options, not for metadata. We use delete_metadata() to delete the metadata, so we can't use wildcards, we have to specify the exact meta key. The main reason that we do this is for the sake of the caches, which delete_metadata() will clear. Though of course we don't do this with options, and so I see no reason to do it with metadata either. Maybe, though, we should consider clearing the entire user meta/options cache after that? Anyway, that would really be for another ticket.

JDGrimes commented 8 years ago

Actually, what we do for the options is actually perform a search based on the wildcard, and then use delete_option() to delete the options. We can do a similar thing with our delete meta functions.