Problem/Motivation

When installing a Drupal 8 site that has no English language configured, the default configuration shipped is still in English. The user interfaces to edit shipped configuration (eg. views, maintenance mode setting, node types, fields, etc) are all editing the original configuration (in English). The user interface for editing will be in the foreign language installed however. When adding new elements, eg. new fields to a view, the user is expected to add them in English and translate them in configuration translation (so enabling the configuration translation module is required to keep it sane). That is silly.

Why is this so?

We keep shipped config as-is as a reference point for configuration translation, so we can apply configuration translation against the shipped configuration text. We break down default configuration to its translatable strings and each string that was not modified in active config (and shows up in the same config file) gets its translation applied. This is good for various reasons. (A) When you install, you don't need 100% of the translations, you can keep adding them later (eg. we can import translations after configuration import and don't bother if translations cannot be imported at one point, it can be fixed later). (B) When you add more languages, they still have the same original reference point, so their translations will work too.

Comparing this to Drupal 7, there default configuration was shipped in core and t()-ed in modulename.install when saved into variables, etc. Only translations available at that time were applied, so when you updated your interface translation later on, your fields, content types, etc. were still half-translated and there was no way to fix them. They were "user provided configuration" so the locale module did not concern those. If you changed the site default language, the configuration was still in the same possibly half-baked form and did not adapt to your language change. All of those are fixed with this "original reference configuration" approach.

The use cases and the current architecture

The reason we have the original config in English is to keep a relatively stable copy to reference.

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 (given 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 and adding to it is silly in English. It is 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.

Proposed resolution

We satisfy all of the use cases above 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 current proposal satisfies all those use cases.

The most UX friendly proposal is to have active configuration saved in the default foreign site language of the site instead of imported in English. That means, we apply translations at first when importing config. What comes out of that?

  1. Satisfying use case B needs more work if you add English later: If the site default language is English, 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 after importing. If English is added later, we need to go through the original shipped configs and extract their English sources and save those as the English translation (for config strings which are still present). This is 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. Because we can only compare key names then, changes to lists of items (eg. allowed values) may result in confusing results. Eg. if you change allowed values to "yes/no" from "on/off", the translations will still consider "yes/no" because allowed values are stored as lists.
  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 needs to be aware of both active config and language overrides. Prior to this patch 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 (AKA modified).
  5. Finally how we compare shipped config with active config would be a lot more fragile thanks to skipping string comparisons on translatable strings. There is an existing problem that shipped config may change in a module without the active config changing and we should compare active config to the shipped config of the time when the active config was originally imported. Given we skip comparing the translatable strings making the comparison more fragile, we need to make the system less fragile on the other end. We should keep a copy of the shipped config at the time of installation to compare for localization. This is in a separate issue, see #2428045: Comparing active storage to install storage is problematic, install storage may change anytime.

Use case E is already satisfied with the above modifications.

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).

A sample process with installing a file and updating translations as well as editing the file manually, editing translations manually and updating translations again:

Remaining tasks

Reviews.

User interface changes

1. You can now edit foreign language configuration as the active configuration on a non-English default site.
2. Language names are not provided in English anymore either, but in the language of the site at the time of adding the language.

API changes

Let's look at the current API first. The locale - config integration is done with two main drivers. The LocaleConfigSubscriber listens on configuration override changes and when configuration overrides are saved or deleted, it reacts by updating locale database data. The LocaleConfigManager is invoked after database changes in locale (when importing translations, editing translations manually, etc) and it reacts on those to update configuration overrides:

The basic structure of this setup is kept and responsibilities are cleaned up in the proposed patch.

1. When active configuration is edited, we also respond to that in LocaleConfigSubscriber.

2. When a new module is installed, we also react to that by updating the (just imported) configuration with the site default langcode (so LocaleConfigManager can later update it with translations as appropriate). When installing locale module itself, we update all prior active configuration that was imported from default configuration to the site default langcode.

3. The locale_config_update_multiple() function is now LocaleConfigManager::updateConfigTranslations().

