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.
Comment | File | Size | Author |
---|---|---|---|
#4 | 2052233-locale_languages-04.patch | 9.3 KB | Jose Reyero |
#4 | 2052233-locale_languages-04-diff.txt | 5.99 KB | Jose Reyero |
#1 | 2052233-locale_languages-02.patch | 10.19 KB | Jose Reyero |
Comments
Comment #1
Jose Reyero CreditAttribution: Jose Reyero commentedAnd the patch
Comment #2
andypostnew line
Should be Drupal::config()
Comment #2.0
andypostUpdated issue summary.
Comment #3
andypostproper tags
Comment #4
Jose Reyero CreditAttribution: Jose Reyero commentedChanged all that 'config()' to '\Drupal::config()'.
Removed small changes to UnitTestCase, not needed anymore.
Comment #5
andypostSlash should be removed because \Drupal use only for classes, module's files always have Drupal
Comment #6
Gábor HojtsySo 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.
Comment #7
Jose Reyero CreditAttribution: Jose Reyero commentedLocale 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.
Comment #8
Gábor HojtsyThanks for the longer explanation. Would be a great addition to the issue summary :) Looks like a good approach to me indeed! Great work!
Comment #8.0
Gábor HojtsyUpdated issue summary.
Comment #9
alexpottLet'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.
Comment #10
catchI've committed the other issue, so we can continue here now. Re-titling to make it clearer what the change is.
Comment #11
Gábor HojtsyGiven #1998088: Convert locale_translate_english variable to CMI landed, retitling for current focus. Also that means the patch does not apply anymore :/
Comment #12
Gábor HojtsyThat was a good new title. Crosspost :)
Comment #12.0
Gábor HojtsyUpdated issue summary.
Comment #13
xjmComment #14
xjmThe 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.
Comment #15
alexpottSince #1998088: Convert locale_translate_english variable to CMI landed