Make enable_components a non-domain setting

[colemanw]

Overview

Make enable_components a global setting rather than domain-specific. This is a preamble to #26036 which aims to sync Components with some new core-extensions. Extensions are always enabled globally but Components have been treated as per-domain (even though that's never made much sense). This fixes that discrepancy.

Before

Not possible to have global settings; every setting is domain-specific, even enable_components.

After

Possible to have non-domain settings, and enable_components is the first.

Technical Details

Each setting has 2 metadata flags: is_domain and is_contact. Until now the rule (enforced by a unit test) is that one or the other must be true (xor). That's rather silly because having two xor flags is effectively the same as just one flag (it would be like having two light switches for one light, one for off and the other for on, and a rule that you must flip both simultaneously). But it gets weirder because even for settings where is_contact = 1 and is_domain = 0, they are still treated as domain settings!! So all is_contact setting are ALSO per-domain even though they declare is_domain = 0!
Ugh. Such bad logic. According to the current rules, the is_domain flag shouldn't even exist because ALL settings are per-domain.
However, I'm glad it does exist, because now it's actually going to be used for something. The new logic as of this PR lifts the xor rule and allows both flags to be false; doing so creates a non-contact, non-domain setting (a global setting).
But as a follow-up, I'd also like to fix the bad logic around the is_contact flag. It's silly that is_contact settings must declare is_domain = 0 when in fact they are domain-specific.

[civibot]

(Standard links)

• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[totten]

But it gets weirder because even for settings where is_contact = 1 and is_domain = 0, they are still treated as domain settings!!

I'm skeptical about this point as a general claim about pre-PR status quo. But that's a bit academic compared to the overall concept of the patch. (The value of the basic concept remains.)

Recap: settings are stored in two layers or scopes (domain and contact).To my eye, this creates a third scope (system). System scope seems useful. 👍

I find it a little confusing that this implementation loads the "domain" and "system" scopes into the same storage (while keeping "contact" separate) - and that "system" scope is declared by the absence of any other scope. (For example, if I'm skimming correctly, this implies that setting overrides for "system" level settings would be declared anachronisticly as "domain" overrides - because they're technically merged into the same bucket?)

But perhaps this achieves another useful purpose - e.g. it allows drop-in compatibility with any/most existing code that reads or writes the "enabled_components" from its original domain scope?

[colemanw]

I'm skeptical about this point as a general claim about pre-PR status quo.

What I meant by my statement @totten was that domain_id is applied to all settings, regardless of the is_domain flag. See this line which applies the domain_id clause even for contact settings where is_domain = 0:

civicrm-core/Civi/Core/SettingsBag.php

Line 383 in 84d5298

$dao->domain_id = $this->domainId;

Recap: settings are stored in two layers or scopes (domain and contact).To my eye, this creates a third scope (system). System scope seems useful. 👍

Per my gripe note above, the two scopes are, in practice:

1. Per-domain
2. Per-contact-per-domain

I find it a little confusing that this implementation loads the "domain" and "system" scopes into the same storage (while keeping "contact" separate) - and that "system" scope is declared by the absence of any other scope. (For example, if I'm skimming correctly, this implies that setting overrides for "system" level settings would be declared anachronisticly as "domain" overrides - because they're technically merged into the same bucket?)

I find the bucket system unnecessarily complicated, and I didn't see the value in creating a 3rd bucket. When you set or get a setting, you don't really care what bucket it's in, and needing to know that would be a hindrance to the task at hand of just getting the damn setting. (Side-note, how exactly are those setting overrides supposed to work in multi-domain sites? Is it even possible to have more than one civicrm_settings.php file for multiple domains? And if not, aren't we then setting the same value for all domains?)

But perhaps this achieves another useful purpose - e.g. it allows drop-in compatibility with any/most existing code that reads or writes the "enabled_components" from its original domain scope?

Or any other setting which is added as or converted to global in the future. You don't need to know the scope, you can just set and get the setting and keep your sanity :)

[totten]

Apologies for the length of the response. The PR is kinda tricky to review - in that it sets out one specific setting as a goal (enable_components) but also mixes-in a critique and expansion of the general configuration model. It just means that I have to thrash-about on both levels. 😉

---

domain_id is applied to all settings, regardless of the is_domain flag. See this line which applies the domain_id clause even for contact settings where is_domain = 0

Ohhh, I see what you're saying - that the "contact" scope is actually "contact-within-domain". To judge this, it might help to work an example. IIRC, the typical multi-domain setup maps between web-URLs and database-domains, e.g.

