Non-English Drupal sites get default configuration in English, edited in English, originals not actually used if translated

I confirm this is a problem as described in the summary and have checked it in Spanish also. I am moving this to 'base system' and bumping to major.

The patch in #3 fixed it for me. The natural place for a test is in SiteMaintenanceTest.

The problem is repeatable using the locale module to translate the interface, so this should help with testing.

This is a test-only patch, however, it does not catch the bug.

+++ b/core/modules/system/src/Tests/System/SiteMaintenanceTest.php
@@ -112,5 +113,32 @@ function testSiteMaintenance() {
+    $edit['site_default_language'] = 'nl';

'es' was the enabled language on line 121. Use 'es' here or 'nl' there to fix that failure.

We could try that as SiteMaintenanceMultilingualTest.

+++ b/core/modules/system/src/Tests/System/SiteMaintenanceMultilingualTest.php
@@ -0,0 +1,66 @@
\ No newline at end of file

A new line is needed at the end of the test file.

That fix doesn't make sense to me.

getEditable() should only be used when changing configuration, that is not what we are doing here. changing this means that it will ignore overrides that seem to be somewhere, but that also means it ignores translations.

My guess would be that you simply need to change the translation, which currently might not be editable in the UI.

Or maybe the maintenance mode is checked before the proper language negotation happened, and then config translation doesn't work as adviced. Either way, this doesn't seem correct.

I'd suggest to expand the test, add a second language and try to save the default and a translation, both should work.

I ran into this issue in 8.0.0-beta6, updating issue to reflect that this isn't just in the dev tree.

The version needs to stay at 8.0.x-dev, that is where it will be fixed.

Confirmed, this is a bigger problem. I think this is a critical issue with the whole config translation/config overrides systems.

This is what happens:

You install e.g. DE, then the installer automatically downloads translations for german. if you do a configuration export, then you can see that there is config_BLA/staging/language/de/system.maintenance.yml. Now, the backend only lets you edit the english original, and not the translation, but the frontend will only ever show the translation.

That doesn't make sense at all. This affects any configuration with a default translation. adding a bunch of tags ;)

Maybe the installer should not save the translations as overrides, but actually update the default configuration, possibly update the language.

I have a feeling that all our configuration edit forms need a better understanding of the current language/default language, and the language of the config file they're editing, and properly deal with that.

I think we need a lot more test coverage for various scenarios here, like installing in the a different language and then editing default configuration and translations, multiple languages with a language other than en as default, and en being a language the site is translated into, changing the default language (this is where the real fun starts, and something that D7 and earlier has been fighting a long, long time with...)

Oh, for the record, you should be able to work around this by enabling config translation, possibly add a second dummy language, then you can edit the german translation of this in the UI ;)

Maybe the installer should not save the translations as overrides, but actually update the default configuration, possibly update the language.

AFAIK the current behavior is on purpose and avoid further problems mixing configurations in different languages.

Oh, for the record, you should be able to work around this by enabling config translation, possibly add a second dummy language, then you can edit the german translation of this in the UI ;)

Right now IICR we hide the translations operations if there is only one language.
So may be a viable fix is enabling config_translation module on install and enabling the translations operations if the default language is not English?

Right, but that makes no sense. If you have a single language, then forcing you to use the config translation UI, while offering you to edit the original, that is not actually ever used, but only for existing config where there is an existing translation override, while new configuration will be in the default language... that's just immensely confusing?

@Berdir: in Drupal 7 and before people were freaked out that their default config, eg. views, content types, fields, etc. were half-translated when installed and then regardless of what .po files they downloaded and imported, those "translations" never got updated. The Drupal 8 solution is we keep whatever shipped configuration there was and save translations as appropriate. This allows translators to provide updates for your default view, content type, etc. translations after the fact and its not that you are stuck with the translations you got when you installed / enabled a module. It also allows us to install the module and THEN download and import translations rather then needing to make sure we do it the other order (and refuse to let you install modules if we cannot download translations or lock you into an untranslated / half-translated broken situation).

That we do not rewrite default configuration with foreign language stuff is by design to avoid all these problems.

The solution proposed in the patch does not make sense in the config infrastructure in Drupal 8 therefore setting back to active.

BTW the original bug may very well stand, possibly for reasons indicated by @Berdir in #23, but I doubt that a single configuration translation not loaded for the offline message qualifies as critical.

Attempted to reproduce a language problem. I installed Drupal 8 beta6 in Hungarian. The Hungarian translation for the maintenance mode exists. I enabled config translation, edited the maintenance mode translation, saved it. It appears as edited in the translation on the page when in maintenance mode (and anonymous). Went to locale module to edit it there too. Again, it appeared as edited the second time as well on the page when in maintenance mode (and anonymous). So I could not reproduce any specific bug as it belong to the maintenance mode setting and translations.

Started updating the issue summary but still needs to be updated with the tech background, so you can all brainstorm.

If someone can come up with a shorter title, that is also welcome.

@Gábor: Even in your test you're working around a problem that new users will face right off. Given a site installed in another language, going to the page to enable the offline mode prompts you with a big text box for the content of the offline message. Filling in this box has no effect and the site displays the default string for your other language instead. You went straight to the translate UI and entered a custom string, but knowing that step has to happen is both counter-intuitive and not the interface that is presented to the user. The big box that comes up with a message should be the one that gets used.

Added more about the problem to the IS. Have an idea for solving some of the problems, but need an hour or so for something else. Should be back after that posting the idea.

@alerque: I don't contest that it is counter-intuitive or that it would be great to improve/fix. I am saying that new counter-intuitive things looks more scary than old ones, because you already got used to the old ones. I lost count of how many issues we closed as wont fix in Drupal 7 arguing that when they updated their interface translations, their menus updated but their content types and fields did not. Or they installed in English and then added a language and imported translations and their search page was translated but their content creation page (content types and fields) were not. Knowing Drupal 7 well the fact that those are "user entered configuration" even though you never entered them does not sound ridiculous anymore, but its just a different look at the same problem set. We resolved that in Drupal 8 and traded those counter-intuitive things with other counter-intuitive things. Looks like this will be the issue to figure out if it was worth it and what is possible (AKA worth) solving in Drupal 8 before release.

@all: so the reason we have the original config in English is to keep a reference value.