4. We do not respond to modules uninstalled anymore. LanguageConfigFactoryOverride::onConfigDelete() already responds and removes config overrides when the config is removed. Locale's database is additive so we should not remove items when removing components.

Detailed API changes:

Before After Reason
--- TranslationWrapper::getUntranslatedString() TW is used as wrapper of translatables
--- TranslationWrapper::getOption($name) TW is used as wrapper of translatables
locale.config.typed (service) locale.config_manager There was not much typed about it before, and there is nothing typed about it now. Unified naming with other services in and outside of locale.
LocaleConfigManager::hasTranslation($language) LocaleConfigManager::hasTranslation($langcode) We took langcode in every other method.
locale_config_update_multiple($names, $langcodes) LocaleConfigManager::updateConfigTranslations($names, $langcodes) Did things that are the primary responsibility of the locale config manager.
--- locale_system_set_config_langcodes($components) Responds on component install, invoked from hooks.
LocaleConfigManager::deleteComponentTranslations() --- Locale is additive, should not respond on component removal.
locale_translate_english() locale_is_translatable($langcode) More flexible. Such a function with a langcode param was needed to avoid copy-pasting logic around several times.
LocaleConfigManager::get($name) LocaleConfigManager::getTranslatableDefaultConfig($name) Does not return a possibly half-broken typed config anymore. Instead its a nested array of TranslationWrappers.
public LocaleConfigManager::saveTranslationData($name, $langcode, $data) protected LocaleConfigManager::saveTranslationOverride($name, $langcode, $data) Now that both overrides and active config managed here, required a rename to be more specific.
--- protected LocaleConfigManager::saveTranslationActive($name, $data) New method to save translation to active config.
public LocaleConfigManager::deleteTranslationData($name, $langcode) protected LocaleConfigManager::deleteTranslationOverride($name, $langcode) Now that both overrides and active config managed here, required a rename to be more specific.
LocaleConfigManager::getComponentNames(array $components) LocaleConfigManager::getComponentNames(array $components = array()) Was documented to be optional, but the code did not do justice to the docs.
--- LocaleConfigManager::reset() To reset the internal cache used by translateString(). Needed now that we use it for more things.
--- LocaleConfigManager::getStringTranslation($name, $langcode, $source, $context) Takes on responsibility that LocaleConfigSubscriber had duplicate code for (which is now removed).
--- LocaleConfigManager::getDefaultConfigLangcode($name) Utility method to look up default language code of shipped config.
--- LocaleConfigManager::getActiveConfigLangcode($name) Utility method to look up active language code of current config.
--- LocaleConfigManager::isSupported($name) Utility method to verify if this configuration is supported in the locale integration at all. Used both here and in LocaleConfigSubscriber to avoid duplicating logic.
LocaleConfigManager::isUpdatingConfigTranslation() LocaleConfigManager::isUpdatingConfigTranslationFromLocale() Renamed to make the responsibility clear. The prior code indicated this may not have been understood well.
LocaleConfigSubscriber::__construct(StringStorageInterface $string_storage, ConfigFactoryInterface $config_factory, LocaleConfigManager $locale_config_manager) LocaleConfigSubscriber::__construct(ConfigFactoryInterface $config_factory, LocaleConfigManager $locale_config_manager) Does not work with locale string storage anymore directly. Uses LocaleConfigManager::getStringTranslation() instead, which works with the cache there, so lots of duplicate code was removed from here.
LocaleConfigSubscriber::onSave(LanguageConfigOverrideCrudEvent $event) LocaleConfigSubscriber::onOverrideChange(LanguageConfigOverrideCrudEvent $event) Renamed due to the added responsibility of responding to active config events too (named Change because it reacts to save and delete too).
LocaleConfigSubscriber::onDelete(LanguageConfigOverrideCrudEvent $event) LocaleConfigSubscriber::onOverrideChange(LanguageConfigOverrideCrudEvent $event) Yes, same as for save.
--- LocaleConfigSubscriber::onConfigSave(ConfigCrudEvent $event) Responds to active config saves.
LocaleTypedConfig (the whole class) --- Removed entirely. It was only used with possibly broken input data and its code was duplicated elsewhere. A much simplified approach is used directly in LocaleConfigManager.
--- KernelTestBase::defaultLanguageData() Allows kernel tests to simulate a foreign language default environment.
--- langcode key on top level config in config schemas now added by default, no need to define it in schema Required now that we may modify any shipped config with an added langcode.
ConfigMapperInterface::getLanguageWithFallback() --- Removed because English is not special anymore, so this method would not provide any fallback anymore. Use LanguageManager::getLanguage()
CommentFileSizeAuthor
#161 interdiff.txt6.89 KBGábor Hojtsy
#161 2212069-161.patch129.36 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,567 pass(es). View
#157 2212069-157.patch129.33 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,621 pass(es). View
#151 interdiff.txt1.57 KBGábor Hojtsy
#151 2212069-151.patch129.32 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2212069-151.patch. Unable to apply patch. See the log in the details link for more information. View
#149 interdiff.txt6.92 KBGábor Hojtsy
#149 2212069-149.patch127.75 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,707 pass(es), 1 fail(s), and 0 exception(s). View
#143 2212069-143.patch121.9 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,726 pass(es). View
#143 interdiff.txt1.77 KBGábor Hojtsy
#142 interdiff.txt821 bytesGábor Hojtsy
#142 2212069-142.patch121.88 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#135 2212069-135.patch122.03 KBrteijeiro
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,691 pass(es). View
#134 interdiff.txt971 bytesrteijeiro
#134 2212069-134.patch147.71 KBrteijeiro
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#133 interdiff.txt6.81 KBrteijeiro
#133 2212069-132.patch122.05 KBrteijeiro
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#130 config-before.png177.06 KBrteijeiro
#130 config-after.png178.41 KBrteijeiro
#129 interdiff.txt7.05 KBGábor Hojtsy
#129 2212069-129.patch121.95 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,711 pass(es). View
#127 interdiff.txt5.47 KBGábor Hojtsy
#127 2212069-127.patch119.69 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,313 pass(es), 4 fail(s), and 0 exception(s). View
#126 interdiff.txt1.91 KBGábor Hojtsy
#126 2212069-126.patch114.75 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,285 pass(es). View
#125 interdiff.txt1.08 KBGábor Hojtsy
#125 2212069-125.patch114.59 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,271 pass(es). View
#124 2212069-124.patch114.22 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,272 pass(es). View
#124 interdiff.txt10.6 KBGábor Hojtsy
#123 interdiff.txt9.01 KBGábor Hojtsy
#123 2212069-123.patch104.97 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,801 pass(es). View
#116 interdiff.txt4.68 KBGábor Hojtsy
#116 2212069-116.patch103.64 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,459 pass(es). View
#113 interdiff.txt2.46 KBGábor Hojtsy
#113 2212069-113.patch99.82 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,046 pass(es), 3 fail(s), and 1 exception(s). View
#111 interdiff.txt711 bytesGábor Hojtsy
#111 2212069-111.patch99.97 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,104 pass(es). View
#105 multiple_languages.png15.69 KBBerdir
#103 interdiff.txt2.38 KBGábor Hojtsy
#103 2212069-103.patch99.96 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,523 pass(es). View
#101 interdiff.txt1.34 KBGábor Hojtsy
#101 2212069-101.patch97.58 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,521 pass(es), 3 fail(s), and 0 exception(s). View
#99 2212069-99.patch96.24 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,454 pass(es). View
#99 interdiff.txt27.41 KBGábor Hojtsy
#96 views.view_.pruefung.yml.txt3.64 KBGábor Hojtsy
#94 views.view_.test.yml.txt3.61 KBGábor Hojtsy
#92 interdiff.txt1.55 KBGábor Hojtsy
#92 2212069-92.patch75.39 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,647 pass(es). View
#90 interdiff.txt652 bytesGábor Hojtsy
#90 2212069-90.patch75.38 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,641 pass(es). View
#86 interdiff.txt5.46 KBGábor Hojtsy
#86 2212069-86.patch75.3 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,591 pass(es), 1 fail(s), and 0 exception(s). View
#84 interdiff.txt865 bytesGábor Hojtsy
#84 2212069-84.patch73.59 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,569 pass(es), 2 fail(s), and 0 exception(s). View
#82 interdiff.txt5.53 KBGábor Hojtsy
#82 2212069-82.patch73.57 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,485 pass(es), 13 fail(s), and 11 exception(s). View
#79 interdiff.txt4.09 KBGábor Hojtsy
#79 2212069-79.patch71.09 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,562 pass(es), 3 fail(s), and 0 exception(s). View
#77 Locale and config interaction sample.png83.03 KBGábor Hojtsy
#75 interdiff.txt1.4 KBGábor Hojtsy
#75 2212069-74.patch69.37 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,514 pass(es), 3 fail(s), and 0 exception(s). View
#72 2212069-72.patch69.15 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,511 pass(es), 3 fail(s), and 0 exception(s). View
#72 interdiff.txt1.68 KBGábor Hojtsy
#71 interdiff.txt1.33 KBGábor Hojtsy
#71 2212069-71.patch69.46 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,516 pass(es), 3 fail(s), and 0 exception(s). View
#70 Locale vs. configuration in Drupal 8 -- New.png138.77 KBGábor Hojtsy
#67 interdiff.txt14.75 KBGábor Hojtsy
#67 2212069-67.patch68.97 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,519 pass(es), 3 fail(s), and 0 exception(s). View
#62 interdiff.txt4.91 KBGábor Hojtsy
#62 2212069-62.patch66.75 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,287 pass(es), 3 fail(s), and 0 exception(s). View
#60 interdiff.txt620 bytesGábor Hojtsy
#60 2212069-60.patch64.01 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,268 pass(es), 10 fail(s), and 2 exception(s). View
#58 2212069-58.patch63.41 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,039 pass(es), 11 fail(s), and 3 exception(s). View
#58 interdiff.txt2.98 KBGábor Hojtsy
#54 interdiff.txt8.39 KBGábor Hojtsy
#54 2212069-53.patch62.06 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,080 pass(es), 11 fail(s), and 2 exception(s). View
#52 interdiff.txt1.54 KBGábor Hojtsy
#52 2212069-52.patch56.75 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,029 pass(es), 12 fail(s), and 3 exception(s). View
#50 interdiff.txt4.82 KBGábor Hojtsy
#50 2212069-50.patch56.05 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,016 pass(es), 15 fail(s), and 3 exception(s). View
#48 2212069-48.patch54.95 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,989 pass(es), 46 fail(s), and 7 exception(s). View
#47 Locale vs. configuration in Drupal 8 with active config.png83.51 KBGábor Hojtsy
#47 Locale vs. configuration in Drupal 8 (1).png67.22 KBGábor Hojtsy
#38 Drupal 8 config translation components (alternate).png29.71 KBGábor Hojtsy
#37 Drupal 8 config translation components.png70.22 KBGábor Hojtsy
#20 maintenance_mode-2212069-20.patch3.25 KBpjonckiere
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,730 pass(es). View
#20 maintenance_mode-2212069-20.test-fail.patch2.22 KBpjonckiere
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,797 pass(es), 1 fail(s), and 0 exception(s). View
#15 maintenance_mode-2212069-15.patch3.28 KBpjonckiere
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,719 pass(es). View
#15 maintenance_mode-2212069-15.test-fail.patch2.25 KBpjonckiere
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,277 pass(es), 1 fail(s), and 0 exception(s). View
#9 maintenance_mode-2212069-9.patch3.75 KBpjonckiere
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,252 pass(es), 3 fail(s), and 0 exception(s). View
#9 maintenance_mode-2212069-9.test-fail.patch2.71 KBpjonckiere
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,239 pass(es), 3 fail(s), and 0 exception(s). View
#8 maintenance_mode-2212069-7-test-only.patch1.93 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,247 pass(es). View
#3 maintenance_mode-2212069-3.patch1.03 KBpjonckiere
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,218 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