• https://northeast.example.org <=> domain_id=10
• https://southwest.example.org <=> domain_id=11
• https://students.example.org <=> domain_id=20
• https://donors.example.org <=> domain_id=21

As a user who logs into two of those sites, I might expect settings to be separate (different websites!) or shared (same parent website!). There's an example in universe which uses contactSettings() for configurable email signatures. Would I prefer the signature settings on students.example.org and donors.example.org to be different or shared? It depends on the content of the signature and the particular emails...

Either behavior seems sane/relatable to me. I don't really see a (general) way to say that it's better to have contact-settings shared (or unshared) across domains. (Except that the status-quo gets a favorable presumption.)

---

Side-note, how exactly are those setting overrides supposed to work in multi-domain sites?

(My unvalidated supposition...) It's more common to want to override settings across all domains; and if you hard-code the value, then I think it's obvious that it's hard-coded across-the-board.

If you want a more nuanced (per-domain) override, then you have to use some conditional/variable/function, e.g.

$civicrm_setting['domain']['color'] = match($_ENV['HTTP_HOST']) {
  'northeast.example.org' => 'red',
  'southwest.example.org' => 'blue',
  'students.example.org' => 'green',
  'donors.example.org' => 'yellow',
};

---

I find the bucket system unnecessarily complicated ...

But perhaps this achieves another useful purpose - e.g. it allows drop-in compatibility...

Or any other setting which is added as or converted to global in the future. You don't need to know the scope, you can just set and get the setting and keep your sanity :)

Yeah, there is a kind of complexity in have 2-3 SettingsBag objects -- and expecting the caller to choose the right one.

But.... FWIW, I think the complexity arises from allowing >1 scope. After that decision (which predates us...), all you can do is shift complexity into different places.

From one POV, as you mention, it would be easier DX if Civi::settings() provided access to all settings across all scopes. I rather like the sound of that. In fact, if the scope is hidden, then it also opens up new functionality -- scopes could be configurable or cascaded. (Deployment A has mailing_backend at the system-scope; deployment B has mailing_backend at the domain-scope.) Easy and powerful!

Except... each of those scopes depends on other data (e.g. contact ID, domain ID). This data isn't around at the start, and those IDs can be shifty over the life of a PHP process. Instead of 2x-3x SettingBag objects with separate data, the merged bags store overlapping data. In the current patch, this makes them flaky. For example: How would a reader expect this code to behave?

print_r(Civi::settings()->get('enable_components'));
print_r(Civi::settings(2)->get('enable_components'));
print_r(Civi::settings(3)->get('enable_components'));
print_r(Civi::settings(4)->get('enable_components'));

Civi::settings()->set('enable_components', ['CiviContribute']);
Civi::settings(2)->set('enable_components', ['CiviCase']);
Civi::settings(3)->set('enable_components', ['CiviMail']);

print_r(Civi::settings()->get('enable_components'));
print_r(Civi::settings(2)->get('enable_components'));
print_r(Civi::settings(3)->get('enable_components'));
print_r(Civi::settings(4)->get('enable_components'));
print_r(Civi::settings(5)->get('enable_components'));

Looks simple enough. Let's assume the reader is reasonably informed; they know that:

• enable_components is system-scope
• System-scoped data is the same for all domains.
• Civi::settings() provides access to both system- and domain-scoped data.

I would expect all domains to show the same value. From MySQL POV, they do end with the same value. But in PHP, they don't. The last batch of print_r() statements shows:

Array
(
    [0] => CiviContribute
)
Array
(
    [0] => CiviCase
)
Array
(
    [0] => CiviMail
)
Array
(
    [0] => CiviEvent
    [1] => CiviContribute
    [2] => CiviMember
    [3] => CiviMail
    [4] => CiviReport
    [5] => CiviPledge
    [6] => CiviCase
    [7] => CiviCampaign
)
Array
(
    [0] => CiviMail
)

Domains 1+2+4 show incorrect data; domains 3+5 are correct. It can be fixed, but it will add complexity to the implementation of SettingsManager/SettingsBag. (Or you don't fix it - and the behavior is unintuitive - and you have to document this quirkiness - but people don't read the documents - yaddayadda. Similar complexity, different flavor.)

Of course, if the only setting in this scope is enable_components, then it really doesn't get toggled this way, right? Cache-coherence isn't a big deal? It only matters as one branches out to more use-cases.

