Improve obsolete extension management

[colemanw]

Overview

• Obsolete extensions are labeled as such on the extension page
• Obsolete extensions are not allowed to be installed or enabled
• Obsolete extensions are not just disabled but completely uninstalled during core upgrades

Before

Less thorough / verbose

After

Note that obsolete extensions are labeled on extension page and there is no button to install them. If you were to try installing it via cv you'd get an error.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[mattwire]

@colemanw Just a small concern - uninstalling an extension is supposed to remove all data associated/created by that extension. Clearly in the case of api4 that won't be a problem and we are probably "vetting" each extension that is marked obsolete. But we should be aware of this risk

[MegaphoneJon]

How would this work with dependencies? Contact layout editor depends on APIv4 and stores data.

[totten]

@colemanw Just a small concern - uninstalling an extension is supposed to remove all data associated/created by that extension. Clearly in the case of api4 that won't be a problem and we are probably "vetting" each extension that is marked obsolete. But we should be aware of this risk

Right. I had a similar thought recently when skimming the deactivation code. To be concrete - a hypothetical case raising that issue would be importing mosaico. It has a table civicrm_mosaico_template, which poses two issues with upgrading:

1. We don't want to discard the table produced by the extension.
2. The extension may have different versions, and the schema may vary with version. At the time that site-builder upgrades, they could be starting from different permutations of coreVersion x mosaicoVersion - which implies that they should run the same schema updates proscribed by intermediate versions of mosaico.

Of course, we may never have to deal with this - it's just a hypothetical. The five actual cases to date have all been essentially-stateless extensions. And if/when we deal with it, other factors could have changed.

In the spirit of "we should be aware"... we could add some sign-posting around extension-compatibility.json to advise future-devs that this mechanism is only spec'd to handle stateless extensions.

[totten]

How would this work with dependencies? Contact layout editor depends on APIv4 and stores data.

@MegaphoneJon IIRC, the extension system would basically ignore the requirement declared in contactlayout - i.e. https://github.com/civicrm/civicrm-core/blob/master/CRM/Extension/Info.php#L189 would redact org.civicrm.api4 from the list of requirements.

Tangentially, if we had more composer-ified builds doing ext downloads... then I'm pretty sure the correct thing would be for civicrm-core:composer.json to add a replaces directive.

[colemanw]

Those are valid concerns, although as Tim pointed out none of the 5 extensions currently listed as obsolete have any db tables so it hasn't been an issue yet. And yes dependencies are automatically ignored for obsolete extensions.

The extension-compatibility.json file is pretty bare-bones right now but could be built-upon. We could for example add a callback key to fire up a function for handling complex deactivation scenarios. There's lots of potential stuff we could stick in that json file but I've been reluctant to add anything that might be YAGNI because our needs have been pretty simple so far.

But for now how about if I add 2 flags for each extension to make it clear what the system will do (and how to turn off that behavior if a future extension needs to), so it looks like this:

{
  "org.civicrm.api4": {
    "obsolete": "5.19",
    "disable": true,
    "uninstall": true
  },
  "uk.squiffle.kam": {
    "obsolete": "5.12",
    "disable": true,
    "uninstall": true
  },
  "com.aghstrategies.slicknav": {
    "obsolete": "5.12",
    "disable": true,
    "uninstall": true
  },
  "de.systopia.recentitems": {
    "obsolete": "5.12",
    "disable": true,
    "uninstall": true
  },
  "com.ixiam.modules.quicksearch": {
    "obsolete": "5.8",
    "disable": true,
    "uninstall": true
  }
}

[mattwire]

@colemanw Agree with keep it simple until we need to :-) My comments are non-blocking and it seems we're all aware. I like the idea of adding the uninstall flag to the json as that makes it pretty clear to anyone planning on changing that file that something will/will not happen.

[colemanw]

@totten we discussed a 3rd flag called something like "ignore". I've vote for a less ambiguous name (ignore what... the rest of the flags?). Perhaps "suppress" or "do_not_load"

[totten]

@colemanw I've rebased (so that testing would be as realistic as possible) and added an extra commit to create the force-uninstall mode. It felt there was a toss-up in whether the new mode should make the extension completely invisible (e.g. at the CRM_Extension_Container level) or visible-but-always-uninstalled. My sense is that visible-but-uninstalled is a bit kinder on an admin when they compare their file-system to the "Manage Extensions" screen. This also works nicely alongside 25fcba4 - because the extension appears as "Obsolete".

---

For r-run purposes, here's generally what I did:

• Prepare snapshots of old DB
  • Checkout [email protected] and [email protected].
  • Rebuild

    ./bin/givi checkout 5.13 && civibuild reinstall dmaster
    

  • Login. Ensure api4 is enabled.
  • Make DB snapshot.

    civibuild snapshot dmaster --snapshot dmaster513-w-api4
    

• Try CLI-based usage on newer code.

  ./bin/givi checkout master && git checkout colemanw-extCompat && ./bin/setup.sh -Dg
  civibuild restore dmaster --snapshot dmaster513-w-api4 && cv flush
  cv api4 -U admin contact.get +l 1
  

• Try web-upgrade usage on newer code. (Note the double checkout/double flush to ensure that the caches are in old style.)

  ./bin/givi checkout 5.13 && ./bin/setup.sh -Dg
  civibuild restore dmaster --snapshot dmaster513-w-api4 && cv flush
  ./bin/givi checkout master && git checkout colemanw-extCompat && ./bin/setup.sh -Dg
  ## Do web-based upgrade
  ## Check "Manage Extensions" screen
  ## Try APIv4 Explorer
  

With the prior revision, these processes reproduced conflicts on function civicrm_api4(). With the newer revision, it worked despite the presence of the old extension.

[colemanw]

@totten I started writing at length about how this works well with with stateless extensions like api4, but our original plan of INSTALLED_MISSING might be better for extensions that have db tables... but then I thought some more and I think this will work fine with those too. So I've cut my lengthy comment down to just this:

LGTM 👍

[colemanw]

I ran this through an upgrade test and am mostly happy with the results:

Before: Fatal error when upgrading core + api 4.5.0 installed. Cannot upgrade.

After: No fatal error, upgrade proceeds normally. However, the api4 extension remains in the civicrm_extensions table with active = 1 post-upgrade. That doesn't seem right.

[seamuslee001]

This looks like it is ready to merge the testing Coleman and Tim have done lends me to think this is safe any concerns @mattwire @MegaphoneJon ?

[totten]

Updated to address the aesthetic issue about leaving an active row in civicrm_extension.

[mattwire]

@mattwire No concerns from me