pjonckiere’s picture

I cannot reproduce this issue. I guess this is no longer relevant?

cilefen’s picture

Title: Drupal ignores maintenance mode status text » Maintenance mode message is ignored on non-English sites
Component: configuration system » base system
Priority: Normal » Major

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.

pjonckiere’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
1.03 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,218 pass(es). View

One cannot trust an install that ended in a timeout, or so it seems. I couldn't reproduce this with a Dutch or German installation. However, after I was able to run the installer without timeout (yeah, I know), I managed to reproduce it.

Patch included.

I guess this needs test, but I'm not sure where to put them?

cilefen’s picture

Status: Needs review » Needs work

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

cilefen’s picture

cilefen’s picture

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

pjonckiere’s picture

I'm writing a test based on LanguageSwitchingTest.php, but I couldn't get it to work yet.

cilefen’s picture

Status: Needs work » Needs review
FileSize
1.93 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,247 pass(es). View

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

pjonckiere’s picture

FileSize
2.71 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,239 pass(es), 3 fail(s), and 0 exception(s). View
3.75 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,252 pass(es), 3 fail(s), and 0 exception(s). View

Seems like we both were forgetting something. I forgot to set the default language, whereas you forgot to set another maintenance message. New patch included.

Edit: In this patch, the final assertion is not right. But after giving this extra thought, this isn't the right way to test this either.

The last submitted patch, 9: maintenance_mode-2212069-9.test-fail.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 9: maintenance_mode-2212069-9.patch, failed testing.

cilefen’s picture

+++ 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.

pjonckiere’s picture

Yes. I haven't been very careful submitting that patch :$

After fixing the patch (variable name in last name, and language parameter as you suggest), I notice that this still doesn't catch the bug.

  /**
   * @inheritdoc
   */
  protected function installParameters() {
    $parameters = parent::installParameters();
    $parameters['parameters']['langcode'] = 'nl';
    return $parameters;
  }

We could use something like this to change the installation language for the test, but then we should probably move it to a seperate test?

cilefen’s picture

We could try that as SiteMaintenanceMultilingualTest.

pjonckiere’s picture

FileSize
2.25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,277 pass(es), 1 fail(s), and 0 exception(s). View
3.28 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,719 pass(es). View

Let's try that as an approach. No interdiff since I reset the SiteMaintenanceTest and created the SiteMaintenanceMultilingualTest instead.

pjonckiere’s picture

Status: Needs work » Needs review

The last submitted patch, 15: maintenance_mode-2212069-15.test-fail.patch, failed testing.

cilefen’s picture

+++ 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.

pjonckiere’s picture

FileSize
2.22 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,797 pass(es), 1 fail(s), and 0 exception(s). View
3.25 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,730 pass(es). View

