Error with Backdrop/CiviCRM automated tests: overrides checking for arrays and objects

[herbdool]

Reported from @seamuslee001 here backdrop/backdrop#3093:

I work with the CiviCRM library and within our daily automatic tests we have been running into an issue with out backdrop integration namely an error

Fatal error: Uncaught Error: Cannot use object of type CRM_Core_Config as array in /home/jenkins/bknix-max/build/build-1/web/core/includes/config.inc on line 457 Error: Cannot use object of type CRM_Core_Config as array in /home/jenkins/bknix-max/build/build-1/web/core/includes/config.inc on line 457

This aims to fix it so that your overrides checking code works for Arrays and Objects.

---

PR: backdrop/backdrop#3093 by @seamuslee001

[seamuslee001]

@herbdool .@totten and I are also exploring exactly why our CRM_Core_Config option is getting checked by Backdrop as well (we think that its likely due to the fact that for the rest method we have already prepopulated the $config variable https://github.com/civicrm/civicrm-core/blob/master/extern/rest.php#L13 before booting backdrop

[herbdool]

@seamuslee001 yeah, odd. Is this a new issue? The override was just merged into Backdrop so I wonder if something was introduced with it.

[seamuslee001]

basically @herbdool the override specifies that $config variable is now global where was it wasn't before so when we were calling $config previously it wasn't interfering with any backdrop code as a) it had no knowledge of a thing called $ config and b) it wasn't in the global space where as backdrop calls it global now

[herbdool]

Right. Now $config is a new global variable so that we can set overrides in settings.php.

A problem with your PR is that we don't want to include anything that CiviCRM created in the overrides variable since it could be used elsewhere like:

public function isOverridden($key) {
    return $this->getOverride($key) !== NULL;
  }

Though I suppose someone would need to call with the key.

[seamuslee001]

@herbdool @totten I have created civicrm/civicrm-core#16702 to try to remove our usage of $config in some pre cms boot locations which i think might also solve the problem perhaps

[herbdool]

@seamuslee001 thanks, I'll take a look. How about the PR here? Should we close it for now? If we keep it I've recommended a change.

[seamuslee001]

@herbdool i think its still potentially useful given from backdrop's POV if something has tampered with $config turning it into an object then it should at the very least silently ignore the thing IMO

[totten]

This could be viewed as a philosophical trade-off between strict and forgiving behaviors:

• Strict: Throw errors on weird/unexpected conditions. Fail hard and fast (so that people know they have to fix something).
• Forgving: Discard weird inputs and proceed. Fail only if you really need to (so that systems continue working as before).

As a personal/philosophical matter, I tend to favor strict approaches. But it's also legit to trace out the practical impacts and see if those are signficiant. Here are a few thoughts/expectations:

• Older/existing versions of Civi don't have 16702. We'll probably have that published in a stable release circa April (5.24.0) or May (5.25.0). It's possible but debatable that we could get it out in a March patch (5.23.x). (I don't know when Backdrop is looking to publish c2967cde3063294d9ad3e51294a8a6891a17817d.)
• The symptom (failure in extern/*.php) is moderately obscure - which is double-edged sword. It limits impact of the problem. But, when there is a problem, it's harder to discover/recognize. (Someone with an old version of Civi and new version of Backdrop won't get any symptoms in the admin UI - they'd discover a problem when someone complains that their data-sync or click-through tracking is broken.)
• (Unverified) Since the problem only affects extern/*-style entry-points and not admin screens, it should be straight-forward to recover - just upgrade. (If there's no WSOD in the admin/upgrade UI, then you can upgrade whenever you discover the need.)
• (Unverified) A site might be able to workaround this problem without a patch/upgrade by explicitly setting $config = []; in their settings.php. (I'm guessing that file is loaded after extern/rest.php but before running Config::__construct.)

[herbdool]

@totten I guess we'll go with Forgiving with this PR :)

[klonos]

Just a tiny nit with coding standards (single-line comment beyond 80chars) + also suggested using punctuation. Once that's fixed, this can go back to RTBC 😉

[quicksketch]

Thanks @totten, @seamuslee001, and @herbdool! So the fix seems adequate and I'm fine with discarding the unexpected input in this scenario. I'd like see the discarding done further up so that when we parse settings.php, if $config is invalid (not an array) then we don't put it into the global $config variable. That way we don't have to add this same check everywhere we check $config in the future.

[quicksketch]

I'm moving this to the minor milestone (1.16.0) candidate because the code for config overrides is only in 1.x and not in the current 1.15.x releases.