1. We compare the install storage with the active config to figure out which ones to translate (to skip the strings you modified and don't attempt to translate strings for things you added to the config).

2. On the other side, we keep the interface translation as reference for the interface translation which mostly come from the community. If you modify a translation locally (in the interface translation UI or with the config translation UI), we save it back to this reference as customized, so further localization updates will not update it anymore. The user wanted to manually change it so we keep it as to what it was changed manually.

Here are some use cases we satisfy currently in Drupal 8 to see what we are up against:

A. Be able to translate any configuration to any other language anytime.
B. Be able translate shipped configuration with localize.drupal.org / interface translation module.
C. Be able to update translations to shipped configuration (considering people assume it as part of the software) anytime.
D. Be able to modify translations locally and keep those translation modifications protected from these updates.
E. Be able to modify the configuration and still able to apply translations to parts of configuration which were not modified from the original (ie. if I modify the default value for article image fields, it should still get the label translated properly and get any updates to the translation of that label).

So we have a system where the order of installation / config import and translations does not matter and you can edit your configuration or your translations and its all respected. Here is the same in other words on a figure:

The user experience concern is that we do this by keeping the English (copy of original shipped config) as the active configuration and editing that is in some ways pointless where translations apply. It is probably even more confusing to need to edit the English view to add new fields and then either doing that in your own language (and ending up with a mixed language view) or English even though you don't have English on the site.

Still to post an alternate approach for discussion but need to figure out some rough edges.

We satisfy all of the use cases in #37 currently in Drupal 8. A solution that would not allow some of these would be a regression, so need to be considered if such regression is acceptable compared to the experience gain to achieve.

The proposal above is to have active configuration saved in the default foreign site language of the site instead of imported in English. Let's work from there. That means, we apply translations at first when importing config. That does not sound bad. What comes out of that?

1. Satisfying use case B needs more work if you add English later: If we have English on the site (and the shipped config is English), we can import the default config in English (as it is now). If the site does not have English, then we translate the default config while importing. If English is added later, then we loose the availability of the English translation of that config. We then need to go through the original shipped configs and extract their English sources and save those as the English translation (for unmodified config strings). This would be a new custom "translation" process that is not implemented in Drupal 8 now.
2. Further modification to satisfy B: To be able to translate to other languages, we need to make changes to how we compare active config to shipped config, we need to ignore string changes. So if you modified your node admin view to say "Headline" instead of "Title" for your node lists in whatever language, we'll still keep translating "Title" in other languages. I think this is a small price to pay, not concerned for that, but we should keep it in mind.
3. Modification for use case C: now would need to update active config for shipped config that was translated and update language overrides for other languages. So it would need to be aware of both active config and language overrides. Now its only aware of language overrides and how to structure them.
4. Modification for use case D: When editing and saving or importing original configuration, if that had a shipped configuration as well, we need to also save string changes as custom translation. Will need to compare the active config to shipped config to weed out strings that are still original English and not save those as customized translations. Should only save those as customized that are actually translated.

(Use case E is already satisfied with the above modifications if I look at that right). Please help poke any other holes at this.

A very important change in this system compared to current would be configuration translation would fall back to NOT ENGLISH but whatever happens to be the language of the active configuration instead. Ie. in this figure, German would fall back on French and not English (even after you added English). For modules you installed after adding English, the default config would then be English and those would then fall back on English from both French and German.

(Side note: that we compare shipped config with active config is not 100% foolproof either, shipped config may change in a module without the active config changing and we would still need to compare active config to the shipped config of the time when the active config was originally imported. So ideally we'd keep a copy of the shipped config just to compare for localization but that is a problem regardless of this issue).

This must be an upgrade path issue since it is likely installed configuration will change in some way.

Opened #2428045: Comparing active storage to install storage is problematic, install storage may change anytime as the first sub-issue which is an existing problem but is less severe given that we don't expect the active default config copies to change much. Once/if we make them be quite different (AKA translated) then that increases the change factors that break translations so we need to fix the other end of the system.

Did not yet open the rest of the sub-issues pending the critical triage.

Updates issue summary with edited version of #37 and #38, so its in one summary. Retitling as META because this will involve multiple sub-issues and will not be one mega issue.

@catch and I have discussed this issue and agree that is it critical because it prevent someone from installing Drupal and it working as expected - i.e. changing something that has translated in config is impossible without installing config_translation. And even if you do enable config_translation the mono-lingual non-english experience is exceptionally weird since you still see the english translations.

Can remove the triage tag then I guess. I have some work done in #2428045: Comparing active storage to install storage is problematic, install storage may change anytime which found #2429385: locale_system_remove() should not attempt to remove strings from the locale DB as well for stuff to review for this issue. Will start opening issues for the changes required.

Opened #2429651: When adding English, translated default config should get English translations extracted too.

Also it would be a good time for issue followers to evaluate the solution proposed, if they agree with it before we put too much time into implementing it :)

So I thought #2429651: When adding English, translated default config should get English translations extracted will be a walk in the park once I sit down to do some coding. Then I got lost in the maze of what is responsible for what in the codebase. There are 11 (eleven!) different ways the configuration translation and/or the locale db may change where they need to sync their changes for consistency. This involves some thing that are not done right AFAIS, but mostly it involves the LocaleConfigManager and the LocaleConfigSubscriber. For direct config translation changes the later works with the LocaleConfigManager to figure out what to save back to locale if anything. For indirect config translation changes, LocaleConfigManager has methods to update/delete config translations and it works with the LocaleConfigSubscriber to let it save the translation the right way:

Storing configuration translation within the configuration would involve listening in on events on the active configuration and writing back to the active configuration. These go through LocaleConfigManager so that needs to be able to distinguish and figure things out probably encapsulating this more than it did before, so those invoking it will need to know less about the intricacies of where does the translation go at the end.

So we'd need a real LocaleConfigSubscriber that actually deals with config events (rather than override events) (and rename the override subscriber to what it is). Then when we listed in on active config changes, if those are to shipped config, we need to reflect that in locale with an updated LocaleConfigManager. Not sure yet if we need to do something when deleting translated active config, the things we do on removed modules is already a bit sketchy for example...

So my original plan was to break this down to sub-issues and resolve them one by one. However in #2429651: When adding English, translated default config should get English translations extracted I figured out that doing so required workarounds to be implemented temporarily which were not worth it. So I decided to go ahead and implement the whole concept. This means it will be harder to review and more changes at once, but on the other hand the whole concept can be tested and evaluated without awkward partial solutions somehow coming together.

The stage I got to in #2429651: When adding English, translated default config should get English translations extracted is mostly working but still fails at a few places. Here are the main changes:

1. When locale module is installed, if the site language code is not English, we need to update all copies of default config imported before to "pretend" they are in the site default language. This should ensure that whenever you install locale module in the install process, you get all your default config properly localized. This is implemented as a hook_modules_installed/hook_themes_installed.

2. When installing modules after locale module, all newly imported default configuration should get the same update treatment. Implemented in the same place as 1.

3. When running configuration updates based on locale changes (see figure above, where we go into LocaleConfigManager), we need to update both translated active config and config overrides. The active configs that "pretend" to be in a language get their translations merged in here. To put this to where it should be, I moved this code from locale_config_update_multiple to LocaleConfigManager::updateConfigTranslations().

4. The concept of taking only the parts of active config that stayed the same as the default config was fragile even before. You may not even be able to match schemas if key parts changed and you end up with unprocessable goo. So what's cleaner is if we extract all translatable text from the default config and then filter that through the active config. We cannot depend on the values being the same anymore, so we only check the keys are still the same. This is a bit fragile for sequenced values (eg. allowed value configuration), but I don't know how to make that less fragile. At least the prior fragility of the config schema matching is gone.

5. Instead of using some typed data foo in a LocaleTypedConfig and then redoing it again in LocaleConfigSubscriber and to some degree also in LocaleConfigManager, I opted to use a nested array of TranslationWrappers to transport the translatable source data. That allows us to extract all the English originals (when you add English on a translated site) and also to translate it as needed, although we actually translate it with our own logic (which I kept from before) to save proper locations, etc.

6. Previously LocaleConfigManager was only invoked for translatable languages. We now need to possibly create English language overrides even if English was not translatable, so we needed to move that check to the manager. I attempted to add plenty of comments to make this clearer.

7. The interaction between LocaleConfigManager and LocaleConfigSubscriber was pretty confused. To avoid confusion in the future, I spent considerable time to document their roles and how they interact. Renamed the status flag in LocaleConfigManager and explained it on the property and the method. This now does less useless work when the initiator of action is LocaleConfigManager, the LocaleConfigSubscriber should not fire back data to locale storage, that is pointless to even check.

8. When removing modules, the locale translations should not be removed, I deleted that code from the LocaleConfigManager. The strings are not unique to default config, they may be used for other things, and we simply don't know, so we need to keep that as-is.

7. LocaleConfigSubscriber also got the delete listener removed for the same reason (although this pertains to config overrides). The listener is renamed for better clarity. A new listener is added for active config writes.

8. I used the same TranslationWrapper based solution here which simplified a lot of the code. We can update locale based on both active config and config overrides with almost the same code using this.

9. I noticed that LocaleConfigSubscriber totally ignored supporting string context for translatable strings. Duh. Now it supports it.

Ps. those interested in process archeology can look at #2429651: When adding English, translated default config should get English translations extracted for the process of how I got here. Closing that now as duplicate.

If the string did not have a translation yet, we need to create one. That was in the old code but I did not carry it over before. Now implementing that in the right responsible class, LocaleConfigManager, which makes it possible to keep the central cache.

Fix ConfigNamesMapperTest by updating the expected arguments to hasTranslation in the test (signature changed) and remove a leftover debug.

1. Renamed LocaleConfigManager's service name to be more in line with the rest of the naming patterns. From locale.config.typed (was very nonintuitive) to locale.config_manager.

2. Fix LocaleConfigManagerTest, the hasTranslation() method takes different arguments now.

3. Fix LocaleConfigTranslationTest.php by looking at the resulting translations directly instead of inspecting them via the config manager. This way we test the actual output is done, which is the whole point of LocaleConfigManager in the first place.

@catch, @effulgentsia, @webchick, @xjm and I discussed this. We agreed that is critical because installing Drupal in a language other than English should not prevent a user from changing configuration.

Fix logic in LocaleConfigSubscriber::saveCustomizedTranslation() at least as it pertains the active configuration integrated solution now. It does not apply to overrides because overrides don't save strings that are the same anymore. There are some fails involved with those not updating in locale, those still need to be resolved.

Hopefully resolving InstallerTranslationMultipleLanguageTest. I cannot reproduce the exact fail locally but the fail reported by testbot makes total sense. We now save the translation to base config not to override when installing foreign.

We have the same amount of fails, but the ConfigSchemaTest is new, so that's why. That test does not even need locale module, so that is easy to resolve.

- Fix locale integration in LocaleConfigSubscriberTest. Both saveLocaleTranslationData() and deleteLocaleTranslationData() were affecting locale storage. That is not true anymore. The prior implementation did feed back to the locale DB in those cases when refreshing from locale, but that was only practical for tests. In a real sceniaro, when we initiate updating config translations from locale, we already have the data in locale, no need to write it back again. So writing the data there here.

- Fixing LocaleConfigSubscriber::setUpTranslation() which worked directly with locale and not LocaleConfigSubscriber, so the local cache in LocaleConfigSubscriber was not updated. Now updating that cache properly.

- Changing the assumption that when a custom string translation was already available in a config override, that would not be treated as customized translation. It is. Unless the locale source had the same string earlier, it is custom.

These fix almost all fails in LocaleConfigSubscriberTest.

Started going through this patch, but did not finish will try to finish this until Wednesday.

First off the API changes in this issue need to be documented in the issue summary.

In general this patch touches a lot of different code paths and a lot of the changes do not come with documentation or reasoning why they were made. All the additions to LocaleConfigManager are also not unit tested which makes me especially uneasy because of some of the inconsistencies I noted below.

Here's some concrete points. They jump around because I tried to follow the code path and not the patch from top to bottom.

1. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
   @@ -60,6 +60,29 @@ public function __construct($string, array $arguments = array(), array $options
   +  public function getString() {
   ...
   +  public function getOption($name) {
   

   I'm not really sure adding these (especially in this issue) is necessary. They are used in getTranslatableData() to create TranslationWrapper objects, just to decompose them in processTranslatableData() on the very next line of updateConfigTranslations(). This is seems rather unnecessary and overly complex.

2. +++ b/core/modules/locale/src/LocaleConfigManager.php
   @@ -7,33 +7,54 @@
   + * The saved changes result in configuration events to fire, which would make
   + * LocaleConfigSubscriber write back to the interface translation storage. To
   + * stop that from happening in case we initiate the writes from here, the
   + * $isUpdatingFromLocale flag is maintained, which is queried by
   + * LocaleConfigSubscriber.
   
   @@ -66,11 +87,17 @@ class LocaleConfigManager {
   +   * In this case, LocaleConfigManager is in control of the process and the
   +   * reference data is locale's storage. This is used to let
   +   * LocaleConfigSubscriber know that it does not need to feed back to locale.
   +   * On the other hand, when not updating from locale and configuration
   +   * translations change, we need to feed back to the locale storage.
   
   @@ -330,29 +402,203 @@ public function translateString($name, $langcode, $source, $context) {
   +   *   If TRUE, LocaleConfigManager is in control of the process and the
   +   *   reference data is locale's storage. Changes made to active configuration
   +   *   and overrides in this case should not feed back to locale storage.
   

   This seems like a bit too detailed information to be part of the class docs.

   Moreover, we're documenting this now at three places, which doesn't make much sense to me.

   Let's properly document this complexity at the entry point of it's API, LocaleConfigManager::isUpdatingTranslationsFromLocale() and let's move it from the return value documentation into the actual method documentation.

   Then let's add a @see on the $isUpdatingFromLocale property to this method (and perhaps to the setter method as well)

3. +++ b/core/modules/locale/src/LocaleConfigManager.php
   @@ -7,33 +7,54 @@
   + * In turn when translated configuration or configuration language overrides are
   + * changed, it is the responsibility of LocaleConfigSubscriber to update locale
   + * storage.
   

   This on the other hand seems worth noting, but let's add a @see to LocaleConfigSubscriber

4. +++ b/core/modules/locale/src/LocaleConfigManager.php
   @@ -99,61 +126,113 @@ public function __construct(StorageInterface $config_storage, StorageInterface $
   +   * @return array
   +   *   Array of translatable elements of the default configuration in $name.
   ...
   +  public function getTranslatableDefaultConfig($name) {
   +    if ($this->isSupported($name)) {
   ...
   +      return $this->getTranslatableData($typed_config);
   +    }
      }
   

   This can also return NULL if isSupported() is FALSE or a single StringTranslationWrapper() if $typed_config is not traversable. It should always be traversable, but that is never verified, so we can't assume.

5. +++ b/core/modules/locale/src/LocaleConfigManager.php
   @@ -99,61 +126,113 @@ public function __construct(StorageInterface $config_storage, StorageInterface $
   +      $data = $this->installStorageRead($name);
   

   This is inside of a isSupported() check, which in turn calls to installStorageRead() again. So this ends up reads up reading the file and parsing the YAML twice. This is not in the critical path but it's still unnecessarily wasteful IMO.

6. +++ b/core/modules/locale/src/LocaleConfigManager.php
   @@ -99,61 +126,113 @@ public function __construct(StorageInterface $config_storage, StorageInterface $
   +   *   Configuration data children of $element filtered to translatable children
   

   What is "Configuration data children" supposed to mean?

7. +++ b/core/modules/locale/src/LocaleConfigManager.php
   @@ -99,61 +126,113 @@ public function __construct(StorageInterface $config_storage, StorageInterface $
   +    $translatable = array();
   ...
   +    return $translatable;
   

   This should be moved into the instanceof TraversableTypedDataInterface check as otherwise we are returning an empty array for simple config objects, which is nonsensical.

8. +++ b/core/modules/locale/src/LocaleConfigManager.php
   @@ -99,61 +126,113 @@ public function __construct(StorageInterface $config_storage, StorageInterface $
   +   * @param array $translatable
   +   *   The translatable array structure, see this::getTranslatableData().
   

   I'm not sure we can get away with not actually documenting properly what needs to be passed in as the structure here is rather specific.

9. +++ b/core/modules/locale/src/LocaleConfigManager.php
   @@ -99,61 +126,113 @@ public function __construct(StorageInterface $config_storage, StorageInterface $
   +        if ($langcode != 'en' || locale_translate_english()) {
   

   Let's wrap this is a local method for easier unit testing.

10. +++ b/core/modules/locale/src/LocaleConfigManager.php
    @@ -330,29 +402,203 @@ public function translateString($name, $langcode, $source, $context) {
    +  public function defaultConfigLangcode($name) {
    ...
    +  public function activeConfigLangcode($name) {
    

    If we want to keep these methods (see below) they should be named get*()

11. +++ b/core/modules/locale/src/LocaleConfigManager.php
    @@ -330,29 +402,203 @@ public function translateString($name, $langcode, $source, $context) {
    +      return !empty($shipped['langcode']) ? $shipped['langcode'] : 'en';
    ...
    +      return !empty($active['langcode']) ? $active['langcode'] : 'en';
    

    I don't see any point in falling back to 'en', especially since these methods can return NULL anyway. If there *is* a reason it needs to be documented.

12. +++ b/core/modules/locale/src/LocaleConfigManager.php
    @@ -330,29 +402,203 @@ public function translateString($name, $langcode, $source, $context) {
    +  public function isSupported($name) {
    +    return $this->defaultConfigLangcode($name) == 'en' && !is_null($this->activeConfigLangcode($name));
    +  }
    

    I don't see a reason to limit this logic to English shipped configuration. I.e. there could be an install profile which chooses to ship it's default configuration in a different language.

    Also, the second check here is unnecessarily complex. The method always returns something different than NULL if the config object exists at all in the active storage, so this can be replaced with $this->configStorage->read($name) directly (as it will be cast to bool automatically due to the &&).

13. +++ b/core/modules/locale/src/LocaleConfigManager.php
    @@ -330,29 +402,203 @@ public function translateString($name, $langcode, $source, $context) {
    +  public function isUpdatingTranslationsFromLocale() {
    ...
    -  public function isUpdatingConfigTranslations() {
    

    I'm fine with adding the "fromLocale" suffix to this method if people think that adds value. But because locale and specifically LocaleConfigManager interacts with both string translations and config translations the "config" part is rather important and should not be dropped.

14. +++ b/core/modules/locale/src/LocaleConfigManager.php
    @@ -330,29 +402,203 @@ public function translateString($name, $langcode, $source, $context) {
    +      // Only deal with configuration which was shipped.
    +      if (!$this->isSupported($name)) {
    

    This comment is currently not accurate as it deals with configuration that was shipped in English only.

15. +++ b/core/modules/locale/src/LocaleConfigManager.php
    @@ -330,29 +402,203 @@ public function translateString($name, $langcode, $source, $context) {
    +      $active_langcode = $this->activeConfigLangcode($name);
    +      $active = $this->configStorage->read($name);
    

    $active_langcode is just $active['langcode'] plus a fallback (which I find questionable anyway, see above), so this is unnecessarily complex.

    Seems like activeConfigLangcode() can be ditched entirely.

16. +++ b/core/modules/locale/src/LocaleConfigManager.php
    @@ -330,29 +402,203 @@ public function translateString($name, $langcode, $source, $context) {
    +  protected function mergeToActiveConfig(array $active, array $translated) {
    

    It needs to be documented what this does differently from a simple array_merge(). From looking at it, it seems the only difference is that it does not take over keys which do not exist in the first array. So it needs to be documented why this skipping of extraneous keys is important and why those keys can occur in the first place.

17. +++ b/core/modules/locale/locale.module
    @@ -356,6 +359,41 @@ function locale_cron() {
    +function locale_system_set_config_langcodes(array $components) {
    

    Let's not add a new global function please.

@tstoeckler: re #64, sorry most points I would contest:

1. FEEDBACK NEEDED: Right, I use TranslationWrapper as a container for a string and context pair. We can introduce yet one more such container object or pass them around as anonymous arrays (is that better?) or try to generate some fake config schema definition on the fly (sounds like pointless to me). #2363099: Using translated field definition descriptions in entity schema results in different schema definitions, resulting in update.php changes also suggested methods to get out data from a TranslationWrapper, so there were other use cases. It looked logical to do it. The logic before the patch is that the intersection of the active config and the default config was parsed with config schema. Given that the intersection may end up with unexpected data lacking schemas, that likely resulted in half-broken structures. In my patch, we parse the default config only for translatable things and then that result is intersected with active config. But that result is not maintained as typed config anymore, because we could not have type data definitions for the nested arrays of the translatable keys only. So I think we needed a different transport mechanism. We can also pass around the whole typed data structure of the default config and intersect each time with active config when we need it but that sounds like lots of overhead?

2. TODO: We can merge the docs, sure. I am not concerned that it is now documented at too many places, it was not documented at all before, so I actually consider this an improvement. I see it may not be the best one :)

3. TODO: Sure.

4. TODO: Right, we can document that too.

5. FEEDBACK NEEDED: I can avoid adding the methods and instead do everything in-line in the method if that sounds better. It does seem like FileStorage has no caching of the file read, that is true.

6. FEEDBACK NEEDED: The subtree of the data. Better or worse?

7. DEBATED: Don't understand this. Simple config files should stil be lists?

8. FEEDBACK NEEDED: Not sure I get this. In 2 you complained I documented things at multiple places, here you are concerned a @see is not enough?

9. TODO: Ok.

10. TODO: Ok.

11. FEEDBACK NEEDED: They return NULL if the given name does not exist for example or the file was empty. If the file did not have a langcode that is *everywhere* assumed to be English in CMI. Everywhere. The langcode key is optional. This is not something unique here.

12. FEEDBACK NEEDED: This code integrates with locale module, which considers English only as a possible source language. If you want to make this compatible with other source languages, that WAY outreaches the scope of this issue. I also think its elegant to define the support criterion based on the language codes, but we can make this a tiny bit more performant and not use that method.

13. FEEDBACK NEEDED: LocaleConfigManager does not add locale translations and LocaleConfigSubscriber does not write config files, that is the separation of concerns. How is LocaleConfigManager interacts with both string translations and config translations the "config" part is rather important and should not be dropped. true?

14. FEEDBACK NEEDED: Which is true to all of Locale module? The definition of locale module is it translates shipped English to other languages. It never intended to do anything else in Drupal 8? We can certainly repeat it here (again) if that helps.

15. FEEDBACK NEEDED: Not applicable based on the above?

16. TODO: We can improve the @return docs. It currently says Only items from $active will be present merged with alternate value items from $translated. which answers already half of your concern, no? I'll add that active config may have changed since shipped config, which is the reason we don't put in things that don't belong there.

17. FEEDBACK NEEDED: I can modify the existing global functions (two hook implementations) that call this to call each other and then I am not adding a new global function. Introducing a service for this function or starting to add this as a method on the Locale service (which does not have other methods) looks a lot more out of place. This is in line with the rest of the code style of Locale.

OK, so instead of answering that directly and fighting over these minute points I will spend some more time on this and get both a big picture as well as a thorough, in-depth overview. It feels like I will have some high-level criticism of the current approach so I think our time is better spent hashing that out than arguing over specific implementation details. I might not finish that thorough review until Wednesday though, so just a heads up at this point.

Here is an update in the meantime based on the review points I found actionable:

2. Resolved I believe.

3. Resolved I believe.

4. Returning an array() for the !isSupported() case instead now. If its not supported, then there are 0 translatable items. Now also ensuring that we got traversable items before passing in for translation filtering. If it did not end up being traversable, then return an empty array() too. This may happen if there was no config schema.

5. I think logically the isSupported() method makes sense, its also used from the locale config subscriber, so unless we want to reimplement it there too, we need something like this in locale config manager. I eliminated one use of it. It does not resolve your concern here though. I agree its not ideal that it reads the file twice. Any suggestions on how to resolve it?

6. Entirely rewrote.

8. Copied the docs then instead of just the "see".

9. Ok, you will find this hilarious. It does make sense to abstract this logic, because this same condition is in fact copy-pasted in 7 different places in core. So we need a new global function for this one then. (Locale doe not have a class to put these on and that would also require to refactor the function used in the condition to make any sense). New non-critical issues for refactoring locale stuff to an OO model are welcome.

10. Renamed to prefix with get*

11. Documented, but this is the same assumption elsewhere. See for example ConfigNamesMapper::getLangcode().

12. Made it use config storage directly.

14. Actually eliminated this one anyway while reviewing use of isSupported() for 5 above. Given getTranslatableDefaultConfig() returns right away if not isSupported(), we don't need to call that separately.

15. You are right, I think we can use mergeDeepArray given that we already filter to active config keys in processTranslatableData().

Started updating the issue summary.

Added detailed API changes table. Not sure it helps without looking at the code, but it could serve as a good reference.

So locale_is_translatable() is a special case of locale_translate_english(). We can remove the later and then that is not a new global function either, just renamed. The prior issue summary update already included that.

I did not actually use the language manager in the subscriber that I attempted to add, so no need to add it as a dependency.

Need to remove the language manager from the service definition too. Also adding a @todo to the delete case remaining, that needs to be revisited too.

Here is a figure of a sample process that should be supported (I hope already allowed) by this patch. (The same process is allowed without this patch currently in HEAD except using config overrides).

Worked on fixing one of the "delete problems". In this case when locale thinks that an interface translation may be removed just because it ran out of applicable translatable strings. That would happen if the original config was edited to a point where none of the original source strings are there. (In other cases, like customizing all translatable strings, those are still in locale, so those don't belong in this case). The prior patch removed the override in that case which is incorrect. There may still be data in the override for things that reach out of locale's responsibility. We should only remove data that is under our responsibility, not reach over it. So we need to filter the current override through the default config translatable keys and remove the ones we don't have data for anymore and keep the ones outside of our realm.

I don't believe this will fix the other "delete problems" for which we still have the remaining fails.

Also increasing the count of changed config in each case. It was arbitrary that we did not do it before for deletes. The number is used as feedback in reports, so while it does not need to be accurate, this will give a better picture of what happens.

Also update remaining steps.

Attempt to resolve the first delete problem, when items get removed from the override but still present in active config, those need locale updates to not update the override later on. Its fun juggling the default config, active config and override triad in comparisons :D

Wrong variable name.

Resolving the fail in LocaleConfigSubscriberTest by actually invoking the LocaleConfigManager method that would do the right things and not try to work around that by reimplementing parts of it. This is more of a light integration test between LocaleConfigSubscriber and LocaleConfigManager, it has test methods to test changes from both directions.

+++ b/core/modules/locale/locale.services.yml
@@ -1,5 +1,5 @@
-  locale.config.typed:
+  locale.config_manager:
     class: Drupal\locale\LocaleConfigManager
     arguments: ['@config.storage', '@config.storage.installer', '@locale.storage', '@config.factory', '@config.typed', '@language_manager']

Have you considered to use a BC layer using service aliases? http://symfony.com/doc/current/components/dependency_injection/advanced....

@dawehner: Not sure how would that be useful? Most of the public methods of the class changed either in subtle or very significant ways. To make it available under the old name would not help much. In fact renaming the service is not required to fix the bug, so we can keep the old (now incorrect) name, if we don't want to do it here, but I think now would be the time to fix it to be in line with the rest.

I spent a lot of time thinking about the file delete case, which is the remaining fail (indirectly). I came around to the prior behavior that we need to reset locale translations, but at least we should only do it for those strings that were translated before. That is hopefully a one line fix.

So passes existing tests now, but needs more tests for the new features. Although some small tests I updated to assert the new behavior in installer tests, the new behavior needs some more extensive tests. Otherwise should be complete especially for reviewers.

Actually cleaner to name the event responder method something other than save or delete since it responds to both. Let's name it change.

I had an interesting IRC chat with @tstoeckler yesterday and his main concern was the conceptual problem that we mark a config file German yet it has English struff in it possibly forever (until all the pieces are translated either directly in the file or via interface translation). So basically all the steps in this flow that I posted above even at the last step it still has original "English" content in it:

I told @tstoeckler that he is chasing an ideal, and thinking more about it I came up with this very simple counter-example to demonstrate.

1. Install Drupal 8 beta7 in German. Do not use any of the patches on this issue, just plain beta 7. (I just did on simplytest.me)
2. Go create a view, give it a name but do not change any other settings.
3. Save the view.
4. Go to config export and either export this single view or the whole config.
5. Look at what's in your view.

Now that file is langcode: de and yet its chock full of English text (see attached). Some samples:

    display_title: Master
          submit_button: Apply
          reset_button_label: Reset
          exposed_sorts_label: 'Sort by'
          sort_asc_label: Asc
          sort_desc_label: Desc
            items_per_page_label: 'Items per page'
            items_per_page_options_all_label: '- All -'
            offset_label: Offset
            previous: '‹ previous'
            next: 'next ›'
            first: '« first'
            last: 'last »'

What happens here is when the view is created, all plugins get to provide their default settings and those (edit: should) use t(), so the result of those is a half-translated version of the default settings based on interface translation available at the time. On top of that, this is not even shipped config (we don't have the English reference to translate against), so we cannot update these strings later on as the translations backing t() are made available. With core only, users need to translate these later on one by one in the config files via their regular config edit UI or export/import.

So that a German config file may have English text mixed in is coded into how Drupal creates new configuration in the first place. What this issue/patch does it using the same for shipped imported configuration and then letting those be updated at least.

The exported view I forgot to attach

@tstoeckler: note that views default options have a problem with translations, see #2446431: Views default options are not translated when creating a foreign language view but that does not change the situation, all the things you would configure on the view would be German, resulting in mixed languages. (Also looking at the German translation '< previous' is not translated for example and 'Master' is translated as 'Master'). Once the translations are fixed, you would still end up with mixed language config.

An updated view after the fixes in #2446431-6: Views default options are not translated when creating a foreign language view applied. I created this view on a German only site, so I provided the title as "prüfung" in German of course. Then there are things that had the same translation as the source ("Master"), things that lack translation in German right now ("Asc", "Desc", and the pager ones "‹ previous", "next ›", "« first", "last »") and finally things that are proper German ("Elemente pro Seite", "- Alle -", "Sortieren nach", "Zurücksetzen", "Anwenden"). This is a totally realistic example of a foreign language view created. This conceptually the same state as the merged active config translations as shown on the process figure.

So #2446431: Views default options are not translated when creating a foreign language view landed and #2447139: Config entities should be created in the negotiated language unless otherwise specified is RTBC and will further clarify how configuration is created on a multilingual site (currently its a bit confusing). Those should help put this issue into (even) more perspective.

Just a quick update: I am working hard on new tests, but I ran into some interesting integration issues between the listeners, especially the locale config subscriber that I need to investigate is actual problem or just a symptom of the test.

Sooooo finally here comes new test coverage for all the things introduced in the patch:

1. Modified LocaleConfigSubscriberTest to be more extensible (eg.componentized setUp()), so we can subclass it with a test that uses a foreign site default language.
2. Added LocaleConfigSubscriberForeignTest to test a foreign site default language (and adding English in that scenario).

Note that I kept these as kernel tests, which makes them way faster then if they were webtests, but some things are thus simulated. I think it is worth it this way to test a lot of things quickly. There are two changes that may look unrelated:

A. Modified ConfigFactoryOverrideBase to only save the filtered override if there are changes. This avoids events firing and makes saving active config changes speedier. (For me it avoided working around the prior behavior in the test).
B. Modified KernelTestBase to allow for a foreign site default language via a method override.

The new tests pass locally.

I consider this complete. Updated issue summary to be fully accurate. Now really needs reviews.

An interesting side effect of this patch is languages will not be English either. We should not lie to people about that. The language entities will be of course subject to the same system as every other entity for default config translation.

Those were easy to fix.

Minor revisions to the issue summary to make points cleaner. No major changes in the summary, just clarifications for hopefully easier review.

So decided to skip most of the issue summary and the patch and just try it out.

First, this is working well already. Impressive work! I think the system is now way more intuitive.

Here's what I did so far:

1. Install in german.
2. Edit the maintenance message, displayed in german, I can edit it, and the maintenance message is displayed with my changes. YAY!
3. Add french as another language.
4. Add english as another language.
5. Enable config translation.
6. Check the translations for french and english, they contain default translations, visiting the maintenance mode in different languages works fine. YAY!
7. Editing them, works all fine.
8. Exporting configuration, I notice how it saved translations for config entities in language/de/ and simple configuration was directly edited and it has a langcode: de key now (Interesting, how does that work with config schema?)

Now I'm starting to do mean things :)

9. Change site default language to english.
10. Edit maintenance message. Still shows the original german message now in the normal form, but when visiting the config translation page, I can see that it knows that it is german and I can still edit the french and english translation. I think that's good enough, trying to automatically switch out the original and translations would be crazy and scary thing to do.
11. Go to site information, which doesn't have a default translation. On the config translate tab, I can see that it thinks that english is the original language. Interesting...
12. So I go and change the default language to french and voila! Now it tells me that the original is french :)
13. The reason is that we have a conflict in system.site, where langcode is actually the default language.
14. Because I can, I change the default language back to german and I edit the english translation. I notice that there's a default translation for the site name (which is d8 in the original) called "Drupal". Seeing this, I go back to the maintenance page screen and yes, the site name for french/english is in fact Drupal. system.site is one of those strings that isn't really translatable, only localizable. Not sure if that's worth to care about.
15. Switching between languages, I notice that I can get the site offline message to show multiple times in different languages. See screenshot.

The only problem that I found so far that we need to think about is 13, with default language conflict. The others are likely not really related to this issue. Will look at the issue summary/patch asap.

To all the others we reported this issue and want to see it fixed. Test this patch *now*. You can even do it on simplytest.me, just go to http://simplytest.me/project/drupal/8.0.x?patch[]=https://www.drupal.org.... Try to translate and create and edit stuff, try to break it and find flaws.

Accidental status changes, had a (very) old tab open.

Exporting configuration, I notice how it saved translations for config entities in language/de/ and simple configuration was directly edited and it has a langcode: de key now (Interesting, how does that work with config schema?)

That looks incorrect. Config entities should also be in-place translated. Any config that you created in a foreign language (or the API created in non-German) should have an override in languages/de/ but installed shipped config should be in-place translated and not using overrides, regardless of it being a config entity or simple config.

So I looked into that problem. It indeed seems like some things are translated still as overrides. Spot-checked some of them on a fresh site installed in Hungarian:

- language.entity.hu was added in EN and translated with an override
- core.entity_view_mode.user.full.yml has langcode UND (why?!) but is still translated
- tour.tour.locale has langcode en and translated with an override

I think there are probably different reasons for these, looking into them. There were 24 language overrides for a total of 186 config items.

Setting needs work for #108, but people should not hold off on reviews, this issue has way too many facets to it :)

Found the reason for the und config entities: #2451885: Config entities need to ship with language or are assumed undefined (existing problem, not introduced by this patch). Did not yet find the reason for the 'en' entities yet. Curiously exactly three entity types are affected: views, search pages and tours. None of the other entity types are affected. But all instances of those entity types appear as overrides, while others appear as inline translated. Anybody knows what may be common?

Found the reason for the remaining ones. Those get weeded out at (!$entity->isNew()), so each time we attempt to look at them, they appear as new. That seems to be down to caching and multiple modules installed in one request. If the locale module is installed and then a few other modules are installed, then locale will attempt to read all the install storage stuff at once and make "new" cached things appear for those that are not yet installed. Then when those are actually installed in the same request, they are still new, so they are not modified. By just resetting before loading, that solves that problem and then we only have #2451885: Config entities need to ship with language or are assumed undefined left.

I also found more instances of lack of langcode (#2451885: Config entities need to ship with language or are assumed undefined), where Hungarian translation would not have been available anyway.

Duh, that does not seem to be fixing the issue on a new site, I don't know how my local environment seemed fixed, it had other changes definitely. Will need to explore that more. It certainly is that $config->isNew() says true.

A more complete assessment of the problem is that when installing components, we may install configuration from other (previously installed) components too. When installing Views or Tour module, we install views and tours from modules installed before. If we only look at the configuration names for the components installed at that moment, we don't get those other components when they are installed. And before that, when we looked at the modules that ship them (eg. node), the views and tours were not yet imported, so we did not update them then either.

There are two possible solutions that I see:

1. Always look at all default config and update all active copies of those that are not yet modified. I did this here.
2. Keep the component installation code for when locale module itself is installed and update all prior config there. Add an event to config import and listen on that for all future config imports.

Let's discuss if my fix is good or we should go with 2 or some other solution.

Note that when installing in a foreign language with this patch, there are still files that get a language override, but ALL those are due to #2451885: Config entities need to ship with language or are assumed undefined and not an error in this patch.

So the new fails are due to langcode being applied to all imported configuration, while it is not defined at several places. I started going manually and adding langcode definitions to the ones that were failing, but that quickly gets out of hand. So why do we need to add that langcode to files? Why can't we tell if the file may contain translatable things or may not. The matter of fact is, we can examine the current state of the file with the schema corresponding to that but that does not help us decide if that file MAY contain text in a human language later on. Config schema is dynamic, it may depend on the data, so we cannot tell if there is going to be data (such as using different plugin or a new third party setting) that would make the file use human language info. So we need to force on the langcode on config files.

The question then is how are we going to cover that in the schema.

1. Should we expect people to manually add langcode to each of their schemas? Sounds like it works but will need a LOOOT of diffs here.

2. Should we add in langcode as a default mapping key to config definitions in TypedConfigManager? Sounds like some API changes required so we can track where we look up the high level element or a deeper element, so we don't add langcode there.

3. Should we introduce a high level config type that extends from mapping. Named something like 'config_root' that would contain a langcode and all config file roots would use it (eg. config_entity would extend from that too).

I can go on implementing either but it feels like those are pretty different so I would not go into code-mode unless we have some direction decided.

Any preferences?

Implementing #115/2 for now after discussion with @berdir. Now this means that the existing config schemas are full of top level langcode keys that are not needed there anymore, but we can remove them later as a cleanup. (Or if we end up going with #115/3). They way I implemented #115/2 is I added a new method getDefinitionForConfigName($name) which is a special case of getDefinition($plugin_id) on the TypedConfigManager and only used for the top level when looking up definition for the config name itself. The lower levels keep recursing into getDefinition($plugin_id).

Added the langcode schema key as an API change, although its mostly just an addition.

#2451885: Config entities need to ship with language or are assumed undefined landed, so this should be completely working as it should :)

Blocked on reviews now.

are the images in the issue summary up to date?

@YesCT: yes, all of them are up to date AFAIS.

I don't have nearly enough experience with multilingual issues to RTBC this, but I looked over the patch and nothing jumped out at me as problematic. This code is pretty complex, but I don't think the patch makes it any more so than is already the case in HEAD.

1. +++ b/core/lib/Drupal/Core/Config/ConfigFactoryOverrideBase.php
   @@ -87,27 +87,32 @@ protected function filterOverride(Config $config, StorableConfigBase $override)
       *   Override data to filter.
       */
      protected function filterNestedArray(array $original_data, array &$override_data) {
   ...
   +    return $changed;
   

   Need to document @return param

2. +++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
   @@ -148,6 +148,22 @@ public function getDefinition($base_plugin_id, $exception_on_invalid = TRUE) {
    
      /**
   +   * @inheritdoc
   +   */
   +  public function getDefinitionForConfigName($name) {
   +    $definition = $this->getDefinition($name);
   +    // If this definition is defined as a mapping, add a langcode element if
   +    // not already present.
   +    if (isset($definition['mapping']) && !isset($definition['mapping']['langcode'])) {
   +      $definition['mapping']['langcode'] = array(
   +        'type' => 'string',
   +        'label' => 'Language code',
   +      );
   +    }
   +    return $definition;
   +  }
   
   +++ b/core/lib/Drupal/Core/Config/TypedConfigManagerInterface.php
   @@ -102,4 +102,19 @@ public function buildDataDefinition(array $definition, $value, $name = NULL, $pa
   +  /**
   +   * Gets a specific plugin definition for a configuration name.
   +   *
   +   * @param string $name
   +   *   The configuration name.
   +   *
   +   * @return mixed
   +   *   A plugin definition, or NULL if the plugin ID is invalid and
   +   *   $exception_on_invalid is FALSE.
   +   *
   +   * @throws \Drupal\Component\Plugin\Exception\PluginNotFoundException
   +   *   Thrown if $plugin_id is invalid and $exception_on_invalid is TRUE.
   +   */
   +  public function getDefinitionForConfigName($name);
   

   I know that the plugin ID is language borrowed from typed data but does it make sense here? Also when should some use TypedConfigManager::getDefinition() and when should they use TypedConfigManager::getDefinitionForConfigName()? Feels unclear from the code docs.

3. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
   @@ -60,6 +60,29 @@ public function __construct($string, array $arguments = array(), array $options
      /**
   +   * Get the string value stored in this translation wrapper.
   +   *
   +   * @return string
   +   *   The string stored in this wrapper.
   +   */
   +  public function getString() {
   +    return $this->string;
   +  }
   ...
      public function __toString() {
   

   getString and __toString() are returning different things - is getString() really getUntranslatedString() or getSourceString()?

4. +++ b/core/modules/locale/locale.module
   @@ -356,6 +361,34 @@ function locale_cron() {
    /**
   + * Update default configuration when new modules or themes are installed.
   + */
   +function locale_system_set_config_langcodes() {
   

   New method - shall we move it to the static Locale class?

5. +++ b/core/modules/locale/locale.module
   @@ -689,13 +720,15 @@ function locale_form_language_admin_edit_form_alter_submit($form, FormStateInter
   + *   The language code.
     * @return bool
   

   Missing empty comment line between @param and @return

6. +++ b/core/modules/locale/locale.module
   @@ -689,13 +720,15 @@ function locale_form_language_admin_edit_form_alter_submit($form, FormStateInter
   +function locale_is_translatable($langcode) {
   +  return $langcode != 'en' || \Drupal::config('locale.settings')->get('translate_english');
    }
   

   Considering the static Locale class exists - shall we move this new method there?

7. +++ b/core/modules/locale/locale.services.yml
   @@ -1,5 +1,5 @@
   -  locale.config.typed:
   +  locale.config_manager:
   

   +1 this has bothered me before :)

8. +++ b/core/modules/locale/src/LocaleConfigManager.php
   @@ -162,10 +242,24 @@ protected function compareConfigData(array $default, $updated) {
   +  public function saveTranslationOverride($name, $langcode, array $data) {
   ...
   +  public function saveTranslationActive($name, array $data) {
   

   Do these need to be public? Isn't it the job of the LocaleConfigManager to decide which to do? I think we should review what we want users of the locale config manager to be able to do - ie what should be public and what should be protected.

9. +++ b/core/modules/locale/src/Tests/LocaleConfigSubscriberTest.php
   @@ -253,13 +277,27 @@ protected function saveLanguageOverride($config_name, $key, $value) {
       *   The configuration key.
       * @param string $value
       *   The configuration value to save.
   +   * @param string $langcode
   +   *   The language code.
   +   * @param bool $is_active
   +   *   Whether the update will affect the active configuration.
   ...
   -  protected function saveLocaleTranslationData($config_name, $key, $value) {
   +  protected function saveLocaleTranslationData($config_name, $key, $source, $translation, $langcode, $is_active = FALSE) {
   

   Need to fix up @param's

10. It's hard to get an idea if we've got all of the code change covered with tests. Also do we have a test case proving we've fixed the original reported bug?

@alexpott: updates based on your comments. TLDR: 4&6 need your feedback and 10 needs some more work on my part. In detail:

1. Done.

2. This is a bit tricky. We inherit the getDefinition() method from as deep as DiscoveryInterface. We don't have an understanding of an "invalid plugin ID", so the $exception_on_invalid param does not even make sense for us (just kept for compatibility). We don't throw exceptions ever. If an invalid name is found, we just fall back on definition for undefined, so config schema can be optional. Added more specific documentation to TypedConfigManagerInterface. Hope this helps.

3. Renamed to getUntranslatedString() you are right.

4. So Locale is documented as a Static service container wrapper for locale.. The only method we have on it is config(). While it makes total sense in Drupal 8 in general to move global functions to such class, just putting this one (or two) new methods on it will make the internal consistency of locale worse. It will eventually get better but I am not sure we'll move more global functions for the sake of making locale more consistent with locale later, so it will be halfway until D9 at least, no? So why not let it have some internal consistency at least and leave this a global function?

5. Done.

6. Same argument as (4).

7. :)

8. You are right, made them protected. deleteLanguageTranslations() cannot be made protected because that is done in response to a hook, so we need to invoke it from the outside. Or if we'd make protected, we'd need to add yet one more public method, so no win there.

9. Fixed.

10. No test cases for editing a foreign maintenance mode setting yet. Will add that later today.

Updating test coverage as promised in #123/10. While the original report was about the maintenance message, the account settings are just as well a good subject to test (come with the system the same way). So I updated the installer foreign language / multilingual tests with lots more asserts in terms of where translations end up.

I also found out that we did not test for negative English in the multilingual test and modified the testing_multiligual profile to keep English at all times, which is not good. We want to test a profile that does not keep English and one that does. So now I separated that out.

I think this is enough to cover the problem for what the issue was opened with. We cannot really test all config files. I wanted to originally add more assertions for maintenance but then figured out it would not add much that the user settings are not testing already.

Ps. ran into strange local issues while attempting to run the test, so uploading for testbot to check.

Even more test coverage to assert that when English is present and not the site default language, its override is present too.

Asserting negative on the English override when that is expected to round that out. Adding English on a foreign site is already tested in LocaleConfigSubscriberForeignTest::testEnglish(), so no need to elaborate on the test here more.

I think that completes covering testing in the installer too.

Updating the single language foreign language installer test too with config asserts, so that nobody can say we are only testing a multilingual installer. We are testing a foreign language monolingual installer too.

I think we have very good kernel test coverage on the transition interactions (editing overrides updating locale, editing locale saving override, etc). I think we prefer kernel testing those, but its true that those do not 100% ensure that the UI is all wired up. Do we want to cover with webtests too?

After fixing the unrelated fails in my local environment I could finally run the tests and was able to reproduce. Fixes:

1. When adding English and locale import is disabled, the default config stuff was not parsed either (because that was within that conditional). We should parse it. That fixes the single language test proper. Fix in locale.module. Added one more assert to that test just to round it out, that was not required for the fix of course.

2. The test logic itself I added to InstallerTranslationMultipleLanguageTest was incorrect. It was attempting to test adding English even for the testing_multilingual_with_english case, where English was already there. This required me to reproduce similar conditions at the end of the test to what was at the middle, so I decided to merge the parts instead. This makes it look like I changed more, but just moved some things around. Also added lots more comments to explain every bit of what we test. There are many combinations :)

Looks good to me!

Config Export BEFORE

Config Export AFTER

Finally had a look at the patch, looks pretty good to me, I didn't review every line of code, instead I tried to focus a bit on nitpicks/documentation.

Tests look good to me.

Not sure about RTBC though, I don't think we addressed the problem with existing top-level langcode that has a different meaning (like system.site), we need at least a follow-up for that.

1. +++ b/core/modules/locale/locale.module
   @@ -356,6 +361,34 @@ function locale_cron() {
    /**
   + * Update default configuration when new modules or themes are installed.
   + */
   +function locale_system_set_config_langcodes() {
   

   Nitpick: should be UpdateS I think?

   Wondering we don't put this on an object/service, like we did other existing methods?

2. +++ b/core/modules/locale/locale.module
   @@ -645,21 +676,26 @@ function locale_form_language_admin_add_form_alter(&$form, FormStateInterface $f
   +  // Create or update all configuration translations for this language. Need to
   +  // run this even if import is not enabled, if we are adding English, because
   +  // then we extract English sources from shipped configuration.
   +  if (\Drupal::config('locale.settings')->get('translation.import_enabled') || $langcode == 'en') {
   

   Second sentence would be easier to understand if it would start with: If we are adding english then we need to run...

3. +++ b/core/modules/locale/src/LocaleConfigManager.php
   @@ -7,33 +7,53 @@
   + * corresponding storage of the translation (in active storage or overrides).
   

   Overides are in the active storage too, just a special collection there. Maybe "in the original config object or an override", then possibly without ().

4. +++ b/core/modules/locale/src/LocaleConfigManager.php
   @@ -99,61 +121,119 @@ public function __construct(StorageInterface $config_storage, StorageInterface $
   +   * @param array $translatable
   +   *   The translatable array structure. A nested array matching the exact
   +   *   structure under of the default configuration for $name with only the
   +   *   elements that are translatable wrapped into a TranslationWrapper.
   +   *   @see self::getTranslatableData().
   

   Wondering if we can document this as array|TranslationWrapper[], then you don't need the inline @var.

5. +++ b/core/modules/locale/src/LocaleConfigManager.php
   @@ -99,61 +121,119 @@ public function __construct(StorageInterface $config_storage, StorageInterface $
   +   * @param string $langcode
   +   *   The language code to process the array with
   

   Missing .

6. +++ b/core/modules/locale/src/LocaleConfigManager.php
   @@ -330,29 +403,234 @@ public function translateString($name, $langcode, $source, $context) {
   +   * @param $name
   +   *   The configuration name.
   ...
   +   * @param $name
   +   *   The configuration name.
   ...
   +   * @param $name
   +   *   The configuration name.
   

   Missing string ;)

7. +++ b/core/modules/locale/src/LocaleConfigSubscriber.php
   @@ -68,202 +67,165 @@ public function __construct(StringStorageInterface $string_storage, ConfigFactor
   +    if ($this->localeConfigManager->isSupported($name) && locale_is_translatable($langcode)) {
   

   Also wondering if we can move locale_is_translatable() somewhere, e.g. the string translation service. Not a big issue.

8. +++ b/core/modules/locale/src/LocaleConfigSubscriber.php
   @@ -68,202 +67,165 @@ public function __construct(StringStorageInterface $string_storage, ConfigFactor
   +   * @param array $translatable
   +   *   The translatable array structure, see this::getTranslatableData().
   

   same here (documenting TranslationWrapper)

9. +++ b/core/modules/locale/src/LocaleConfigSubscriber.php
   @@ -68,202 +67,165 @@ public function __construct(StringStorageInterface $string_storage, ConfigFactor
   +   * @param $langcode
   +   *   The language code of the translation being processed.
   

   missing string.

10. +++ b/core/modules/locale/src/Tests/LocaleConfigSubscriberForeignTest.php
    @@ -0,0 +1,170 @@
    +  /**
    +   * @inheritdoc
    +   */
    +  public function testDeleteTranslation() {
    

    I don't think that inheritdoc is correct here :)

11. +++ b/core/modules/system/src/Tests/Installer/InstallerTranslationMultipleLanguageForeignTest.php
    @@ -26,7 +26,7 @@ class InstallerTranslationMultipleLanguageForeignTest extends InstallerTranslati
         parent::setUpLanguage();
    -    $this->translations['Save and continue'] = 'Save and continue German';
    +    $this->translations['Save and continue'] = 'Save and continue de';
    

    is there a reason for this change? :)

For #131

Addressed @Berdir points 1, 2, 3, 4, 5, 6, 8, 9, 10. Points 7 and 11 are pending. Not sure about 7 and 11 seems fine to me.

Sorry, messed the comment.

Argh! Rebased now! Previous interdiff if ok.

Re @berdir: 1 and 7: Wondering we don't put this on an object/service, like we did other existing methods? @alexpott asked the same above. Locale only has one global object, Locale. That does not have an equivalent when locale module is not enabled. So we have LanguageManager which does not have locale related stuff so far and TranslationManager which could house the method I guess. However, if you look at locale module, it does not do this other then what's in TranslationManager. If we want to make these changes gradually (ie. move these two functions because we happen to change them and leave everything else as global functions), then Drupal 8 will be more inconsistent than how it is now. At least locale is fairly consistent about using global functions. I don't think that's good, but we also want to release Drupal 8 and not refactor until perfection. So while moving these two functions to a class may be fine, there is no prior practice in locale module so we would introduce some inconsistency within locale module. If we think its better to introduce the inconsistency within locale module so that at least some minimal parts of the module are more consistent with the rest of core, then we can do this. Just wanted to highlight the consequences.

Re @berdir 11: the reason is I added a helper method to generate the .po file for the installer in those tests, and it was easier to not need to pass both the real name of the language and the langcode, so the strings it generates changed slightly. We can also pass the real name to the new method but then all the newly added asserts will be longer too :)

@berdir responded in IRC:

[11:38am] berdir: GaborHojtsy: fine with me, just noticed that, I know that locale.module still has lots and lots of functions, in optional .inc files and so on ;)
[11:38am] GaborHojtsy: berdir: yeah its ugly, but at least its consistenly D7 style :D

@berdir: opened #2457653: System.site langcode is both used as a file language code and a site language code.

1. +++ b/core/modules/locale/src/LocaleConfigManager.php
   @@ -190,13 +191,13 @@ protected function getTranslatableData(TypedDataInterface $element) {
   -   * @param array $translatable
   +   * @param array|\Drupal\Core\StringTranslation\TranslationWrapper $translatable
   

   TranslationWrapper[], it is a list of them (or a list of translation wrapper lists)

2. +++ b/core/modules/locale/src/LocaleConfigSubscriber.php
   @@ -133,7 +133,7 @@ protected function updateLocaleStorage(StorableConfigBase $config, $langcode, ar
       *   The active configuration data or override data.
   -   * @param array $translatable
   +   * @param array|\Drupal\Core\StringTranslation\TranslationWrapper $translatable
       *   The translatable array structure, see this::getTranslatableData().
   

   Same here.

Also opened #2457703: Default translatable site name is "Drupal" (incorrectly) for @berdir's other concern.

@Berdir re 141:

1. It is either a single TranslationWrapper or an array of TranslationWrappers or an array of arrays of ... Not sure how to reflect that better than it is now? getTranslatableData () may return a single translation wrapper. The current doc seems to be correct stating its either an array or a TranslationWrapper. If we need to further qualify the array, then we need to somehow qualify that its either an array of arrays or an array of TranslationWrappers. Do you think array[]|array|\Drupal\Core\StringTranslation\TranslationWrapper would be more correct? Or array[]|\Drupal\Core\StringTranslation\TranslationWrapper[]|\Drupal\Core\StringTranslation\TranslationWrapper Or?

2. It is true that processTranslatableData() takes an array of translation wrappers or an array of array of ... Also not sure how to reflect this better. Before @rteijeiro started addressing your concerns it was simply array which is what it is. Is it better to elaborate on it as array[]|\Drupal\Core\StringTranslation\TranslationWrapper[] (as in array of arrays or array of TranslationWrappers). Not sure that's better? I see the single TranslationWrapper mention is not correct now.

@berdir explained me in IRC. He is right. I misunderstood what he meant.

+++ b/core/modules/locale/src/LocaleConfigSubscriber.php
@@ -133,9 +133,9 @@ protected function updateLocaleStorage(StorableConfigBase $config, $langcode, ar
+   *   @see \Drupal\locale\LocaleConfigManager::getTranslatableData().

Nitpick for next reroll, no . on @see lines

(Migrate file test unable to copy file fail is unrelated. Sent for retest.)

Resolve @tim.plunkett's review in #146.

Also noticed that we still have the "Built-in English" special language for configuration which does not make sense anymore. We don't keep the shipped config in English, so we the "Built-in English" would not appear anymore. All the code comments surrounding supporting it were all incorrect with this patch obviously.

The assert that is failing does not make sense in the world order set up by this patch. Once removing English, English will not be displayed as a valid option when editing a menu, so the language flips over to the first available option (in that case 'aa' was the first language created). If you remove a language that will not be available as an option to assign, that is not something we should specifically test, so removing this test method.

Any other concerns?

Updated API changes including the getLanguageWithFallback removal.

Added the following 5 change notice drafts. We may want to add 2 more (for the locale_is_translatable() name change and the KernelTestBase addition) if people feel those would be helpful:

For site builders:

Default configuration is now translated in-place

For developers:

• Removed LocaleTypedConfig
• Made possible to get the source data out of TranslationWrappers
• The locale.config.typed service is renamed to locale.config_manager, most public methods changed
• The langcode top level key is automatically added to configuration schemas

My favorite quote: LocaleTypedConfig (which does not equal the LocaleConfigManager which was previously exposed under the confusing locale.config.typed service name, but is now renamed locale.config_manager, see https://www.drupal.org/node/2458583) is removed.

Added Made it possible to use KernelTestBase to test foreign language environments and locale_translate_english() is now locale_is_translatable($langcode) change notices so I believe all changes/additions are now covered.

Back to RTBC!

Rerolled. Conflict was on MenuLanguageTest.php due to #2457653: System.site langcode is both used as a file language code and a site language code.

Retroactively tagging for tracking.

Opened #2459377: Upgrade path for #2212069 (config translations in active config) in head2head too yesterday.