It seems like the patch applied anyway, making me miss those lines. Thanks for bringing it to my attention.

The last submitted patch, 20: maintenance_mode-2212069-20.test-fail.patch, failed testing.

Berdir’s picture

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.

Berdir’s picture

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.

alerque’s picture

Version: 8.0.x-dev » 8.0.0-beta6

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

Berdir’s picture

Version: 8.0.0-beta6 » 8.0.x-dev

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

Berdir’s picture

Priority: Major » Critical
Status: Needs review » Needs work
Issue tags: +D8MI, +language-config, +Needs issue summary update, +Needs D8 critical triage

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...)

Berdir’s picture

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 ;)

penyaskito’s picture

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?

Berdir’s picture

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?

Gábor Hojtsy’s picture

@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.

Gábor Hojtsy’s picture

Status: Needs work » Active

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

Gábor Hojtsy’s picture

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.

Gábor Hojtsy’s picture

Title: Maintenance mode message is ignored on non-English sites » Non-English Drupal sites get default configuration in English, edited in English, originals not actually used if translated
Issue summary: View changes
Issue tags: -Needs tests, -Needs issue summary update

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.

alerque’s picture

@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.

Gábor Hojtsy’s picture

Assigned: Unassigned » Gábor Hojtsy
Issue summary: View changes

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.

