Refactor un/installer class #404
Comments
|
Related: #264 |
|
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. |
|
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. |
|
One thing I noticed while refactoring is that currently in |
|
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:
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. |
|
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. |
|
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 55267ab. 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. |
|
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. :-( |
|
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). |
|
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 I don't really want to |
|
On the other hand, I was wondering if we really need to use |
|
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. |
|
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 A little more investigation revealed, however, that we actually never pass a value in for the 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 |
|
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. |
|
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. |
|
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. |
|
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. |
|
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. |
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. |
|
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. |
|
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. |
When an extension was not network active, the old installables class would still run the updates for all sites at once. However, in the new installables app this was fixed for the new installable objects. We still weren't handling it properly in our shim for legacy entities still using the un/installer API though, so we've updated it to pass in the `$network` parameter correctly for extensions. See #706, #404
Introduces a factory class for update routines, to enable most logic to be removed from the installables an into the updater itself. The installable must now return an array of update routine factories to the updater, rather than the update routines themselves. The updater then uses the factories to retrieve the routines that it determines need to run. The logic in the updater is also updated to determine the network version for per-site installed entities separately from per-site versions. The installer is updated to save the per-site version in that case as well, and not just when network-active. The core installables have been updated to use the new factory design. Because one of the updater objects needs to be constructed with 5 args, the utility function for that purpose had to be updated to support 5 args instead of just 4. Fixes #706 See #404
Fixes issues resulting from the installables app not being mocked during some tests, leading to the installable remaining registered with the app, and perpetuating its version in the database. In order to fully fix this, we needed to also allow the installable to be registered more than once, so that it would have its app registered again. Some issues with the tests were also revealed by this, and fixed. We also realized that we use 'extension' as the type for the installables registered in the new code, but the legacy code was still using 'module', so we've standardized on 'extension'. See #404
The class is now deprecated and no longer contains any functionality, so the tests would be useless. See #404
When we decided to make this class abstract we missed updating these usages. See #404
Calling a static method by name from a variable is not supported prior to PHP 7. See #404
Otherwise the installables app will attempt to use the registered class, which won't be loaded because the component isn't active. See #404
This isn't a good pattern to use in general anyway, and it didn't work on PHP 5.2, so we've removed these methods. This updates the tests to avoid using the methods. - First, we can avoid using them in the component and extension installable tests by updating the code to use a method we already provide instead of accessing the property directly. - We change how the installables class handles custom caps, so that it now uses a method instead of a property. This was a carry over from the old un/installer, but it was a poor design. We've already made a similar change for the database tables, this makes it so that the code for the custom caps is now consistent with that. - In the installable tests where these methods were used to access the properties of install or uninstall routines, we've simply omitted this from the code completely. We test that the routines returned exhibit the correct behavior in our uninstall tests. - We utilize the new basic installable class in the installable tests rather than a mock, which avoids having to set the properties this way since we can just pass them to the constructor. - For the order test for the routine class, I've moved the test of this behavior to the tests for the uninstall routine class, which actually sets the property internally to utilize this behavior. See #404
We would get failures like this: ``` 1) WordPoints_Installer_DB_Tables_Test::test_creates_db_tables Failed asserting that format description matches text. --- Expected +++ Actual @@ @@ CREATE TABLE `wptests_test` ( `id` bigint(20) NOT NULL -) %autf8 %autf8_general_ci +) ENGINE=InnoDB DEFAULT CHARSET=utf8 /home/travis/build/WordPoints/wordpoints/tests/phpunit/tests/classes/installer/db/tables.php:67 2) WordPoints_Installer_DB_Tables_Test::test_creates_db_tables_with_prefix Failed asserting that format description matches text. --- Expected +++ Actual @@ @@ CREATE TABLE `wptests_test` ( `id` bigint(20) NOT NULL -) %autf8 %autf8_general_ci +) ENGINE=InnoDB DEFAULT CHARSET=utf8 /home/travis/build/WordPoints/wordpoints/tests/phpunit/tests/classes/installer/db/tables.php:95 ``` The problem is that MySQL does not always include the collation in `SHOW CREATE TABLES`. So we've adjusted the tests to not include the collation in the pattern we check for. See #404
Supports more mock object methods on PHP 5.2. See #404
So that we don't have to try to set the protected property using reflection, which doesn't work on PHP 5.2. See WordPoints/wordpoints#404
So that we don't have to try to set the protected property using reflection, which doesn't work on PHP 5.2. See #404
|
OK, I guess if anything else needs to be done here new issues should be opened. |
The "r" key on my keyboard has been acting up... See #404
The legacy installables app needs to use `module` internally, but the new installables app uses `extension`. We attempted to handle this in 2e516a7, but accidentally were modifying the `$slug` of the installable instead. We also didn't correctly ensure that the installable objects were still constructed with `module`, because that is what they expect. See #404, 2e516a7
It is no longer used in core, and has been replaced by the new installables API. Only the breaking updater still uses it, and that will be fixed in a subsequent commit. See #404
The code for uninstalling an extension with an installable class was broken because we were passing the basename of the extension to `wordpoints_get_module_data()`, when it actually expects the full path. Our tests were updated in the last commit so that this is now tested, and of course that exposed that it wasn't working. We thought it was worth committing the fix separately, since it wasn't directly related to the main brunt of that commit. See #404
And deprecate the old version, which extended the now-deprecated base un/installer. In the process we found a bug in the filesystem mock library, which we fixed, requiring us to update to the newer version. See #404
Deprecates the old un/installer. See WordPoints/wordpoints#404
Extensions that use the new installables API were not being updated correctly, because it was always assumed that they were network active when WordPoints was network active. This is not the case. In fact, the extensions are not network active by default, and so update tests for them would fail. See WordPoints/wordpoints#404
And deprecate the old un/installer class. See WordPoints/wordpoints#404
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.
The text was updated successfully, but these errors were encountered: