Take out the two upgrade paths: a pre-1.0 alpha is reinstalled, not upgraded

Closes [#3624519].

Two hooks said this project has an upgrade path:

  • orchestra_server_api_update_11001(), which installed the orchestra_identity_schemes field storage on the consumer entity;
  • orchestra_client_post_update_remote_entities(), which moved a single remote setting into a remote Orchestra entity and deleted the settings object.

Both are removed. Reinstalling is how a site moves between these releases, and one hook is worse than none: an operator who runs the database updates and sees this project go by has been told it handles upgrades, and what they were handed was one field out of a release's worth of renamed entity types, added base fields and retyped columns. Nothing announced the rest.

update_11001 was also a second home for something hook_install already owns. The install hook installs four consumer field definitions from _orchestra_server_api_consumer_fields(), named once on purpose so installing and uninstalling cannot come to disagree; the update hook installed the fourth again from its own copy of the call. A fresh install is unchanged, which is what the server API kernel tests exercise.

The guard

NoUpgradePathTest, a Unit test over the new getShippedUpgradeFiles() collector, which walks exactly the two names Drupal reads upgrades from: <module>.install and <module>.post_update.php. Three assertions:

  • no shipped file declares a hook_update_N or a hook_post_update_NAME;
  • no module ships a post_update.php at all, said separately because an empty one declares nothing and would pass the rule above while still being the file somebody adds the next one to;
  • the install and uninstall hooks that remain are still there, so the two absences above are not passing over a walk that read nothing.

No linter reports this. A hook_update_N is valid PHP, valid Drupal and good practice nearly everywhere else, so it arrives from habit, one field at a time, and each one is individually reasonable.

Proven both ways

With both hooks restored the guard fails and names them:

modules/orchestra_client/orchestra_client.post_update.php declares a hook_post_update_NAME: function orchestra_client_post_update_remote_entities
modules/orchestra_server_api/orchestra_server_api.install declares a hook_update_N: function orchestra_server_api_update_11001

Removed, it passes. HandshakeTest and InitiatorAcrossTheHopTest stay green (32 tests), so the field the update hook installed is still installed by the install hook on a fresh site.

Drive-by in the same pass: orchestra_interaction.install's @file line named update hooks that file does not have.

What the audit round on this MR turned up

The deleted post_update does more than nothing. It ends with $settings->delete() on orchestra_client.settings, which it emptied of the three keys it was moving. That config object has since acquired the notice request limits, and it ships in config/install as FullyValidatable. So a site running drush updb today would have its notice limits silently deleted: flood.per_remote and flood.across_endpoint come back NULL, cast to 0, and the endpoint stops counting while the status report reports "On, and no limit set". Deleting the hook removes that, and is the stronger half of the argument for doing it.

The guard was blind where the rule is most likely to be broken. getShippedUpgradeFiles() first walked modules alone, and collect() never walks the project root as a directory - it takes a separate keeper for the root's own files, which every sibling collection that can have one passes. The engine is where the entity types live, so orchestra.install at the root is the single most likely place for a hook_update_N in this project, and it was the one place the guard could not see. Proven: with a fixture orchestra.install declaring orchestra_update_11001(), the first version of the guard reported OK (3 tests, 16 assertions); with the root keeper it fails and names it.

On the rebase. Rebased onto 1.x after [#3624518] merged; the two branches both added a collector to ShippedFilesTrait at the same anchor, resolved by keeping both. The follow-up commit that had moved this one down the file to dodge that collision was dropped, since the collision it worked around is resolved here directly.

Edited by Frank Mably

Merge request reports

Loading
Loading