Gábor Hojtsy’s picture

@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.

Gábor Hojtsy’s picture

Issue tags: +sprint
FileSize
70.22 KB

@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.

Gábor Hojtsy’s picture

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).

alexpott’s picture

Issue tags: +D8 upgrade path

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

Gábor Hojtsy’s picture

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.

Gábor Hojtsy’s picture

Title: Non-English Drupal sites get default configuration in English, edited in English, originals not actually used if translated » [META] Non-English Drupal sites get default configuration in English, edited in English, originals not actually used if translated
Issue summary: View changes

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.

alexpott’s picture

@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.

Gábor Hojtsy’s picture

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.

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

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 :)

Gábor Hojtsy’s picture

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...

Gábor Hojtsy’s picture

Title: [META] Non-English Drupal sites get default configuration in English, edited in English, originals not actually used if translated » Non-English Drupal sites get default configuration in English, edited in English, originals not actually used if translated
Status: Active » Needs review
FileSize
54.95 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,989 pass(es), 46 fail(s), and 7 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 48: 2212069-48.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
56.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,016 pass(es), 15 fail(s), and 3 exception(s). View
4.82 KB

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.

Status: Needs review » Needs work

The last submitted patch, 50: 2212069-50.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
56.75 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,029 pass(es), 12 fail(s), and 3 exception(s). View
1.54 KB

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

Status: Needs review » Needs work

The last submitted patch, 52: 2212069-52.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
62.06 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,080 pass(es), 11 fail(s), and 2 exception(s). View
8.39 KB

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.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs review

Status: Needs review » Needs work

The last submitted patch, 54: 2212069-53.patch, failed testing.

alexpott’s picture

Issue tags: +Triaged D8 critical

@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.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
2.98 KB
63.41 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,039 pass(es), 11 fail(s), and 3 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 58: 2212069-58.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
64.01 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,268 pass(es), 10 fail(s), and 2 exception(s). View
620 bytes

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.

Status: Needs review » Needs work

The last submitted patch, 60: 2212069-60.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
66.75 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,287 pass(es), 3 fail(s), and 0 exception(s). View
4.91 KB

- 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.

Status: Needs review » Needs work

The last submitted patch, 62: 2212069-62.patch, failed testing.

tstoeckler’s picture

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.

Gábor Hojtsy’s picture

@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.

tstoeckler’s picture

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.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
68.97 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,519 pass(es), 3 fail(s), and 0 exception(s). View
14.75 KB

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().

Status: Needs review » Needs work

The last submitted patch, 67: 2212069-67.patch, failed testing.

Gábor Hojtsy’s picture

Issue summary: View changes

Started updating the issue summary.

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
69.46 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,516 pass(es), 3 fail(s), and 0 exception(s). View
1.33 KB

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.

Gábor Hojtsy’s picture

Issue summary: View changes
FileSize
1.68 KB
69.15 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,511 pass(es), 3 fail(s), and 0 exception(s). View

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.

The last submitted patch, 71: 2212069-71.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 72: 2212069-72.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
69.37 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,514 pass(es), 3 fail(s), and 0 exception(s). View
1.4 KB

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.

Status: Needs review » Needs work

The last submitted patch, 75: 2212069-74.patch, failed testing.

Gábor Hojtsy’s picture

Issue summary: View changes
FileSize
83.03 KB

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).

Gábor Hojtsy’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
71.09 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,562 pass(es), 3 fail(s), and 0 exception(s). View
4.09 KB

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.

Gábor Hojtsy’s picture

Issue summary: View changes

Also update remaining steps.

Status: Needs review » Needs work

The last submitted patch, 79: 2212069-79.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
73.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,485 pass(es), 13 fail(s), and 11 exception(s). View
5.53 KB

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

Status: Needs review » Needs work

The last submitted patch, 82: 2212069-82.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
73.59 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,569 pass(es), 2 fail(s), and 0 exception(s). View
865 bytes

Wrong variable name.

Status: Needs review » Needs work

The last submitted patch, 84: 2212069-84.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
75.3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,591 pass(es), 1 fail(s), and 0 exception(s). View
5.46 KB

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.

Status: Needs review » Needs work

The last submitted patch, 86: 2212069-86.patch, failed testing.

dawehner’s picture

+++ 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....

Gábor Hojtsy’s picture

@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.

Gábor Hojtsy’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
75.38 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,641 pass(es). View
652 bytes

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.

Gábor Hojtsy’s picture

Issue tags: +Needs tests

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.

Gábor Hojtsy’s picture

Issue summary: View changes
FileSize
75.39 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,647 pass(es). View
1.55 KB

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

Gábor Hojtsy’s picture

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.

Gábor Hojtsy’s picture

FileSize
3.61 KB

The exported view I forgot to attach

Gábor Hojtsy’s picture

@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.

Gábor Hojtsy’s picture

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.

Gábor Hojtsy’s picture

Issue summary: View changes

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.

Gábor Hojtsy’s picture

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.

Gábor Hojtsy’s picture

Issue tags: -Needs tests
FileSize
27.41 KB
96.24 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,454 pass(es). View

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.

Gábor Hojtsy’s picture

Assigned: Gábor Hojtsy » Unassigned
Issue summary: View changes

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

Gábor Hojtsy’s picture

FileSize
97.58 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,521 pass(es), 3 fail(s), and 0 exception(s). View
1.34 KB

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.

Status: Needs review » Needs work

The last submitted patch, 101: 2212069-101.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
99.96 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,523 pass(es). View
2.38 KB

Those were easy to fix.

Gábor Hojtsy’s picture

Issue summary: View changes

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

Berdir’s picture

Assigned: Unassigned » Gábor Hojtsy
Status: Needs review » Needs work
FileSize
15.69 KB

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.

Berdir’s picture

Assigned: Gábor Hojtsy » Unassigned
Status: Needs work » Needs review

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

Gábor Hojtsy’s picture

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.

Gábor Hojtsy’s picture

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.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

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

Gábor Hojtsy’s picture

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?

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
99.97 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,104 pass(es). View
711 bytes

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.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

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.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
99.82 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,046 pass(es), 3 fail(s), and 1 exception(s). View
2.46 KB

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.

Status: Needs review » Needs work

The last submitted patch, 113: 2212069-113.patch, failed testing.

Gábor Hojtsy’s picture

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?

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
103.64 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,459 pass(es). View
4.68 KB

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).

Gábor Hojtsy’s picture

Issue summary: View changes

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

Gábor Hojtsy’s picture

#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.

YesCT’s picture

are the images in the issue summary up to date?

Gábor Hojtsy’s picture

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

effulgentsia’s picture

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.

alexpott’s picture

Status: Needs review » Needs work
  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?
Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
104.97 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,801 pass(es). View
9.01 KB

@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.

Gábor Hojtsy’s picture

FileSize
10.6 KB
114.22 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,272 pass(es). View

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.

Gábor Hojtsy’s picture

FileSize
114.59 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,271 pass(es). View
1.08 KB

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

Gábor Hojtsy’s picture

FileSize
114.75 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,285 pass(es). View
1.91 KB

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.

Gábor Hojtsy’s picture

FileSize
119.69 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,313 pass(es), 4 fail(s), and 0 exception(s). View
5.47 KB

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?

Status: Needs review » Needs work

The last submitted patch, 127: 2212069-127.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
121.95 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,711 pass(es). View
7.05 KB

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 :)

rteijeiro’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
178.41 KB
177.06 KB

Looks good to me!

Config Export BEFORE

Config Export AFTER

Berdir’s picture

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? :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

For #131

rteijeiro’s picture

Status: Needs work » Needs review
FileSize
122.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
6.81 KB

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.

rteijeiro’s picture

FileSize
147.71 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
971 bytes

Sorry, messed the comment.

rteijeiro’s picture

FileSize
122.03 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,691 pass(es). View

Argh! Rebased now! Previous interdiff if ok.

The last submitted patch, 133: 2212069-132.patch, failed testing.

The last submitted patch, 134: 2212069-134.patch, failed testing.

Gábor Hojtsy’s picture

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 :)

Gábor Hojtsy’s picture

@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

Gábor Hojtsy’s picture

Berdir’s picture

  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.

Gábor Hojtsy’s picture

FileSize
121.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
821 bytes

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.

Gábor Hojtsy’s picture

FileSize
1.77 KB
121.9 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,726 pass(es). View

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

The last submitted patch, 142: 2212069-142.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 143: 2212069-143.patch, failed testing.

tim.plunkett’s picture

+++ 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

Status: Needs work » Needs review

Gábor Hojtsy queued 143: 2212069-143.patch for re-testing.

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

FileSize
127.75 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,707 pass(es), 1 fail(s), and 0 exception(s). View
6.92 KB

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.

Status: Needs review » Needs work

The last submitted patch, 149: 2212069-149.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
129.32 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2212069-151.patch. Unable to apply patch. See the log in the details link for more information. View
1.57 KB

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?

Gábor Hojtsy’s picture

Issue summary: View changes

Updated API changes including the getLanguageWithFallback removal.

Gábor Hojtsy’s picture

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:

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.

Gábor Hojtsy’s picture

rteijeiro’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 151: 2212069-151.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
129.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,621 pass(es). View
effulgentsia’s picture

Issue tags: +Configuration system

Retroactively tagging for tracking.

Gábor Hojtsy’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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;
+  }

I think instead of adding another method we should overload getDefinition() with a $top_level parameter. Also I think we should open a followup to discuss removing it.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
129.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,567 pass(es). View
6.89 KB

I thought of integrating that into getDefinition() originally but given that TypedConfigManager already ignores the second argument, it felt wrong to add yet one more optional argument after an ignored argument. Also it allowed me to document the getDefinitionForConfigNames() with API docs, while this relatively more obscure bool argument is harder to grasp. I understand that this way there is a one stop shop to get the definition and is to some degree more standard among plugin managers (even if the signature is different).

Gábor Hojtsy’s picture

I added #2459971: The langcode key on configuration files is not explicit in configuration schema. Not sure it makes sense to document/publish http://drupal.org/node/2458579 then, if we are to roll it back soon?

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Back at @alexpott then.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This is fantastic work and it's great to see mono-lingual sites not in English become easier to understand and have the same performance as a monolingual English site. I agree that we should not publish http://drupal.org/node/2458579 until we've resolved what to do on #2459971: The langcode key on configuration files is not explicit in configuration schema.

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed c34478f and pushed to 8.0.x. Thanks!

  • alexpott committed c34478f on 8.0.x
    Issue #2212069 by Gábor Hojtsy, pjonckiere, rteijeiro, cilefen: Non-...
Gábor Hojtsy’s picture

Published 6 of out of the 7 change notices then. Deleting http://drupal.org/node/2458579, so nobody publishes it accidentally. For reference, this was the content of it:

The langcode top level key was used in all configuration files which may contain language specific content. With the changes in #2212069: Non-English Drupal sites get default configuration in English, edited in English, originals not actually used if translated all configuration files are now required to have a top level langcode key with correct information on the language of the file. Configuration entities may never use the langcode 'und' because they may have third party settings that are language specific. Although this key is required, it is not required to define it anymore.

The new TypedConfigManager::getSchemaForConfigName() was added and is now used to retrieve configuration schema for configuration names. This method automatically adds the langcode key to the schema if not already defined.

Gábor Hojtsy’s picture

Issue tags: -sprint

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Gábor Hojtsy’s picture