eileenmcnaughton / deduper

Other
3 stars 6 forks source link

Calling an extension's class in settings is unsupported #23

Closed MegaphoneJon closed 3 years ago

MegaphoneJon commented 3 years ago

I'm still on the fence about whether this is a deduper or firewall bug - or core? I'm leaning toward deduper, but we can move it elsewhere.

In Dedupe.settings.php this troublesome line reads:

    'default' => array_keys(CRM_Deduper_BAO_MergeConflict::getLocationTypes()),

However, this can lead to Class 'CRM_Deduper_BAO_MergeConflict' not found errors on all page loads.

You can replicate this by installing the firewall extension, then deduper, then truncating civicrm_cache.

In firewall_civicrm_config(), there's a call to \Civi::service('settings_manager')->useDefaults();. If the defaults aren't cached (important!) this will call Civi\Core\SettingsMetadata::loadSettingsMetadata() to retrieve them. However, _deduper_civix_civicrm_config() hasn't run yet (if firewall was installed first), so when Dedupe.settings.php is included, you get the Class not found error when it tries to run the troublesome line.

If the determination is that calling an extension's class from *.settings.php is unsupported, the bug is here. However, there are a whole class of bugs I've seen starting this year that boil down to "that file isn't in the autoloader/include path/etc. as early as it needs to be." e.g. core#2729, core#1132 and several others. So maybe this is a core issue?

totten commented 3 years ago

However, _deduper_civix_civicrm_config() hasn't run yet (if firewall was installed first), so when Dedupe.settings.php is included, you get the Class not found error when it tries to run the troublesome line.

As a workaround, this might be worth a try: enable the <psr0> autoloader in info.xml, eg

  <classloader>
    <psr0 prefix="CRM_" path=""/>
    <psr4 prefix="Civi\" path="Civi"/>
  </classloader>

That starts earlier than hook_config.

I'm still on the fence about whether this is a deduper or firewall bug - or core? I'm leaning toward deduper, but we can move it elsewhere.

Yeah, that's tricky. They're all doing something a little funny, eg

  1. deduper has a dynamic default. (Arguably, this setting should be bit more stable -- eg achieve dynamism with a constant-placeholder like `'default'=>'auto'`).
  2. firewall calls useDefaults() prematurely. (Arguably, contrib shouldn't use this method - eg note that useDefaults() has an internal flag which makes it a one-shot method; this differs from useMandatory() which was designed to run as-needed.)
  3. Core has this useDefaults() thing. (Arguably, core makes an odd distinction between different settings/phases. Tho from my POV, this distinction was important to resolving some dependency-loops between the settings and the bootstrap process.).

Some possible mitigations (notes: not all mutually exclusive; also, I've not tested any of these):

MegaphoneJon commented 3 years ago

@totten Thanks for thinking this through. I'm going to test some of these tomorrow.