CiviGrant - Fix installation of dependencies during upgrade #22881
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(Standard links)
|
|
Should be 5.47 right? But I can test in the meantime. |
|
I'm still getting that type error at . $props is['name'] at that point, and the return value (the value of the 'options' element) is FALSE. Sample stack trace:
|
|
If I disable civigrant, uninstall search kit, reinstall search kit, then enable civigrant, then it's ok and I can see the |
|
So the problem is that the managed entity provided by SearchKit isn't getting inserted? Do you have to do all of those steps to get it to work, or is a cache clear enough? |
|
Cache clear isn't enough. |
Fixes dev/core#3093
This guard was added by 912511a as part of a previous approach to managing hooks during upgrades. This general approach changed with civicrm#17126; so 17126 partially undid this... but it inadvertently had the effect of completely disabling `reconcile()` (because this guard was left).
Before ------ `migrateCiviGrant()` migrates some metadata from core-ownership to core-extension-ownership... and it ALSO activates the extension. However, the extension depends on other (possibly-inactive) extensions. This creates an inconsistent state (where active parts of `civigrant` depend on inactive parts of `search_kit`). In this inconsistent state, `ManagedEntities::reconcile()` fails. After ----- `migrateCiviGrant()` still migrates metadata. However, it initially leaves the extension inactive. So `ManagedEntities::reconcile()` won't try to setup these records. After core schema is fully resolved, then it installs all necessary extensions using normal mechanisms (with normal ordering).
|
Pushed up more fixes. Briefly:
There are longer descriptions in the commits. |
totten
force-pushed
the
civiGrantFixSecondAttempt
branch
from
72a5313 to
458e2d8
Compare
|
So far so good. I'll try a couple more, and make a separate ticket for the civireport menu issue, which I wouldn't call a blocker. |
|
Hmm... it doesn't like upgrading from 5.39.1... but there are higher+lower versions which are OK... |
|
5.39.1 is one of the ones I'm working with. But I went straight up, not in steps like that test. I think that acl_bypass has come up elsewhere and I thought it was dealt with. |
Note: This step was added to the codebase circa 5.47. The underlying schema change actually originated circa 5.46. The step should be idempotent.
totten
force-pushed
the
civiGrantFixSecondAttempt
branch
from
1386dd2 to
448a357
Compare
|
Ok I did another little round on the latest here and I think I've done about as much testing as I'm going to on this so 👍 . |
totten
changed the title
|
@totten FYI - this has caused a regression - analysis in https://lab.civicrm.org/dev/core/-/issues/3119 |
This was referenced Mar 19, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
During the upgrade process the CiviGrant extension may be installed. This ensures its dependencies are also enabled.
Fixes https://lab.civicrm.org/dev/core/-/issues/3093. Alternative to #22861 and #22880.
(Note: PR description filled-in after a couple rounds of editing. Reflects final form.)
Before
The
FiveFortySevenupgrade migrates CiviGrant data and partially installs thecivigrantextension. It does not activate dependencies likeafformandsearch_kit.This leaves the system in an inconsistent state and leads to errors.
After
The
FiveFortySevenupgrade migrates CiviGrant and fully installs the CiviGrant extension along with its dependencies (whatever they may be at time of execution; egafformandsearch_kit).Technical Details
The primary change is to split/move/expand the installation step:
civicrm_extension. This placeholder is inactive (is_active=0).This requires some revisions in
CRM/Upgrade. TheCRM/Upgradesystem builds a queue of upgrade-tasks (specifically, a "priority queue" sorted on(weight,id)). Moving the installation step requires setting theweightexplicitly.