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

dev/civicrm-setup#1 CRM_Core_I18n - Don't require immediate bootstrap #11682

Merged
merged 1 commit into from
Feb 19, 2018

Conversation

totten
Copy link
Member

@totten totten commented Feb 16, 2018

Overview

For civicrm/civicrm-setup#1, the general goal is to allow installing the
database schema without needing to run GenCode.

The current draft is crashing because the SQL does translation using ts().
But if you try to use ts() in a pre-boot environment, it will attempt to
boot automatically so that it can read $config->customTranslateFunction.

This is a chicken-egg situation. We haven't yet reached the phase where the
installer can boot up Civi... because we don't have the SQL... but the SQL
can't be generated (translated) because Civi hasn't been booted.

The aim of this patch is to loosen the coupling between ts()
and CRM_Core_Config so that ts() can be used on its own.

Aside: You might ask how this works today -- basically, GenCode does a
database-less-boot, which placates ts(). However, the civicrm-setup
will eventually need to do full-boot, and AFAIK we don't have any
situations where one transitions from database-less-boot to full-boot; I
have a gut fear that such a transition would be its own slipper slope.

Before

  • On first invocation, ts() calls CRM_Core_Config::singleton() to probe for customTranslateFunction.
  • Thereafter, it uses a short test on a static variable to avoid extra lookups.

After

  • During the first 1..n invocations (pre-boot), ts() calls getBootService() to see if we're ready. On the first successful result, it calls CRM_Core_Config::singleton() to probe for customTranslateFunction.
  • Thereafter, it uses a short test on a static variable to avoid extra lookups.

Comments

For r-run testing, I've done a few things:

  • Go to "Administer => Localization => Language". Change from en_US to fr_FR and observe UI displays the new language.
  • Go to "Administer => Localization => Language". Enable multi-lingual (en_US and fr_FR) and enable CMS-language inheritance. In Drupal, setup user preferences. Observe that the active language in Civi tracks the user preference.
  • Run bin/setup.sh -g. Spot-check sql/civicrm_data.fr_FR.mysql to ensure that it contains translated strings.
  • In civicrm.settings.php, create a function swedishchef($string). Go to "Administer => Localization => Language" and use swedishchef as the translation function. Observe that translations use this.
function swedishchef($string) {
  return "Bork, $string, bork";
}

(Note: UI elements in the dashboard are cached and require an explicit refresh to reflect changes in the language settings. The nav-menu gets translated most of the time, but it never seems to use custom-translation-function. However, that's not a new regression -- the same problem occurs without this patch.)

For civicrm/civicrm-setup#1, the general goal is to allow installing the
database schema without needing to run `GenCode`.

The current draft is crashing because the SQL does translation using `ts()`.
But if you try to use `ts()` in a pre-boot environment, it will attempt to
boot automatically so that it can read `$config->customTranslateFunction.

This is a chicken-egg situation.  We haven't yet reached the phase where the
installer can boot up Civi...  because we don't have the SQL...  but the SQL
can't be generated (translated) because Civi hasn't been booted.

The aim of this patch is to loosen the coupling between `ts()`
and `CRM_Core_Config` so that `ts()` can be used on its own.

> Aside: You might ask how this works today -- basically, `GenCode` does a
> database-less-boot, which placates `ts()`.  However, the `civicrm-setup`
> will eventually need to do full-boot, and AFAIK we don't have any
> situations where one transitions from database-less-boot to full-boot; I
> have a gut fear that such a transition would be its own slipper slope.
@JoeMurray
Copy link
Contributor

I have reviewed from perspective of performance and am satisfied it is no worse than current implementation for the vast majority of calls, and have no concerns in other cases of first page load and pre-installed calls.

@eileenmcnaughton
Copy link
Contributor

I read through the code & could make sense of the changes. Relying on @JoeMurray testing. Merging

@eileenmcnaughton eileenmcnaughton merged commit e66cdab into civicrm:master Feb 19, 2018
@totten totten deleted the master-i18n-preboot branch February 19, 2018 23:00
@mlutfy mlutfy added this to the 4.7.32 milestone Mar 6, 2018
@mlutfy mlutfy changed the title (civicrm-setup#1) CRM_Core_I18n - Don't require immediate bootstrap dev/civicrm-setup#1 CRM_Core_I18n - Don't require immediate bootstrap Mar 6, 2018
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.

5 participants