Fix isMultilingual to use static caching and respect current domain

[colemanw]

Overview

This improves the efficiency and accuracy of the CRM_Core_I18n::isMultilingual function, preventing a large number of duplicate queries on every request.

Before

The function CRM_Core_I18n::isMultilingual apparently had two problems:

1. The results were not cached, and since this function is called frequently, that resulted in repetitive hits on the database.
2. The function did not seem to respect multi-domain setups, as it always used domain 1 instead of the current domain.

After

Fixed.

Comments

This is going to especially help the efficiency of medatata-heavy operations like APIv4, as AllCoreTables calls this function repeatedly.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[seamuslee001]

Do we need to put any kind of statics flushing in https://github.com/civicrm/civicrm-core/blob/master/CRM/Core/I18n/Schema.php#L96 or https://github.com/civicrm/civicrm-core/blob/master/CRM/Core/I18n/Schema.php#L134?

[colemanw]

I feel like I must be missing something because in the file you linked I see the exact same pattern which seems to completely ignore which domain we're on and just arbitrarily select the first one.

It does it here:

civicrm-core/CRM/Core/I18n/Schema.php

Lines 22 to 24 in 8928896

	public static function dropAllViews() {
	$domain = new CRM_Core_DAO_Domain();
	$domain->find(TRUE);

And here:

civicrm-core/CRM/Core/I18n/Schema.php

Lines 46 to 48 in 8928896

	public static function makeMultilingual($locale) {
	$domain = new CRM_Core_DAO_Domain();
	$domain->find(TRUE);

Am I missing something obvious? These lookups do not pass in domain ID before calling find() so wouldn't that always return domain 1? Is this a bug that no one noticed because it's so rare to use multi-domain and multi-lingual at the same time?
@mlutfy ?

[colemanw]

Do we need to put any kind of statics flushing

@seamuslee001 I've switched this PR to use CRM_Core_DAO::getFieldValue() which has static caching and automatic flushing whenever there is a write operation to that object.

[mlutfy]

I never used multi-domain, but the fix seems OK (with my limited understanding of the underlying workings of statics).

[seamuslee001]

Change makes sense to me tests have passed merging as per review from @mlutfy

[colemanw]

Thanks @seamuslee001 I followed up with civicrm/civicrm-drupal#604

[totten]

Not a blocker/revertable thing -- but the function CRM_Core_BAO_Domain::getDomain() returns a cached instance of the active domain record, eg perhaps:

return (bool) CRM_Core_BAO_Domain::getDomain()->locales;

I'm pretty sure getDomain() is used in a bunch of other places, so using it would avoid the extra query.

[colemanw]

@totten the results of CRM_Core_DAO::getFieldValue are cached in a static var.

I think the real problem with this PR is that it switches from reading domain 1 to reading the current domain, and it was an undocumented rule previously to only store locales data in domain 1. That's the part I want to revert but will have to think it through as the first record in the civicrm_domain table isn't guaranteed to have id 1, so it's not as simple as changing $domainId to 1.

[colemanw]

@eileenmcnaughton @seamuslee001 @totten we have multiple discussion threads about this and multiple open PRs.
I've changed my mind about the solution and I think having locales populated consistently for all domains is a good thing.
https://lab.civicrm.org/dev/core/-/issues/1852

[totten]

Aaaah, that makes sense.