This is an alternate patch for #1998088: Convert locale_translate_english variable to CMI
And part of #1775842: [meta] Convert all variables to state and/or config systems

Patch description

Instead of just adding a setting for 'locale_translate_english' we keep a list of translatable languages in config('locale.languages') which will allow us to get the full list of 'locale translatable languages' from configuration and looks like this:

# Example for English not translatable, Spanish translatable.
en:0
es:1

The reasons

The reasons, copied from https://drupal.org/node/1998088#comment-7693353
- Other locale components do need the full list of translatable languages injected into DIC somehow. Right now the only way of having it without having a dependency on locale module fully loaded is duplicating the code. See the patch here #1966538: Translation is not updated for configuration, when the config changes

+class LocaleConfigManager {
+
............................
+  /**
+   * Returns list of translatable languages.
+   *
+   * @return array
+   *   Array of installed languages keyed by language name. English is omitted
+   *   unless it is marked as translatable.
+   */
+  public static function getTranslatableLanguages() {
+    $languages = language_list();
+    if (!variable_get('locale_translate_english', FALSE)) {
+      unset($languages['en']);
+    }
+    return $languages;
+  }
+
}

- Since we have that 'locale_translatable_language_list' the fact that LocaleTranslation or LocaleConfigSubscriber don't use that list is an API consistency issue.

Explaining current dependency issues

When locale module is enabled, since the Locale components are loaded into the compiled DIC, configuration events may trigger LocaleConfigSubscriber events, which most likely will have a dependency on locale_translatable_language_list(). There are chances though, that these configuration events are triggered before the locale module is loaded.
Atm we are blindly loading language configuration overrides -or attempting to- for all languages, also for those that are not translatable. This 'problem' goes unnoticed for now because it only relies on such configuration overrides existing, though it is certainly a waste of resources. But it cannot be ignored when configuration updates trigger complex translation update events, like searching for translatable strings inside the configuration.
This applies too to the LocaleTranslation component, that is used from t() and right now cannot check whether the requested translation is for a 'translatable language' (other than English for which a variable is used). This is ok atm because all the configured languages -with the exception of English- are meant to be always translatable but will prevent future usage of the configured language list for other purposes that do not require interface translation.

So I think this issue would be a good chance to fix this variable in a really useful and more generic way. Though not an API addition by itself, it should allow contributed modules to further tweak the list of translatable languages and make it different from the list of content languages.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jose Reyero’s picture

Status: Active » Needs review
FileSize
10.19 KB

And the patch

andypost’s picture

+++ b/core/modules/locale/config/locale.languages.ymlundefined
@@ -0,0 +1 @@
\ No newline at end of file

new line

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -26,7 +26,7 @@ function locale_translate_import_form($form, &$form_state) {
+    if (config('locale.languages')->get($langcode)) {

@@ -149,7 +149,7 @@ function locale_translate_export_form($form, &$form_state) {
+    if (config('locale.languages')->get($langcode)) {

+++ b/core/modules/locale/locale.installundefined
@@ -20,6 +20,31 @@ function locale_install() {
+  $config = config('locale.languages');

+++ b/core/modules/locale/locale.moduleundefined
@@ -272,6 +272,8 @@ function locale_stream_wrappers() {
+  config('locale.languages')->set($language->id, 1)->save();

@@ -322,8 +327,10 @@ function locale_language_delete($language) {
+    if (!config('locale.languages')->get($langcode)) {

@@ -736,7 +743,7 @@ function locale_form_language_admin_overview_form_alter(&$form, &$form_state) {
+    if (!$language->locked && (config('locale.languages')->get($langcode))) {

@@ -806,7 +813,7 @@ function locale_form_language_admin_edit_form_alter(&$form, &$form_state) {
+      '#default_value' => config('locale.languages')->get('en'),

@@ -816,17 +823,7 @@ function locale_form_language_admin_edit_form_alter(&$form, &$form_state) {
+  Drupal::config('locale.languages')->set('en', intval($form_state['values']['locale_translate_english']))->save();

+++ b/core/modules/locale/locale.pages.incundefined
@@ -103,7 +103,7 @@ function locale_translate_filters() {
+    if (config('locale.languages')->get($langcode)) {

Should be Drupal::config()

andypost’s picture

Issue summary: View changes

Updated issue summary.

andypost’s picture

Issue tags: -CMI +Configuration system

proper tags

Jose Reyero’s picture

Changed all that 'config()' to '\Drupal::config()'.
Removed small changes to UnitTestCase, not needed anymore.

andypost’s picture

Slash should be removed because \Drupal use only for classes, module's files always have Drupal

Gábor Hojtsy’s picture

So long as the config is in locale module, how does this resolve dependency on locale module loaded? Do you need this in earlier boostraps? It would certainly not make it available without locale module being enabled.

Jose Reyero’s picture

Locale configuration will be available when the module is not yet loaded, as long as it has already been installed, of course and that would fix the problem.
If the module is not enabled/installed, then there's no DIC locale components loaded and anyway there's nothing to translate yet, but when it is enabled, since the Locale components are loaded into the compiled DIC, configuration events may trigger LocaleConfigSubscriber events, which most likely will have a dependency on locale_translatable_language_list()

Atm we are blindly loading language configuration overrides -or attempting to- for all languages, also for those that are not translatable. This 'problem' goes unnoticed for now because it only relies on such configuration overrides existing, though it is certainly a waste of resources. But it cannot be ignored when configuration updates trigger complex update events, like searching for translatable strings inside the configuration.

In short, by properly implementing current API -aka using translatable language list for config translation operations- we'd have a dependency of the DIC component on the module code -that may not be loaded yet- and that is not good.

Gábor Hojtsy’s picture

Thanks for the longer explanation. Would be a great addition to the issue summary :) Looks like a good approach to me indeed! Great work!

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary.

alexpott’s picture

Let's get #1998088: Convert locale_translate_english variable to CMI in to get CMI nearer to done and continue to discuss further. I've added a patch to that issue that moves us in the direction but just gets the job done form a CMI perspective.

catch’s picture

Title: (Alternate patch) Convert locale_translate_english variable to CMI » Ensure list of translatable languages is available early in bootstrap

I've committed the other issue, so we can continue here now. Re-titling to make it clearer what the change is.

Gábor Hojtsy’s picture

Title: Ensure list of translatable languages is available early in bootstrap » Expand locale_translate_english to full list of translatable languages to avoid circle of dependency
Status: Needs review » Needs work
Issue tags: +language-ui

Given #1998088: Convert locale_translate_english variable to CMI landed, retitling for current focus. Also that means the patch does not apply anymore :/

Gábor Hojtsy’s picture

Title: Expand locale_translate_english to full list of translatable languages to avoid circle of dependency » Ensure list of translatable languages is available early in bootstrap

That was a good new title. Crosspost :)

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes
Issue tags: +beta blocker
xjm’s picture

The configuration variable is already converted, so this issue is no longer a part of #1775842: [meta] Convert all variables to state and/or config systems nor a beta blocker.

alexpott’s picture

Status: Needs work » Closed (duplicate)