(In the status-quo for Civi::settings() and Civi::contactSettings(), every SettingBag has separate/non-overlapping data. The equivalent examples have consistent/intuitive behavior. But alas, they don't support system-scope and they don't have a migration-path between domain<=>system.)

---

OK, actual opinions:

(A) Magically Merged Scopes: I definitely see the appeal of a unified Civi::settings() facade. I think you make an excellent point about migration-paths. The upshot is a DX that looks flatter (from the outside) and eventually makes a more powerful configuration mechanism.

(B) Verbosely Isolated Scopes: I also see appeal in going the opposite direction - e.g. kill off the is_domain/is_contact/scoping thing. Domain-settings remain as-is, but system-settings and contact-settings move to separate data-structures. There may be some shared traits/patterns. The upshot is that each entity has simpler implementation.

(C) Mix: As a long-term thing, I'm not keen to stay in a position where domain+system settings are merged but contact settings are split. It feels like we'd end-up with an inconsistent interface -- and more complex implementation (since we basically have to do both merged and unmerged scopes).

I suppose the responsible thing is to mock-up how these are likely to look and get feedback about each mockup. That's a longer game :(

Another option would be:

• Merge this. Call it an interim step toward (A)
• Remain open to further revision/complication
• Be bit reserved about how often this is advertised/used.

[colemanw]

@totten the 5-domain scenario you illustrated above sounds like the bug would be caused by in-memory caching per-domain. I think it's a pretty unusual situation for any application to write a setting to one domain then retrieve it from another domain, all within the same page request! But I take your point, we should take care not to introduce bugs no matter how edgey, and I think this one could be avoided by caching the global settings separately from the domain settings, then array_mergeing them together when fetching domain settings. Perhaps this is what you meant by "(A) Magically Merged Scopes". I support that option, and I second the next step to:

• Merge this. Call it an interim step toward (A)

[totten]

OK, yeah, I'm amenable then.

So I looked a bit more at the upgrade steps. Had a couple concerns but pushed up another patch to address. My configuration was:

## Setup on 5.60
cv api4 Domain.create +v name=two +v version=5.60.0
cv api4 Domain.create +v name=three +v version=5.60.0
cv api4 Domain.create +v name=four +v version=5.60.0

cv vset --scope=domain:1 enable_components='["CiviEvent","CiviMail"]'
cv vset --scope=domain:2 enable_components='["CiviEvent","CiviMail"]'
cv vset --scope=domain:3 enable_components='["CiviContribute","CiviMail"]'
cv vset --scope=domain:4 enable_components='["CiviCase"]'

for N in 1 2 3 4; do cv vget --scope=domain:$N enable_components ; done

civibuild snapshot dmaster

Then switched to master, applied the patchset, and ran the upgrade. This gave me an expected warning about the consolidation. The upgrade merges the settings:

mysql> select id, domain_id, value from civicrm_setting where name="enable_components";
+----+-----------+-----------------------------------------------------+
| id | domain_id | value                                               |
+----+-----------+-----------------------------------------------------+
|  4 |         1 | a:2:{i:0;s:9:"CiviEvent";i:1;s:8:"CiviMail";}       |
|  7 |         2 | a:2:{i:0;s:9:"CiviEvent";i:1;s:8:"CiviMail";}       |
|  8 |         3 | a:2:{i:0;s:14:"CiviContribute";i:1;s:8:"CiviMail";} |
|  9 |         4 | a:1:{i:0;s:8:"CiviCase";}                           |
+----+-----------+-----------------------------------------------------+
4 rows in set (0.01 sec)

-- Run upgrade... then...

mysql> select id, domain_id, value from civicrm_setting where name="enable_components";
+----+-----------+--------------------------------------------------------------------------------------------+
| id | domain_id | value                                                                                      |
+----+-----------+--------------------------------------------------------------------------------------------+
|  4 |      NULL | a:4:{i:0;s:9:"CiviEvent";i:1;s:8:"CiviMail";i:2;s:14:"CiviContribute";i:3;s:8:"CiviCase";} |
+----+-----------+--------------------------------------------------------------------------------------------+
1 row in set (0.00 sec)

I think it would be good if someone with more multi-domain experience (e.g. @seamuslee001 @eileenmcnaughton) could note whether the consolidated setting is acceptable for their use-cases. However, I kinda expect that would've come up in earlier chats if it wasn't. So I'll just mark it "merge ready".

[totten]

civibot, test this please