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

Database Upgraders - Continuing support for <5.37? #317

Closed
totten opened this issue Nov 28, 2023 · 1 comment
Closed

Database Upgraders - Continuing support for <5.37? #317

totten opened this issue Nov 28, 2023 · 1 comment

Comments

@totten
Copy link
Owner

totten commented Nov 28, 2023

(This is an off-shoot from discussion with @jaapjansma in https://lab.civicrm.org/extensions/civirules/-/merge_requests/221.)


Definition

The "Database Upgrader Class" (civix generate:upgrader) is a mechanism for managing schema changes on behalf of an extension. One defines a class similar to this (pseudocode):

class CRM_Myextension_Upgrader extends SomeBaseClass {
  public function install() {
    $this->doSomeInstallStuff();
  }

  public function upgrade_1000() {
    $this->doSomeUpgradeStuff();
  }
}

Mechanism (General)

This mechanism relies on two major pieces:

  1. Base Class: There's a base-class with common methods/behaviors. (These are behaviors like "check revision numbers", "execute a SQL template", or "add new SQL column").
  2. Event Wiring: You need to call the class whenever the extension is installed, uninstalled, upgraded, etc. (In other words, listen to "Lifecycle Hooks".)

The approach has evolved:

  • The original (heavier) way to implement these two pieces:
    1. Base Class: civix copied-in a boilerplate base class
    2. Event Wiring: civix provided a series of 2x6 hook stubs (main.php and main.civix.php - each file had stubs for hook_install, hook_enable, etc)
  • The newer (lighter; 5.38+) way to implement these pieces:
    1. Base Class: Core provides a base-class. Civix doesn't need to copy-in the base-class.
    2. Event Wiring: Core recognizes an info.xml tag <upgrader>CRM_Myext_Upgrader</upgrader>. Civix doesn't need to provide 2x6 stubs -- it merely sets one XML tag.

(Aside: Neither of these approaches use "mixins". The goal is similar -- reduce boilerplate. But the APIs+mechanics are different.)

Mechanism (Civix changes)

As of v22.12.1, civix made a full switch from heavy upgrade-classes (<=5.37) to light upgrade-classes (>=5.38). It warns before doing so, but it doesn't provide any alternatives.

  • This change was actually done in 2 attempts.
  • The first attempt aimed to respect the <compatibility> flag. (If the target version was <=5.37, then it would use the original/heavy way.) However, QA was difficult. To do it right, one should alternately raise/lower/raise the <compatibility> flag. After each transition, ensure that several pieces look correct -- and then run the full lifecycle (install=>upgrade=>disable=>re-enable=>uninstall=>etc)... and do that on multiple versions of CiviCRM. Which is a lot QA work. And so there were various bugs. And this wasn't officially released.
  • The second attempt made a "clean break" and focused on 5.38. This simplified testing - making it possible to fix the various bugs discovered in the first attempt. But this meant that 5.38 became a hard requirement (at least, for anyone using a "Database Upgrader Class").

Alternatives

Personally, if I needed to use current civix to make a database-upgrader for on an old CiviCRM, I would consider backporting
civicrm/civicrm-core#20091 + civicrm/civicrm-core#20090 + civicrm/civicrm-core#20116 (and possibly 1-2 bugfixes related to cache-management). The upshot is that the old Civi would run newer extensions. I like that git logs provide pointers on what to do. But obviously the cost-benefit calculus varies based on specific version#s, #deployments, patch-mgmt tooling, #extensions, etc.

Alternatively, if one really needs a more flexible generator... something generate DB-upgraders in either old or new style... then yes, I do think it's possible. After all, we do have working templates for both styles. But I think it sounds easier than it actually is.

  • One needs to address both "Base Class" and "Wiring" (lifecycle hooks). IMHO, the lifecycle hooks are the harder part.
  • Lifecycle hooks are special. They dispatch differently from most hooks. They run at times when the extension is in a transitional phase (somewhere between "active" and "inactive"). Comparing lifecycle hooks to regular hooks... testing lifecycle hooks is more time-consuming, and it's harder to apply mixin/polyfill/symfony-event techniques.
  • One needs to define the transitions/developer-workflows. (What happens when you raise <compatibility> -- does it modify both the main.php and main.civix.php and CRM/Myext/Upgrader.php? What happens when you lower the <compatibility>?)

I suppose... the smallest adaptation might be something like...

  • When civix upgrade hits the incremental-upgrade step (22.12.1.up.php), it asks if you want to convert. But it gives the option to skip conversion.
    • If you do conversion, then all the current behaviors are fine.
    • If you skip conversion, then the current behaviors aren't fine. Expected interventions:
      • Update the template for myext.civix.php to conditionally provide lifecycle-stubs. (If there is an Upgrader class and if info.xml does not specify <upgrader>, then generate stubs for hook_install, hook_uninstall, etc.)
      • Leave the stubs in myext.php as-is.
      • Leave the file CRM/Myext/Upgrader/Base.php as-is
  • In various commands (like civix upgrade, civix generate:module ., civix generate:upgrader), do a validation check. Show a warning if it looks like the code uses the old style.
  • If you change your mind and want to do the conversion, you could run civix upgrade --start=22.12.0
@totten
Copy link
Owner Author

totten commented Jan 10, 2024

I believe the issue became moot when CiviRules made a major-version branch. (A maintenance branch for <5.37 with older civix templates; another branch for >5.38 with newer civix templates.) If someone wants to purse, then... I guess comment+reopen...

@totten totten closed this as completed Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant