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

CiviGrant - Fix upgrade to work on multiple domains #26118

Merged
merged 1 commit into from
Apr 28, 2023

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Apr 28, 2023

Overview

Ensures CiviGrant component is completely removed by the upgrader.

Before

In certain conditions, such as multi-domain, CiviGrant setting was not completely removed by the upgrader step. This is pretty harmless except it was causing problems with #26036.

After

Component enable/disable functions cleaned up. Upgrader fixed.

Technical Details

The CRM_Core_BAO_ConfigSetting::disableComponent() wasn't working as expected by the upgrader because

  1. It's not multi-domain aware
  2. It was short-circuiting on non-existent components, and CiviGrant no longer exists as a component.
  3. Domain settings for components have been consolidated by Make enable_components a non-domain setting #26043

@civibot
Copy link

civibot bot commented Apr 28, 2023

(Standard links)

@civibot civibot bot added the master label Apr 28, 2023
@eileenmcnaughton eileenmcnaughton merged commit 07699f6 into civicrm:master Apr 28, 2023
@eileenmcnaughton eileenmcnaughton deleted the civiGrantUpgrade branch April 28, 2023 06:00
@eileenmcnaughton
Copy link
Contributor

It would be good to fully remove that config property (well nosily deprecate I expect)

image

@seamuslee001
Copy link
Contributor

@eileenmcnaughton the only thing I wonder is if its needed in a pre boot situation at all

@colemanw
Copy link
Member Author

@seamuslee001 I don't think that's an issue because $config->__get() is just a magic function that calls settings()->get. Here's #26121

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

Successfully merging this pull request may close these issues.

3 participants