Closed JDGrimes closed 7 years ago
I'm not sure that we're gaining much by this though. The main factor seems to be trying to prevent from uninstalling on some sites unnecessarily. However, this is more of an edge-case, and a decision that we made to change the logic because at the time we didn't realize that we didn't track the installed sites in this case. Uninstall routines need to be idempotent anyway, so other than optimization reasons this shouldn't actually be an issue.
The only other issue that I can think of is when an entity has been network activated, then deactivated and only activated per site. Again though, that's rather an edge-case. And once again, all that it would result in is the uninstall routines running on all site even though they might not need to, because we'd return the list of all sites as the installed sites (in the current logic), because we were network installed.
And as far as optimization goes, we might also consider that we're saving more data in the database this way, FWIW.
So I'm thinking that maybe it is best not to pursue this. Note that our logic that checked the count of installed sites would still work, because we'd just return the list of all sites. Although it would be sub-optimal, because there's really no need to pull up the list of all site IDs in that case; it would be better to only check that if we're not network installed.
So I'm closing as wontfix
. If we find a more compelling reason to do this, we can reopen.
One other thing that this potentially relates to that was mentioned in https://github.com/WordPoints/wordpoints/issues/704#issuecomment-318859561 is when we are de-network-activated and then re-network-activated. If any new sites were added, we wouldn't have been automatically installed on them. However, we realized that the install routines are automatically run again on activation anyway, so we'd get installed on any new sites if needed. We'd only need a list of the sites to install on if we wanted to optimize this, but that would be a larger shift that is completely separate from what we're dealing with right now.
I think https://github.com/WordPoints/wordpoints/issues/703#issuecomment-321902407 summarizes fairly well:
In https://github.com/WordPoints/wordpoints/issues/404#issuecomment-321925437 we decided to branch out into a dedicated ticket, since this is really independent of the un/installer rewrite itself.