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

ExtensionUpgrades - Skip trying to upgrade missing dependencies #22623

Merged
merged 1 commit into from
Jan 27, 2022

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Jan 25, 2022

Overview

This allows the extension upgrade to proceed without error, even if there are missing dependencies.
The user will be prompted to install the missing dependencies afterward, e.g. #22464

See https://lab.civicrm.org/dev/core/-/issues/3036

Before

All dependencies would be added to the upgrade queue and then the extension upgrader would try and fail to upgrade them even if they were missing.

After

Upgrading dependencies is skipped.

Technical Details

Maybe I'm missing something, but that while loop with the tracking variables and the array_shift() seemed overcomplicated. I had to read it a couple times to figure out that all it does is iterate through each item once. That can be more simply achieved with a foreach loop, so I changed it to that.

@civibot
Copy link

civibot bot commented Jan 25, 2022

(Standard links)

@civibot civibot bot added the master label Jan 25, 2022
@demeritcowboy
Copy link
Contributor

Thanks @colemanw will take a look.

@demeritcowboy
Copy link
Contributor

I think this should be against 5.46 but otherwise this works well for me. After upgrade you get presented with two options and it's not clear which to do first, but the top one says "as soon as possible". Even if you do visit the extensions page first what happens is you get a popup alert about needing to run upgrades. So I think the average admin would run upgrades first, but it does still all work if you install search kit first too.

Untitled3

Untitled2

@colemanw colemanw changed the base branch from master to 5.46 January 26, 2022 01:27
@civibot civibot bot added 5.46 and removed master labels Jan 26, 2022
@colemanw
Copy link
Member Author

Sounds like we could merge this and work out the messaging in a followup PR.

@colemanw
Copy link
Member Author

colemanw commented Jan 26, 2022

I'd like @totten to weight in since this is his code I'm mangling, just to double-check that I'm not removing something important.

CRM/Extension/Upgrades.php Outdated Show resolved Hide resolved
@colemanw
Copy link
Member Author

Retest this please

@colemanw colemanw added the merge ready PR will be merged after a few days if there are no objections label Jan 26, 2022
@totten
Copy link
Member

totten commented Jan 26, 2022

In the screenshot that @demeritcowboy showed above, it includes the status warning about "Extension Error": "Form Builder depends on org.civicrm.search_kit, which is not enabled."

Two things:

  • This message doesn't in 5.46. One needs to backport Extensions - Add dependency status check #22464 to 5.46.
  • The message prose feels a little strange (one extension by name; the other by key?). I'd go for something like this pseudocode:
    "$dependentName" (<code>$dependentKey</code>) depends on missing extension "$dependencyName" (<code>$dependencyKey</code>). 
    Please <a href="civicrm/admin/extensions?reset=1">correct the extension configuration</a>.

I'd say those can fixed in 5.46/5.45 in the scope of dev/core#1306, but they don't have to be in this PR.

Before: Missing dependencies would be added and then the extension upgrader would try and fail to upgrade them
After: They are skipped, as missing extension dependencies are handled elsewhere

This allows the extension upgrade to proceed without error, even if there are missing dependencies.
The user will be prompted to install the missing dependencies afterward.
@colemanw
Copy link
Member Author

@totten I changed that comment to a throw new CRM_Extension_Exception('Invalid extension key... because I think it's so unlikely to happen we really ought to know about it if it does.

@totten
Copy link
Member

totten commented Jan 27, 2022

I'll give one last bit of r-run with and then merge.

@totten totten merged commit fa931b2 into civicrm:5.46 Jan 27, 2022
@totten totten deleted the extensionUpgrades branch January 27, 2022 05:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.46 merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants