Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extensions - Setup <classloader> during "enable" and "uninstall" #20116

Merged
merged 2 commits into from
Apr 21, 2021

Conversation

totten
Copy link
Member

@totten totten commented Apr 21, 2021

Overview

The builds on the theme of PR #20091. Recall from the previous PR that an extension might declare classloaders:

<classloader>
  <psr0 prefix="CRM_" path="" />
  <psr4 prefix="Civi\" path="Civi"/>
</classloader>

and then use them for installation:

function whizbang_civicrm_install() {
  \Civi\Whizbang\Setup::doInstall();
  \CRM_Whizbang_Setup::doInstall();
}

This PR extends classloading support to other stages of the lifecycle:

function whizbang_civicrm_enable() {
  \Civi\Whizbang\Setup::doEnable();
  \CRM_Whizbang_Setup::doEnable();
}

function whizbang_civicrm_disable() {
  \Civi\Whizbang\Setup::doDisable();
  \CRM_Whizbang_Setup::doDisable();
}

function whizbang_civicrm_uninstall() {
  \Civi\Whizbang\Setup::doUninstall();
  \CRM_Whizbang_Setup::doUninstall();
}

For a more complete example, see https://gist.github.com/totten/4864a530b078c6ea86b52d7b578d7c16

Before

Some of these workflows fail:

  • ✔️ Enable WF: cv en whizbang
  • 🔴 Re-enable WF: cv en whizbang && cv dis whizbang && cv en whizbang
  • 🔴 Long uninstall WF: cv en whizbang && cv dis whizbang && cv ext:uninstall whizbang
  • ✔️ Quick uninstall WF: cv en whizbang && cv ext:uninstall whizbang

After

All of these workflows pass:

  • ✔️ Enable WF: cv en whizbang
  • ✔️ Re-enable WF: cv en whizbang && cv dis whizbang && cv en whizbang
  • ✔️ Long uninstall WF: cv en whizbang && cv dis whizbang && cv ext:uninstall whizbang
  • ✔️ Quick uninstall WF: cv en whizbang && cv ext:uninstall whizbang

Technical Details

Why is this necessary? Recall that (for a typical admin) the lifecycle of an extension is:

  1. Enable extension $x
  2. Disable extension $x
  3. Either:
    • (a) Re-enable extension $x
    • (b) Uninstall extension $x

Step #2 disables the classloader for purposes of regular page-loading. However, when you get to step #3a or #3b, then you need the classloader again. Of course, it would be premature to fully activate the extension (e.g. do not fire hook_civicrm_config on a disabled module). But you should have access to the classes.

This is an analog/follow-up to civicrm#20091

Why is this necessary? Recall that a typical admin will go through this lifecycle:

1. Enable extension $x
2. Disable extension $x
3. Either:
   * (a) Re-enable extension $x
   * (b) Uninstall extension $x

Step `#2` disables the classloader for purposes of regular page-loading. However, when you
get to step `#3a` or `#3b`, then you need the classloader again.
@civibot
Copy link

civibot bot commented Apr 21, 2021

(Standard links)

@civibot civibot bot added the master label Apr 21, 2021
@colemanw
Copy link
Member

This makes sense & is well-documented.

@colemanw
Copy link
Member

@totten test failures relate

@totten
Copy link
Member Author

totten commented Apr 21, 2021

Yup. The failure in testInstall_DirtyRemove_Disable_Uninstall was relevant - it indicated that if someone did a "dirty removal" (deleting the code for an active extension), then one could get stuck (unable to disable the missing extension). Pushed a fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants