Convert locale_translate_english variable to CMI

Adding tags

hello,
i look at this ;)
i 've got correction, the time to test the patch and i post it ;)
i make correction on my patch (only space change option ) and it's goog ;)

yes i still work on this ;).
comming soon

Hello,
I create the patch, i hope is good for you. If not tell me i recreate it ;)
Have a nice day
Eric

Hello

Patch in #5 doesn't apply now. Re-rolling

I think that is not necessary use update_variable_to_config, given that this variable doe not exist in D7.

Regards

it was my issue, I did not have too much time these last time to fix my patch
too bad, it'll be for next time
sincerely,
Eric

Shouldn't this use Drupal::config() instead of config()?

#15: Convert_locale_translate_english_variabletoCMI-1998088-15.patch queued for re-testing.

+++ b/core/modules/locale/tests/Drupal/locale/Tests/LocaleTranslationTest.phpundefined
@@ -46,7 +46,7 @@ protected function setUp() {
+    $translation = new LocaleTranslation($this->storage, $this->cache, $this->lock, $this->getConfigFactoryStub(array()));

I am sorry but this is not the right fix.

Sorry to show up a bit late here but I think what we'd need is the 'locale_translatable_language_list' in the configuration. And if that's ok, then 'locale_translate_english' would be just one one element of this array. My reasons:

- Other locale components do need that full list 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.

So I think this issue would be a good chance to fix this variable in a really useful and more generic way. My intention is to post a patch in a few hours for this provided you guys agree. Otherwise I'll just keep off this one and work on other workarounds for the problem.

Alternate patch is here, #2052233: Ensure list of translatable languages is available early in bootstrap

Fixing tags.

Also, is there a good reason why you can't just post your patch from #2052233: Ensure list of translatable languages is available early in bootstrap here and close that issue? Having duplicate issues is confusing.

So let's prepare the ground for #2052233: Ensure list of translatable languages is available early in bootstrap but do the minimum to get rid of the variable.

Also added missing upgrade path.

Upgrade path looks good. The schema for the config is incorrect IMHO though. It is said to be a sequence however its a language code / boolean pair. The way the schema is expressed, this is not an associative list. Otherwise looks good to me.

The patch in #24 looks like a step ahead to me, just that schema thing @Gabor mentions.

Hmmm... so the thing is if the enable_translation is meant to be become a list of languages then isn't that a sequence.

Here's an idea - how about we change this around and make it a list of languages that have translation disabled.

+++ b/core/modules/locale/config/schema/locale.schema.yml
@@ -7,6 +7,12 @@ locale.settings:
+          label: 'Langauge'

Just so it's not overlooked before commit: Typo Langauge -> Language.

I think it looks good outside of that typo :)

Fixed that typo only. I think this one is good to go and can be further refined in #2052233: Ensure list of translatable languages is available early in bootstrap.

Interest test fails due to assumptions made about locale config :)

Well that was very fun. The WebTestBase:setup() method was writing an incomplete version of locale.settings.yml to the active directory so that the translation.path can be overridden whilst testing.

Looks good to me, but the language check in getStringTranslation() could be simplified. I mean instead of:

+    if ($langcode == Language::LANGCODE_SYSTEM || ($langcode == 'en' && is_null($disable_translation)) || (is_array($disable_translation) && in_array($langcode, $disable_translation))) {
       return FALSE;
     }

We could have:

+    if ($langcode == Language::LANGCODE_SYSTEM || $disable_translation && in_array($langcode, $disable_translation)) {
       return FALSE;
     }

But yes, anyway we get both features for the same price :-)
I mean, other (contrib) modules can just add languages to that list.

However, this may look a bit twisted, but for the follow up patch I'd rather convert the list to a list of 'translatable ones'. One of the points of the other patch is being able to easily pull the list of translatable languages from configuration, we need to iterate over them in a few places, and by having a 'non translatable' list, we need both the full language_list() and the 'non translatable ones' for that. And then we may need that logic duplicated in locale module and in the components that go into DIC for config translation/update if we don't want one depending on the other. The catch is at that point, if we do it in two steps instead of one, that will mean an API change, right?

Well, in that case, we would be better off listing the translatable languages not the untranslated languages here :)

I might be totally missing something here, but the comments from #21 onwards seem to be addressing three separate issues.

a) Converting the locale_translating_english variable to CMI.
Drupal is shipped in English and the Locale module allows the user to translate it into their own language. "Translating" from English to English is a bit of a confusing concept, so that ability is hidden using this variable.

b) The ability to easily tell which languages are available for translating INTO, early in the bootstrap and whatever modules are enabled. This can already be calculated by calling language_list() and removing English if appropriate. For good data integrity you should avoid storing something if you can calculate it, so I don't think we should be creating new config for this, but we might want to move locale_translatable_language_list() from locale.module to language.module.
(I'm a bit confused about the dependencies here, it doesn't seem correct to be calling Drupal::config('locale.settings') when you can't call locale_translatable_language_list(), so the proposed variable wouldn't have removed the dependency).

c) The ability to turn translation off for additional languages. Hopefully, once (a) and (b) were fixed there would be no more need to hard code 'en' into if statements, and this could be done entirely inside locale_translatable_language_list().

My suggestion is that this issue returns to the approach in #19 to fix (a). We then address (b) in #2052233: Ensure list of translatable languages is available early in bootstrap and (c) in a further followup if there is a use case for disabling other translations.

Yeah I agree with #37. If the only thing that's ever going to be configured is whether English is translatable or not, that should be the only thing in the configuration.

Bumping this to critical since it's one of the very last conversions at the moment And re-uploading #19 for the bot.

1. +++ b/core/modules/locale/config/locale.settings.yml
   @@ -1,4 +1,5 @@
    cache_strings: true
   +translate_english: 0
   

   this is a boolean variable so the default value should be false instead.

2. +++ b/core/modules/locale/locale.module
   @@ -818,7 +818,7 @@ function locale_form_language_admin_edit_form_alter(&$form, &$form_state) {
   +  Drupal::config('locale.settings')->set('translate_english', intval($form_state['values']['locale_translate_english']))->save();
   
   @@ -828,7 +828,7 @@ function locale_form_language_admin_edit_form_alter_submit($form, $form_state) {
   +  return Drupal::config('locale.settings')->get('translate_english');
   

   We do add slashes also to Drupal in procedural code.

Thank you!

Committed/pushed to 8.x, thanks!

D8MI tags.

#2052233: Ensure list of translatable languages is available early in bootstrap explores the expansion of this variable (as it was for a while above) to have a full list of translatable things to avoid circular dependencies / allow for earlier caching, etc. See you there. :)