Problem/motivation

Drupal 8 has configuration schemas to describe the structure of configuration. This was proposed in part to resolve regressions and keep content types, menu items, etc. translatable. The schemas describe the structure of configuration but not yet which items are translatable and there is no connection with the software translation system (locale module) in core to use that information to make shipped configuration translatable. This solves regressions making shipped settings like email texts, content types, etc. translatable again.

Proposed solution

Patches at #1648930: Introduce configuration schema and use for translation all included this integration but that was postponed on other attempts and #1866610: Introduce Kwalify-inspired schema format for configuration got committed after all, which was more minimalistic. So we need to resurrect all the previously written code and adapt to the current schema system. Take things removed in progress in #1866610: Introduce Kwalify-inspired schema format for configuration and the feature set delta between that and #1648930: Introduce configuration schema and use for translation and make it work with the committed schema system.

Changed user interfaces

The only user interface change is a new step is added in the installer to import configuration translations. Module enable and disable batches will last a bit longer because the configuration translation update steps are included in the existing batches.

API changes

Configuration schema elements get a 'translatable' boolean property, and the text and label types are marked true. Core already uses this guideline to use text and label types for UI facing text elements that are to be made translatable, so this is just codifying an existing pattern.

API additions

Drupal\locale\LocaleConfigManager is added to wrap shipped configuration and access with translations. locale_config() is added in locale.module to access an instance of that. Drupal\locale\LocaleTypedConfig is added to wrap each config Element.

Steps to test #

1. Apply patch.
2. When installing Drupal and enabling modules(Language, Interface Translation), strings from the configuration of said modules will show up under Admin > Configuration > User interface translation.
3. Search for strings there like the shipped user role names ("Authenticated user"), the default contact category ("Website feedback"), block placement titles ("User login", "Footer menu"), content type names and descriptions ("Basic page", "Use basic pages for your static content, such as an 'About us' page.") etc.
4. Translate them on this user interface (some/all of them may already be translated based on the .po files imported from localize.drupal.org, feel free to modify the translations for testing too).
5. Watch as the translation is being used when you switch language on the site (on the block, add content page, contact page, user editing screen in a foreign language, etc). (The contact category title will show on the dedicated URL of the contact category, eg. /contact/feedback for the default one, it does not show on the main /contact page).

Remaining tasks

  • Needs more manual testing and code reviews, then done :) (The patch is functionally complete, tests are included and expanded. )
  • figure out if we want to keep implementing Drupal\Core\TypedData\TranslatableInterface, which is what makes the getTranslationLanguages() weird code and the strict mode in getTranslation() required. (see #153)

Followups

#1966538: Translation is not updated for configuration, when the config changes
#2029075: Configuration translation step in the installation takes a reeeeaaallly long time when installing in a non-English language

CommentFileSizeAuthor
#154 interdiff.txt7.76 KBGábor Hojtsy
#154 1905152-metadata-locale-integration-154.patch55.46 KBGábor Hojtsy
#153 interdiff.txt6.95 KBGábor Hojtsy
#153 1905152-metadata-locale-integration-153.patch56.9 KBGábor Hojtsy
#151 1905152-metadata-locale-integration-151.patch58.57 KBvijaycs85
#151 1905152-diff-149-151.txt1.54 KBvijaycs85
#149 1905152-metadata-locale-integration-149-debug.patch58.49 KBvijaycs85
#147 1905152-metadata-locale-integration-147.patch58.44 KBvijaycs85
#147 1905152-diff-145-147.txt1.36 KBvijaycs85
#147 1905152-metadata-locale-integration-147-debug.patch58.59 KBvijaycs85
#145 1905152-metadata-locale-integration-145.patch58.57 KBvijaycs85
#145 1905152-diff-142.5-145.txt3.54 KBvijaycs85
#142 1905152-metadata-locale-integration-142-1.patch57.53 KBvijaycs85
#142 1905152-metadata-locale-integration-142-2.patch57.67 KBvijaycs85
#142 1905152-metadata-locale-integration-142-3.patch58.08 KBvijaycs85
#142 1905152-metadata-locale-integration-142-4.patch57.63 KBvijaycs85
#142 1905152-metadata-locale-integration-142-5.patch57.75 KBvijaycs85
#140 1905152-metadata-locale-integration-140.patch58.56 KBvijaycs85
#136 1905152-metadata-locale-integration-136.patch59.06 KBvijaycs85
#136 1905152-diff-133-136.txt6.29 KBvijaycs85
#133 1905152-metadata-locale-integration-133.patch58.47 KBvijaycs85
#133 1905152-diff-131-133.txt5.14 KBvijaycs85
#131 1905152-metadata-locale-integration-131.patch58.1 KBvijaycs85
#131 1905152-diff-119-131.txt7.06 KBvijaycs85
#119 1905152-metadata-locale-integration-118.patch57.5 KBvijaycs85
#119 1905152-diff-117-118.txt4.65 KBvijaycs85
#119 1905152-metadata-locale-integration-118.patch57.67 KBvijaycs85
#119 1905152-diff-117-118.txt4.82 KBvijaycs85
#117 1905152-metadata-locale-integration-117.patch57.5 KBGábor Hojtsy
#108 1905152-metadata-locale-integration-108.patch66.94 KBGábor Hojtsy
#108 interdiff.txt1.19 KBGábor Hojtsy
#104 interdiff.102-104.txt1.88 KBpenyaskito
#104 1905152-metadata-locale-integration-104.patch66.8 KBpenyaskito
#102 1905152-metadata-locale-integration-102.patch66.94 KBvijaycs85
#102 1905152-diff-100-102.txt1.92 KBvijaycs85
#100 1905152-metadata-locale-integration-100.patch67.49 KBvijaycs85
#100 1905152-diff-95-100.txt14.13 KBvijaycs85
#98 step-2.png25 KBvijaycs85
#98 step-3.png39.86 KBvijaycs85
#98 step-4.1.png26.25 KBvijaycs85
#98 step-4.2.png25.41 KBvijaycs85
#95 1905152-metadata-locale-integration-94.patch56.03 KBJose Reyero
#91 1905152-metadata-locale-integration-91-same-as-72-rerolled.patch56.03 KBGábor Hojtsy
#88 1905152-metadata-locale-integration-88.patch61.42 KBvijaycs85
#82 1905152-metadata-locale-integration-82.patch61.94 KBJose Reyero
#82 1905152-metadata-locale-integration-82-interdiff.txt2.84 KBJose Reyero
#75 interdiff.txt4.97 KBGábor Hojtsy
#75 1905152-metadata-locale-integration-75.patch61.1 KBGábor Hojtsy
#73 interdiff.txt3.89 KBGábor Hojtsy
#73 1905152-metadata-locale-integration-73.patch59.09 KBGábor Hojtsy
#72 interdiff.txt3.97 KBGábor Hojtsy
#72 1905152-metadata-locale-integration-72.patch56.55 KBGábor Hojtsy
#67 interdiff.txt2.23 KBGábor Hojtsy
#67 1905152-metadata-locale-integration-67.patch52.59 KBGábor Hojtsy
#65 1905152-metadata-locale-integration-65.patch111.05 KBYesCT
#65 interdiff-54-65.txt2.46 KBYesCT
#63 fix.txt1003 bytesGábor Hojtsy
#62 interdiff-36-54.txt21.92 KBYesCT
#54 1905152-metadata-locale-integration-54.patch49.65 KBYesCT
#54 interdiff-49-54.txt809 bytesYesCT
#49 1905152-metadata-locale-integration-49.patch49.68 KBYesCT
#49 interdiff-45-49.txt4.96 KBYesCT
#45 1905152-metadata-locale-integration-45.patch49.45 KBYesCT
#45 interdiff-42-45.txt941 bytesYesCT
#42 1905152-metadata-locale-integration-42.patch49.45 KBYesCT
#42 interdiff-36-42.txt20.96 KBYesCT
#41 Screenshot_3_17_13_2_56_PM.png73.02 KBGábor Hojtsy
#36 1905152-metadata-locale-integration-36.patch48.52 KBJose Reyero
#36 1905152-metadata-locale-integration-36.diff.txt22.87 KBJose Reyero
#32 1905152-metadata-locale-integration-32.patch47.6 KBJose Reyero
#31 1905152-metadata-locale-integration-31.patch48.03 KBJose Reyero
#25 1905152-metadata-locale-integration-25.patch42.43 KBvijaycs85
#21 interdiff.txt3.8 KBGábor Hojtsy
#21 metadata-locale-integration-21.patch46.29 KBGábor Hojtsy
#18 metadata-locale-integration-18.patch46.27 KBGábor Hojtsy
#16 metadata-locale-integration-16.patch46.27 KBGábor Hojtsy
#14 interdiff.txt2.97 KBGábor Hojtsy
#14 metadata-locale-integration-14.patch43.81 KBGábor Hojtsy
#12 interdiff.txt7.92 KBGábor Hojtsy
#12 metadata-locale-integration-12.patch43.66 KBGábor Hojtsy
#10 metadata-locale-integration-10.patch39.5 KBGábor Hojtsy
#8 cmi-locale.txt43.84 KBGábor Hojtsy
#2 metadata-locale-integration-2.patch1.05 KBGábor Hojtsy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Bring over some tags from #1648930: Introduce configuration schema and use for translation since this affects missing features in VDC and the config system somewhat.

Gábor Hojtsy’s picture

Status: Active » Needs review
FileSize
1.05 KB

Restoring the translatable key from #1866610: Introduce Kwalify-inspired schema format for configuration (#13). This is far from doing the scope of the issue. Also I think this did not have any test coverage, but we should get that as part of the locale integration.

Status: Needs review » Needs work
Issue tags: -Regression, -Configuration system, -D8MI, -sprint, -language-config, -VDC

The last submitted patch, metadata-locale-integration-2.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review

#2: metadata-locale-integration-2.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, metadata-locale-integration-2.patch, failed testing.

plach’s picture

Status: Needs work » Needs review

#2: metadata-locale-integration-2.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Regression, +Configuration system, +D8MI, +sprint, +language-config, +VDC

The last submitted patch, metadata-locale-integration-2.patch, failed testing.

Gábor Hojtsy’s picture

FileSize
43.84 KB

Also a quick extract of the locale related stuff from #1648930: Introduce configuration schema and use for translation. Obviously almost none of that would apply now so just a txt for posterity, while we figure out where should things go.

Gábor Hojtsy’s picture

Status: Needs work » Needs review

#2: metadata-locale-integration-2.patch queued for re-testing.

Gábor Hojtsy’s picture

Merged in all of those except the getLanguage() from TypedConfig(), not sure where that would go in the new system. Most things will not pass yet due to the straight merge using the old metadata stuff :D Need to work those out.

Status: Needs review » Needs work

The last submitted patch, metadata-locale-integration-10.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
43.66 KB
7.92 KB

Restoring changes in InstallerStorage too for the locale code. The LocaleTypedConfig will need a lot more thinking since it is based on the TypedConfig one by one CMI key accessor idea from #1648930: Introduce configuration schema and use for translation. The current metadata system uses a factory for so it needs to be reimagined on the locale side.

Status: Needs review » Needs work

The last submitted patch, metadata-locale-integration-12.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
43.81 KB
2.97 KB

Made minimal changes in the LocaleTypedConfig class to reflect the now missing underlying structure; it still behaves as a Config-alike solution like before.

Status: Needs review » Needs work

The last submitted patch, metadata-locale-integration-14.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
46.27 KB

All right, needed to restore most of the hunks from #1884182: Import of large number of translations fails on SQLite (except the last hunk that actually caused the SQLite issue), since tracking the individual strings is needed for us to be able to look up exactly which CMI items to update. Without this tracking, we'd need to update all of the CMI translation files, which sounds like would be a bad idea.

The test fails properly pointed at this problem BTW :)

Status: Needs review » Needs work

The last submitted patch, metadata-locale-integration-16.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
46.27 KB

Patch only applied with various offsets. Rerolling. Looking at remaining fails.

Gábor Hojtsy’s picture

So the failures are all down to Fatal error: Call to a member function read() on a non-object in core/lib/Drupal/Core/Config/Config.php on line 416 which is $data = $this->storage->read($this->name); in Config. That is due to LocaleTypedConfig extending Config but not actually implementing it much. Looks like I bugosly tracked down that it was intended to be coming from Config. I could not track down the full thinking behind the original version.

In http://drupal.org/node/1648930#comment-6826770 - TypedConfig extends Config and LocaleTypedConfig extends TypedConfig which is where I took extending LocaleTypedConfig direct from Config here (since we don't have TypedConfig as a wrapper or any similar solution in core with the kwalify based approach).

Will need to spend more time to track this down.

Status: Needs review » Needs work

The last submitted patch, metadata-locale-integration-18.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
46.29 KB
3.8 KB

All right, figured out we do need the injectability of $data for any CMI key since for the translation itself, we need only the original values, so the CMI-sourced data needs to be filtered before it is passed in. So LocaleTypedConfig is not really suitable as an extension to Config or TypedConfigManager for that matter. It can use config_typed() (TypedConfigManager) to dress up data with type information though.

So added a bare-data returning getData() method and a get() which returns typed data wrapped data. This mapped exactly into how this class is already used so just filling in holes. Also found out that system.site now only has 6 elements vs. the 7 there were at the time of the original test being written. Also figured out Config Element has no relation to most typed data elements and isTranslatable with a CMI condition would not make sense on most Typed Data elements, so put back the direct definition based logic similar to what it was before.

This will still fail since LocaleTypedConfig's get() will not return a reusable Config instance like some places assume it does. I did not find a clear cut way right away to make this work nicely.

Also I'm just trying to fill holes while trying to figure out the logic behind the code, so some solutions might be stop-gaps only. However, this version is now fully capable of actually translating a site name and an image style name, etc. as is mostly proven by the test :)

Status: Needs review » Needs work

The last submitted patch, metadata-locale-integration-21.patch, failed testing.

Gábor Hojtsy’s picture

Issue tags: +Configuration schema

Tagging for config schema as well as keeping on sprint :)

Gábor Hojtsy’s picture

BTW #1914366: Move all configuration schema files into a schema subdirectory uses the same patch portion for InstallStorage to be able to read .yml files from other places under components, not just the regular config directory. Hopefully we can get that committed soon which would make this patch smaller :) That should not mean no work should be done here.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
42.43 KB

Re-rolling excluding InstallStorage changes...

Status: Needs review » Needs work

The last submitted patch, 1905152-metadata-locale-integration-25.patch, failed testing.

Jose Reyero’s picture

Assigned: Unassigned » Jose Reyero

I'll be trying to update this one, possibly dropping or simplifying the LocaleTypedConfig object which doesn't make that much sense anymore.

Gábor Hojtsy’s picture

#1763640: Introduce config context to make original config and different overrides accessible got committed, so the system is all there to welcome these files and actually use them :)

vijaycs85’s picture

Status: Needs work » Needs review
Issue tags: -Regression, -Configuration system, -D8MI, -sprint, -language-config, -VDC, -Configuration schema

Status: Needs review » Needs work
Issue tags: +Regression, +Configuration system, +D8MI, +sprint, +language-config, +VDC, +Configuration schema

The last submitted patch, 1905152-metadata-locale-integration-25.patch, failed testing.

Jose Reyero’s picture

New patch, lots of changes, not there yet though.

Main changes:
- Reworked LocaleTypedConfig, now it is a wrapper for Config\Schema\Element.
- Added LocaleConfigManager as a new service that encapsulates most of the funcionality:
- It is in the DIC, can be accesses with locale_config().
- Extends TypedConfigManager so it can get TypedConfig objects with default configuration data (instead of current configuration).
- Provides most of the API that was previously in locale.bulk.inc. See methods compareConfigData(), saveConfigData(), getComponentNames(), etc. This allows us to reuse the same config storage objects for all of them.

Still to do:
- Test partially updated but not passing yet.
- Update strings after a bulk translation import.
- Update locale config overrides after any configuration is updated.

Anyway I thought I would share this one to get feedback on the new LocaleConfigManager approach.

Jose Reyero’s picture

Status: Needs work » Needs review
FileSize
47.6 KB

Done some more improvements, cleanup, fixed tests.
- Moved translateString() method into LocaleConfigManager
- Simplified LocaleTypedConfig wrapper, now extending Element

Still working on:
- Update strings after a bulk translation import.
- Update locale config overrides after any configuration is updated.

Status: Needs review » Needs work

The last submitted patch, 1905152-metadata-locale-integration-32.patch, failed testing.

Gábor Hojtsy’s picture

First off, good stuff. I like how the Locale typed data manager just extends the config typed data manager and how the locale element extends the config element. That should ensure we have added features but also use the same features. That is config > typed config > localized typed config would be layers on top of each other cleanly. That said, no complaints on the approach at all!

So most of my comments are around docs missing or incorrectly provided.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleBundle.phpundefined
@@ -24,6 +24,11 @@ public function build(ContainerBuilder $container) {
+    $container->register('locale.config.typed', 'Drupal\locale\LocaleConfigManager')
+      ->addArgument(new Reference('config.storage'))
+      ->addArgument(new Reference('config.storage.schema'))
+      ->addArgument(new Reference('config.storage.installer'));

Why no argument for locale storage? Not in the DIC? I see the constructor defaults to locale_storage(), but it looks odd only this has a fallback value.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleConfigManager.phpundefined
@@ -0,0 +1,259 @@
+/**
+ * Manages config type plugins.
+ */

Manages localized configuration type plugins. No?

+++ b/core/modules/locale/lib/Drupal/locale/LocaleConfigManager.phpundefined
@@ -0,0 +1,259 @@
+   * @param \Drupal\locale\StringStorageInterface $localeStorage
+   *   The locale storage to use for reading string translations.

Add to the start: "(optional)". Add to the end: "Defaults to locale_storage()".

+++ b/core/modules/locale/lib/Drupal/locale/LocaleConfigManager.phpundefined
@@ -0,0 +1,259 @@
+    // Note we get only the data that didn't change from default.
+    $default = $this->installStorage->read($name);

Note is not really true. Or at least not to the immediately following line.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleConfigManager.phpundefined
@@ -0,0 +1,259 @@
+  /**
+   * Save translated configuration data.
+   *
+   * @param string $name
+   * @param string $langcode
+   * @param array $data
+   */
+  public function saveConfigData($name, $langcode, $data) {

"Saves ...".

Also would be great to document these arguments. Eg. $data would be only the translated values for example.

Also, would it make sense to name these more concretely, like saveTranslationData? Since the manager class inherits from typed data manager, it could look odd that we have this generic method name but its not really dealing with typed data, it is specifically for translations.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleConfigManager.phpundefined
@@ -0,0 +1,259 @@
+  /**
+   * Save translated configuration data.
+   *
+   * @param string $name
+   * @param string $langcode
+   * @param array $data
+   */
+  public function deleteConfigData($name, $langcode) {
+    $locale_name = 'locale.config.' . $langcode . '.' . $name;
+    $this->configStorage->delete($locale_name);
+  }

Copy-paste first line :) Also same for param documentation and method naming.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleConfigManager.phpundefined
@@ -0,0 +1,259 @@
+   *   Array of language codes

Missing .

+++ b/core/modules/locale/lib/Drupal/locale/LocaleConfigManager.phpundefined
@@ -0,0 +1,259 @@
+  public function deleteComponents($components, $langcodes) {

Maybe name this deleteComponentTranslations? (similar to above). It is not really deleting any components, it reacts to removal of components from the live system.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleConfigManager.phpundefined
@@ -0,0 +1,259 @@
+  /**
+   * Delete configuration for language.
+   *
+   * @param $langcode
+   *   Language code to delete.
+   */
+  public function deleteLanguage($langcode) {
+    $locale_name = 'locale.config.' . $langcode;
+    foreach ($this->configStorage->listAll($locale_name) as $name) {
+      $this->configStorage->delete($name);
+    }
+  }

I like how the naming scheme makes this function easy :) Have the same comment on the naming as above.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleConfigManager.phpundefined
@@ -0,0 +1,259 @@
+      return $translation->isTranslation() ? $translation->getString() : FALSE;

It would be great to document what you do with this.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleTypedConfig.phpundefined
@@ -0,0 +1,210 @@
+ * Contains Drupal\locale\LocaleTypedConfig.

\Drupal

+++ b/core/modules/locale/lib/Drupal/locale/LocaleTypedConfig.phpundefined
@@ -0,0 +1,210 @@
+  /**
+   * The configuration name.
+   *
+   * @var string
+   */
+  //protected $name;

Remove?

+++ b/core/modules/locale/lib/Drupal/locale/LocaleTypedConfig.phpundefined
@@ -0,0 +1,210 @@
+  /**
+   * Constructs a configuration wrapper object.
+   *
+   * @param string $name
+   *   The configuration object name.
+   * @param \Drupal\Core\Config\Schema\Element $typedConfig
+   *   Typed configuration element.
+   * @param string $langcode
+   *   Language code for the source configuration data.
+   * @param \Drupal\locale\LocaleConfigManager $localeConfig;
+   *   Locale configuration manager that produced this wrapper.
+   */
+  public function __construct(array $definition, $name, $langcode, \Drupal\locale\LocaleConfigManager $localeConfig) {

The documentation and the actual arguments don't match up at all.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleTypedConfig.phpundefined
@@ -0,0 +1,210 @@
+  /**
+   * Get translated configuration data.
+   *
+   * @param \Drupal\Core\Config\Schema\Element $element
+   *   Typed configuration element.
+   * @param array $options
+   *   Array with options that will depend on the translator used.
+   *
+   * @return array
+   *   Configuration data translated to the requested language.
+   */
+  protected function getElementTranslation($element, $options) {
+    $translation = NULL;
+    if ($element instanceof ArrayElement) {
+      $translation = $this->getArrayTranslation($element, $options);
+    }
+    elseif ($this->translateElement($element, $options) || empty($options['strict'])) {
+      $translation = $element->getValue();
+    }
+    if ($translation || empty($options['strict'])) {
+      return $translation;
+    }
+    else {
+      return NULL;
+    }
+  }
+
+  /**
+   * Get translated configuration data.
+   *
+   * @param \Traversable $element
+   *   Typed configuration element.
+   * @param array $options
+   *   Array with options that will depend on the translator used.
+   *
+   * @return array
+   *   Configuration data translated to the requested language.
+   */
+  protected function getArrayTranslation($element, $options) {
+    $translation = array();
+    foreach ($element as $key => $property) {
+      $value = $this->getElementTranslation($property, $options);
+      if (isset($value)) {
+        $translation[$key] = $value;
+      }
+    }
+    return $translation;
+  }
+
+  /**
+   * Translates element's value if it fits our translation criteria.
+   *
+   * For an element to be translatable by locale module it needs to be of base
+   * type 'string' and have 'translatable = TRUE' in the element's definition.
+   * Translatable elements may use these additional keys in their data
+   * definition:
+   * - 'translatable', FALSE to opt out of translation.
+   * - 'locale context', to define the string context.
+   *
+   * @param \Drupal\Core\TypedData\TypedDataInterface $element
+   *   Configuration element.
+   * @param array $options
+   *   Array with translation options that are dependent on the translator.
+   *
+   * @return bool
+   *   Whether the element fits the translation criteria.
+   */
+  protected function translateElement($element, $options) {
+    if ($this->canTranslate($options['source'], $options['target'])) {
+      $definition = $element->getDefinition();
+      $value = $element->getValue();
+      if ($value && !empty($definition['translatable'])) {
+        $context = isset($definition['locale context']) ? $definition['locale context'] : '';
+        if ($translation = $this->localeConfig->translateString($this->name, $options['target'], $value, $context)) {
+          $element->setValue($translation);
+          return TRUE;
+        }
+      }
+    }
+    // The element doesn't have a translation, if strict mode we drop it.
+    return empty($options['strict']);
+  }

Are these all custom to this class, not defined on any interface? It looks strange that $element on the three methods use entirely different type hints. That is pretty odd. Also inline type hints in the code would be good.

I like the translation criteria is documented on translateElement, but $options is very cryptic at all places. We should find a place to document what can be in there. "dependent on the translator" is pretty obscure. What kind of translators there are? How can one figure out?

Eg. strict and non-strict mode are useful options to document for translation access.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleConfigTranslationTest.phpundefined
@@ -0,0 +1,157 @@
+    //$typed_config = config_typed()->get('system.site');
+    //$wrapper = new LocaleTypedConfig('system.site', 'en', $typed_config, $this->storage);
+    $wrapper = locale_config()->get('system.site');

Remove commented out lines? The last line seems to be the current API, right?

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleConfigTranslationTest.phpundefined
@@ -0,0 +1,157 @@
+    // This really should be testing entity_load_multiple.
+    //$typed_config = config_typed()->get('image.style.medium');
+    //$wrapper = new LocaleTypedConfig('image.style.medium', 'en', $typed_config, $this->storage);
+    $wrapper = locale_config()->get('image.style.medium');

Same, commented out code can be removed. I don't understand the first line of this comment though...

Is that to mean we should have tests for entity_load_multiple() as to whether loading config entities through that would apply language overrides? That is pre the scope of this issue IMHO, not really related to what we are doing here? Regardless, entity loading should also equally work with the page negotiated language that is.

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -277,8 +281,12 @@ function locale_translate_export_form_submit($form, &$form_state) {
+ *   - 'components': Array of arrays of components to refresh the configuration
+ *     indexed by type ('module' or 'theme'). Optional, defaults to none.

No 'profile' component?

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -537,12 +548,66 @@ function locale_translate_batch_import_save($context) {
+      // We will update configuration on next steps.

... in later steps?

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -537,12 +548,66 @@ function locale_translate_batch_import_save($context) {
+    // Not perfect but will give some idea of progress.

indication of progress.

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -685,3 +758,131 @@ function locale_translate_delete_translation_files($projects = array(), $langcod
+ * Update all configuration for names / languages.

Updates

+++ b/core/modules/locale/locale.moduleundefined
@@ -508,11 +515,15 @@ function locale_themes_disabled($themes) {
+ * @param $components
+ *   An array of arrays of component (theme and/or module) names to import
+ *   translations for, indexed by type.
...
+
+  $components += array('module' => array(), 'theme' => array());
+  $list = array_merge($components['module'], $components['theme']);
+

@@ -541,37 +556,41 @@ function locale_system_update($components) {
+ *   An array of arrays of component (theme and/or module) names to import
+ *   translations for, indexed by type.
...
+  $components += array('module' => array(), 'theme' => array());
+  $list = array_merge($components['module'], $components['theme']);

No 'profile' component?

+++ b/core/modules/locale/locale.moduleundefined
@@ -1312,3 +1356,17 @@ function _locale_rebuild_js($langcode = NULL) {
+ * @see Drupal\Core\TypedData\TypedDataManager::create()
+ *
+ * @return Drupal\locale\LocaleConfigManager

\Drupal

Gábor Hojtsy’s picture

Priority: Major » Critical

Elevating now that #1616594: META: Implement multilingual CMI is marked duplicate.

Jose Reyero’s picture

Status: Needs work » Needs review
FileSize
22.87 KB
48.52 KB

Fixed multiple issues:
- Added back refreshing after manual translation update, that was breaking tests before.
- Fixed all issues pointed out by Gábor in #34, some replies below
- Added back the full feature of updating config translations after importing a translation file. To prevent issues like the previous bug caused by this features, js and config translations are refreshed on multiple steps, 100 strings at a time, see #1884182: Import of large number of translations fails on SQLite

I think the only missing features is refreshing the locale configuration after configuration updates from system forms.
And maybe one more test to check config translation updates after a po import.

About #34 agree with all, only two notes:
> Why no argument for locale storage? Not in the DIC? I see the constructor defaults to locale_storage(), but it looks odd only this has a fallback value.
Yup, not yet in DIC, that should be a different patch and then it would be trivial to add it as a parameter here.

> No 'profile' component?
Profile components are translated only on install or when retranslating all configuration so configuration names for these will be returned by InstallStorage::listAll() but 'profiles' never need to be passed around when updating only specific components, that should be only 'modules' and 'themes' when enabled / disabled.
This fact may need some explanation somewhere, just I don't know where it would make sense.

Back to needs review.

Gábor Hojtsy’s picture

Thanks for the update! I might not be able to review this for a few days due to other obligations, but will get to this sometime this week or early next week latest. Other reviewers would be great of course :)

aspilicious’s picture

+++ b/core/modules/locale/lib/Drupal/locale/LocaleConfigManager.phpundefined
@@ -0,0 +1,266 @@
+   * Save translated configuration data.

Saves

+++ b/core/modules/locale/lib/Drupal/locale/LocaleConfigManager.phpundefined
@@ -0,0 +1,266 @@
+   * So far we only know how to translate strings from English so we check
+   * whether the source data is English.
+   * Unlike regular t() translations, strings will be added to the source

Is this still true? I can't find any checks for english in the code.

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -685,3 +755,131 @@ function locale_translate_delete_translation_files($projects = array(), $langcod
+ * Build a locale batch to refresh configuration.

Builds

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -685,3 +755,131 @@ function locale_translate_delete_translation_files($projects = array(), $langcod
+ * Perform configuration translation refresh as a batch step.

Performs

Gábor Hojtsy’s picture

As for locale storage not in the DIC, ok.

Profile components are translated only on install or when retranslating all configuration so configuration names for these will be returned by InstallStorage::listAll() but 'profiles' never need to be passed around when updating only specific components, that should be only 'modules' and 'themes' when enabled / disabled.
This fact may need some explanation somewhere, just I don't know where it would make sense.

But profiles will behave as modules on an installed site, right? How is that solved in this setup?

Gábor Hojtsy’s picture

BTW looking through the interdiff, all changes look good.

Gábor Hojtsy’s picture

Also tried the patch manually. First off, it works wonderfully. I got locale.config.hu files created for my Hungarian translations in the installer, eg. locale.config.hu.system.maintenance.yml, locale.config.hu.user.mail.yml, locale.config.hu.views.view.archive.yml, etc. Updating translations from locale's UI worked. :) Wonderful!

This functionality has almost no UI, but the little it has can be improved a bit I think. The installer spawns too many tasks for language stuff when you go to install in a foreign language in my view. Can we piggy-back the configuration translation update on the existing second translation import wizard step / batch? There are already two of those, and this patch adds one more:

Screenshot_3_17_13_2_56_PM.png

It would also be great to present a percentage complete in the config batch instead of a number / total like it is now IMHO.

As for the two missing items:

I think the only missing features is refreshing the locale configuration after configuration updates from system forms.

Do you want to tie this to forms? Or rather config save events? (I'd say events, that is more API friendly :). I assume the translation files would be re-generated based on locale db translations when the config files are re-saved. (The contrib config translation module will need a similar solution when config files are saved, although it does not have the luxury of a db backend where the strings can be pulled from anytime to regenerate files in different structures). Do you want to target this for the current patch before commit? (It is true that before this patch, there was nobody managing these files).

And maybe one more test to check config translation updates after a po import.

That would be great too yup :)

Stopped reviewing. Looking forward to the updates! I think this is shaping up to be very nice.

YesCT’s picture

I had some awesome explanations in dreditor, but then I lost them. Sad.

So, in general:
most of this was docs and standards review, based on:
http://drupal.org/node/1354
http://drupal.org/coding-standards
http://drupal.org/node/1353118 (namespaces)
http://drupal.org/node/325974 (simple test)
#1158720: [policy, no patch] Add parameter type hinting to function declaration coding standards

Status: Needs review » Needs work

The last submitted patch, 1905152-metadata-locale-integration-42.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review

I didn't read through all the code in detail.

The part I did spend some time on was this:

+++ b/core/modules/locale/lib/Drupal/locale/LocaleConfigManager.phpundefined
@@ -0,0 +1,282 @@
+  /**
+   * Translates string using the localization system.
+   *
+   * So far we only know how to translate strings from English so the source
+   * string should be in English.
+   * Unlike regular t() translations, strings will be added to the source
+   * tables only if this is marked as default data.
+   *
+   * @param string $name
+   *   Name of the configuration location.
+   * @param string $langcode
+   *   Language code to translate to.
+   * @param string $source
+   *   The source string, should be English.
+   * @param string $context
+   *   The string context.
+   *
+   * @return string|false
+   *   Translated string if there is a translation, FALSE if not.
+   */
+  public function translateString($name, $langcode, $source, $context) {
+    if ($name && $langcode && $source && $context) {
+      // If translations for a language have not been loaded yet.
+      if (!isset($this->translations[$name][$langcode])) {
+        // Preload all translations for this configuration name and language.
+        $this->translations[$name][$langcode] = array();
+        foreach ($this->localeStorage->getTranslations(array('language' => $langcode, 'type' => 'configuration', 'name' => $name)) as $string){
+          $this->translations[$name][$langcode][$string->context][$string->source] = $string;
+        }
+      }
+      if (!isset($this->translations[$name][$langcode][$context][$source]) {
+        // There is no translation of the source string in this config location
+        // to this language for this context.
+        if ($translation = $this->localeStorage->findTranslation(array('source' => $source, 'context' => $context, 'language' => $langcode))) {
+          // Look for a translation of the string. It might have one, but not
+          // be saved in this configuration location yet.
+          // If the string has a translation for this context to this language,
+          // save it in the configuration location so it can be looked up faster
+          // next time.
+          $string = $this->localeStorage->createString((array) $translation)
+            ->addLocation('configuration', $name)
+            ->save();
+        }
+        else {
+          // No translation was found. Add the source to the configuration
+          // location so it can be translated, and the string is faster to look
+          // for next time.
+          $translation = $this->localeStorage
+            ->createString(array('source' => $source, 'context' => $context))
+            ->addLocation('configuration', $name)
+            ->save();
+        }
+
+        // Add an entry, either the translation found, or a blank string object
+        // to track the source string, to this configuration location, language,
+        // and context.
+        $this->translations[$name][$langcode][$context][$source] = $translation;
+      }
+
+      $translation = $this->translations[$name][$langcode][$context][$source];
+      // Return the string only when the string object had a translation.
+      return $translation->isTranslation() ? $translation->getString() : FALSE;
+    }
+    return FALSE;
+  }
+
+}

The comments I changed here, need to be checked if they are accurate.

Also, $name confused me. I think it's what is referred to in the code as the location.

Is that the hash of the active config, or something else?

YesCT’s picture

YesCT’s picture

+++ b/core/modules/locale/lib/Drupal/locale/LocaleConfigManager.phpundefined
@@ -214,53 +216,67 @@ public function deleteLanguageTranslations($langcode) {
   public function translateString($name, $langcode, $source, $context) {
-    if ($source) {
...
+    if ($name && $langcode && $source && $context) {

I could not see a reason to check just $source. So I added checking the rest of the arguments too.

YesCT’s picture

from irc with @Gábor Hojtsy
Some of this might be good in the issue summary.

tagging for needs manual testing http://drupal.org/node/1489010
updating the issue summary (aka issue summary initiative). http://drupal.org/node/1155816

---

this exposes default (shipped) config via the locale module, so you can translate pieces of default config there
anonymous user name, user email subjects and bodies from account settings, etc.
some views stuff as well as much as we have schema for that now
it works but some more testing helps :)

well, it creates the locale.config.* files in the installer from the .po file imported earlier
and it also lets you modify the translations via locale
the interface translation UI
for SHIPPED configuration files
(not for ones you create)
also if you slightly edit the config, it will still translate the rest of the pieces
since it identifies translatables key by key :)

its a very important missing piece and it in fact works

Status: Needs review » Needs work

The last submitted patch, 1905152-metadata-locale-integration-45.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
FileSize
4.96 KB
49.68 KB

some fixes for type hinting.

YesCT’s picture

+++ b/core/modules/locale/lib/Drupal/locale/LocaleConfigManager.phpundefined
@@ -84,13 +84,13 @@ public function get($name) {
    * @param array $default
    *   Default configuration data.
-   * @param array $updated
-   *   Current configuration data.
+   * @param array|false $updated
+   *   Current configuration data, or FALSE if no configuration data existed.
...
-  protected function compareConfigData(array $default, array $updated) {
+  protected function compareConfigData(array $default, $updated) {

I'm wondering if should change the $default one similarly, since it comes from a ->read too
http://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Config%21S...

Do we need some logic here for when one of them is false? Like another speedup?

protected function compareConfigData(array $default, $updated) {
    // Speed up comparison, specially for install operations.
    if ($default === $updated) {
      return $default;
    }
    $result = array();
    foreach ($default as $key => $value) {
      if (isset($updated[$key])) {
        if (is_array($value)) {
          $result[$key] = $this->compareConfigData($value, $updated[$key]);
        }
        elseif ($value === $updated[$key]) {
          $result[$key] = $value;
        }
      }
    }
    return $result;
  }

note that gets called from

  public function get($name) {
    // Read default and current configuration data.
    $default = $this->installStorage->read($name);
    $updated = $this->configStorage->read($name);
    // We get only the data that didn't change from default.
    $data = $this->compareConfigData($default, $updated);
YesCT’s picture

+++ b/core/modules/locale/lib/Drupal/locale/LocaleTypedConfig.phpundefined
@@ -120,8 +120,9 @@ protected function canTranslate($from_langcode, $to_langcode) {
    * Gets translated configuration data for a typed configuration element.
    *
-   * @param \Drupal\Core\Config\Schema\Element $element
-   *   Typed configuration element.
+   * @param mixed $element
+   *   Typed configuration element, either \Drupal\Core\Config\Schema\Element or
+   *   \Drupal\Core\Config\Schema\ArrayElement.

@@ -129,7 +130,7 @@ protected function canTranslate($from_langcode, $to_langcode) {
-  protected function getElementTranslation(Element $element, array $options) {
+  protected function getElementTranslation($element, array $options) {

@@ -148,8 +149,8 @@ protected function getElementTranslation(Element $element, array $options) {
    * Gets translated configuration data for an element of type ArrayElement.
    *
-   * @param \Traversable $element
-   *   Typed configuration element.
+   * @param \Drupal\Core\Config\Schema\ArrayElement $element
+   *   Typed configuration array element.

@@ -157,7 +158,7 @@ protected function getElementTranslation(Element $element, array $options) {
-  protected function getArrayTranslation(\Traversable $element, array $options) {
+  protected function getArrayTranslation(ArrayElement $element, array $options) {

maybe that should be \Traversable instead of mixed? and then \Traversable in the type hint in the function definition.

Status: Needs review » Needs work

The last submitted patch, 1905152-metadata-locale-integration-49.patch, failed testing.

YesCT’s picture

+++ b/core/modules/locale/lib/Drupal/locale/LocaleConfigManager.phpundefined
@@ -214,53 +216,67 @@ public function deleteLanguageTranslations($langcode) {
-    if ($source) {
-      // Preload all translations for this configuration name and language.
+    if ($name && $langcode && $source && $context) {

putting this back to see if it does the trick.

YesCT’s picture

Status: Needs work » Needs review
FileSize
809 bytes
49.65 KB

running locally too. but could be faster on the testbot.

Status: Needs review » Needs work

The last submitted patch, 1905152-metadata-locale-integration-54.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review

passed locally. retesting.

YesCT’s picture

The last submitted patch, 1905152-metadata-locale-integration-54.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review

from 9 to 1 fail... but that fail passes locally.

YesCT’s picture

The last submitted patch, 1905152-metadata-locale-integration-54.patch, failed testing.

YesCT’s picture

FileSize
21.92 KB

a interdiff to @Jose Reyero 's last patch (#36)

Gábor Hojtsy’s picture

FileSize
1003 bytes

I think the fail is caused by the French translations actually being downloaded and applied to configuration (which is the goal of this patch :), while in the ConfigLocaleOverrideWebTest, we use system.site config overrides from config_test module. Those overrides are written over when the French overrides are written over when this functionality saves the overrides based on the French translation, so the tests will not pass. Writing the overrides over again after adding French solves the problem (and allows us to remove the locale.config.fr.system.site.yml from the config_test module which is not done in this addendum). I think a better fix would be to use an xx language code like most other tests to avoid the lengthy download/import cycle happening in the test (speed it up) and to make sure our overrides stick.

I don't know why this fail did not occur with Jose's patch, but it seems to be the side-effect of the goal of this issue in general.

Gábor Hojtsy’s picture

Also it did not fail for Jose, because that new test was committed 2 days ago (http://drupalcode.org/project/drupal.git/commitdiff/dd927b00083c96dd4255...) and Jose's patch was posted 8 days earlier than that.

YesCT’s picture

Status: Needs work » Needs review
FileSize
2.46 KB
111.05 KB

locally, my test is still not ending...

using a custom language instead of french.

Gábor Hojtsy’s picture

@Berdir points to #1887480: Automated download of translations when adding languages/enabling modules can not be disabled which proposes a way to disable the remote download for tests. I fully agree with that however the tests introduced here do not rely on the remote download either (plenty of other ways to test the functionality), so it is not required for these to work.

Gábor Hojtsy’s picture

The patch in 65 contains some cruft from unrelated things. I also ran the test locally on a modified version of 54 based on the interdiff from 65 and figured it still fails. The reason for that is this patch is essentially making locale the truth, the canonical source of translation string information for translatable config strings. That is intended, since otherwise how would it be confidently rewriting files when new translation updates are downloaded? Other modules will need to work with this, such as I opened this issue 2 weeks ago for config_translation UI that will make it compatible with this behaviour: #1936208: If config is translated from shipped config or partially edited, also push translations to locale storage.

So regardless of whether we add a real language like fr or a made up one like xx, since the truth comes from locale, the override file from config_test will not survive past adding the language (which is when locale refreshes config override files, and will remove the file we shipped for the test). There are two options, either we add the translation on the locale UI or we write the config override direct from the test *after* we add the language, so it will be effective. Since we are not interested in the longevity of the override here past the test, I think writing the config direct is fine.

This should fully pass now.

YesCT’s picture

also, #1936208: If config is translated from shipped config or partially edited, also push translations to locale storage related to the change from .yml to save() and
merging translations local/remote for example when the community updates translations
and
controlling which translations of your own can be overwritten on a string by string basis
[making a note of something said in irc so I can sort it out later]

Gábor Hojtsy’s picture

Title: Integrate CMI metadata system with locale module » Integrate config schema with locale, so shipped configuration is translated

Update issue title for better explanation of the feature. I'll look into updating issue summary tomorrow.

YesCT’s picture

Status: Needs review » Needs work

also, notes from irc:

need some more test stuff
import a .po file with translations and see how it applies
it currently only tests through the translation UI

(this might be both looking for more reviews and needing work to add the tests. :) )

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Added tests for config translation updates after a .po import. I extended the existing import tests, since those have utility functions to import .po files already. The missing feature of changed config needing to update override files is still missing. That was the only remaining concern that was brought up above and not yet resolved.

Gábor Hojtsy’s picture

Here is a first stab at the config update => config translation update patch. Extending the config event listener in locale, we check if the saved config is among locations identified for configuration (a file in shipped config originally), and update the translation file based on locale backend data. Test coverage also added. It does not pass yet, so the implementation is certainly not entirely correct. Will debug this more, but wanted to post as an interim step to avoid extra parallel work from others just in case...

Status: Needs review » Needs work

The last submitted patch, 1905152-metadata-locale-integration-73.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
61.1 KB
4.97 KB

Solved most test issues by making sure that the location check is not run if the locale database is not yet installed. There may be better ways to check this, but this was certainly one :) Also fixed the actual override bug with better checking for location, so now it actually updates the override in the locale.config.* space as designed. There are still in-page caches which don't make our in-page assertions in the test pass. I made cache clearing effective at two places, the locale config manager clears its internal cache for a config key, when it is saved and all override listeners re-set their overrides when config is saved (the config clears prior overrides out). This still does not make the strict translation override retrieval work in-request yet, so there is likely some other static in-page cache that is not yet cleared.

@reyero: can you help here? Looks like this is very close otherwise. Thanks!

Status: Needs review » Needs work

The last submitted patch, 1905152-metadata-locale-integration-75.patch, failed testing.

Gábor Hojtsy’s picture

I drastically updated the issue summary including steps to test, api changes/additions and remaining tasks. Please help do manual testing. Don't be shy testing this manually, the fails are not indicative of this not working :D

vijaycs85’s picture

The last submitted patch, 1905152-metadata-locale-integration-75.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

vijaycs85’s picture

Issue summary: View changes

Updated issue summary - link to steps

vijaycs85’s picture

Issue summary: View changes

Updated issue summary - link to steps

Jose Reyero’s picture

It looks to me like the main issue is DIC persistent components. When a module is disabled, the container is rebuilt, but LocaleConfigListener is attached to a service (ConfigContextFactory) that persists across container rebuilds, thus it is still loaded and called on subsequent config calls, even if the module is already disabled.

(I didn't know before about these 'Services to Persist' so... wondering whether thisy makes any sense...?)

Anyway, I'll try to see where in the code makes sense to fix this and post a patch.

Gábor Hojtsy’s picture

Yay, thanks!

Jose Reyero’s picture

Here's a first patch, that should fix many of the test errors. It takes care of unregistering the module's own event listeners when the module is disabled, to fix the issue explained in #80

Just for the record, this looks like a potential issue for any other modules defining event listeners too, so maybe this should be better fixed at the DIC level. But for now, we need to move on with this patch.

(Moving to 'needs review' to trigger testbot. I will be doing some more cleanup in this patch though)

Status: Needs review » Needs work

The last submitted patch, 1905152-metadata-locale-integration-82.patch, failed testing.

Jose Reyero’s picture

Well, the bad news is we didn't get many more tests passing; the good news is failing tests are a bit different, some tests not passing before are passing now, but some other tests failing.

We can get some more passing by adding back the 'installed schema version' check, though it seems it doesn't really get us much further. There are, again, interesting race conditions when installing / uninstalling modules. And one of them is the container being rebuilt, and the module bundle being added (so module's event listeners kicking in), before the module is actually installed. Similar things happen with the upgrade tests: locale module enabled, thus event listeners added, but the module schema not yet upgraded.

The problem is there are a lot of configuration updates from the point the module bundle is added till the point the module's schema is installed, and even then, there are some before the module is actually loaded, and I can see still error happening after the schema is installed because the configuration may get updated before the module is loaded.

We need to work out a better solution for this than just checking the module's schema version:
- Besides it being ugly, needing to check for the schema every time, this is not always enough, we'd need to check whether the module is actually loaded.
- The event listener relying on the module API is also a problem, it should depend on the container only (because it is actually there before the module is loaded) for what we need to add some more parameters here.
- It depending on locale storage is also a problem because it is not in the container, but created with locale_storage(), we may need to fix this one first of all.
- There are some other weird errors caused by check_requirements, since the system module updating the file system and the 'system.file' configuration triggers configuration updates too.
- Overall, the configuration system doesn't provide enough hooks and needing to implement an event listener to react on configuration updates is not the better solution either because it causes a lot of circular dependencies.

In short, we may need to do a lot of minor improvements, one at a time, to fix all these issues, but we'll be able to measure progress by the number of tests that are passing.

Gábor Hojtsy’s picture

Good investigative work, sad for the race conditions.

Jose Reyero’s picture

I forgot to mention, we should fix first this one #1813762: Introduce unified interfaces, use dependency injection for interface translation
That will move 'locale storage' fully into the DIC and would fix some of the issues and allow some other improvements on this one.

Gábor Hojtsy’s picture

I'm afraid if we fix the other one first, that could linger for quite a while. We have an API freeze to hit in 2.5 months. :/ So if we need the change tracking to have that, then maybe we want to make the scope smaller here, not deal with change following yet given we have a complete well working solution for that scope (it passes tests above :). And then have the change tracking be a followup dependent on #1813762: Introduce unified interfaces, use dependency injection for interface translation. What do you think?

That would allow us to push forward with the config_translation integration (which is also proposed for core even it might not have a chance :D). See #1952394: Add configuration translation user interface module in core (core issue) and #1936208: If config is translated from shipped config or partially edited, also push translations to locale storage and #1936186: Make use of locale API when it becomes available in the config translation queue. We'll take care of these issues there, however they are postponed on the basics of this issue :)

vijaycs85’s picture

Straight re-roll of #82 after 34e6309 commit.

Changes are:
1. CoreBundle.php
from:

diff --git a/core/lib/Drupal/Core/CoreBundle.php b/core/lib/Drupal/Core/CoreBundle.php
index ee63bfa..fc35f81 100644
--- a/core/lib/Drupal/Core/CoreBundle.php
+++ b/core/lib/Drupal/Core/CoreBundle.php
@@ -83,6 +83,10 @@ public function build(ContainerBuilder $container) {
     $container
       ->register('config.storage.schema', 'Drupal\Core\Config\Schema\SchemaStorage');
 
+    // Register installer configuration storage.
+    $container
+      ->register('config.storage.installer', 'Drupal\Core\Config\InstallStorage');
+

to

diff --git a/core/core.services.yml b/core/core.services.yml
index c73db7f..3f4e87f 100644
--- a/core/core.services.yml
+++ b/core/core.services.yml
@@ -86,6 +86,8 @@ services:
     arguments: ['@database', config_snapshot]
   config.storage.schema:
     class: Drupal\Core\Config\Schema\SchemaStorage
+  config.storage.installer:
+    class: Drupal\Core\Config\InstallStorage

2.localBundle.php
from

diff --git a/core/modules/locale/lib/Drupal/locale/LocaleBundle.php b/core/modules/locale/lib/Drupal/locale/LocaleBundle.php
index 7c2ca6e..61eae0a 100644
--- a/core/modules/locale/lib/Drupal/locale/LocaleBundle.php
+++ b/core/modules/locale/lib/Drupal/locale/LocaleBundle.php
@@ -24,6 +24,11 @@ public function build(ContainerBuilder $container) {
       ->addArgument(new Reference('language_manager'))
       ->addArgument(new Reference('config.context'))
       ->addTag('event_subscriber');
+    // Register the locale configuration data manager.
+    $container->register('locale.config.typed', 'Drupal\locale\LocaleConfigManager')
+      ->addArgument(new Reference('config.storage'))
+      ->addArgument(new Reference('config.storage.schema'))
+      ->addArgument(new Reference('config.storage.installer'));
   }
 
 }

to

diff --git a/core/modules/locale/locale.services.yml b/core/modules/locale/locale.services.yml
index 012a7d9..15763a4 100644
--- a/core/modules/locale/locale.services.yml
+++ b/core/modules/locale/locale.services.yml
@@ -4,3 +4,6 @@ services:
     tags:
       - { name: event_subscriber }
     arguments: ['@language_manager', '@config.context']
+  locale.config.typed:
+    class: Drupal\locale\LocaleConfigManager
+    arguments: ['@config.storage', '@config.storage.schema', '@config.storage.installer']
YesCT’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1905152-metadata-locale-integration-88.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
56.03 KB

Here is a reroll of #72 instead, which was the pre-change tracking state all passing :) The services were moved to services.yml files, so those needed the same changes explained in #88 but otherwise applied. So as I said above, what about accepting this as the scope, since its already a huge step ahead, lots of added tests, etc. and do the change tracking in a followup, so we can parallelise getting reviews on this with Jose working on #1813762: Introduce unified interfaces, use dependency injection for interface translation. :) I'll try to get reviewers on this as soon as its green :)

Status: Needs review » Needs work

The last submitted patch, 1905152-metadata-locale-integration-91-same-as-72-rerolled.patch, failed testing.

Jose Reyero’s picture

@Gábor, I think this (#92) is a great idea, since the other patch is going to take a while.

Gábor Hojtsy’s picture

Superb, and that only has one fail (for some reason) in a new test we added, so should be easier to contain :)

Jose Reyero’s picture

Status: Needs work » Needs review
FileSize
56.03 KB

Rerolled the patch fixing the only test fail (due to config file with added properties).

@@ -89,7 +89,7 @@ function testConfigTranslation() {
     // Get non strict translation and check we've got all properties.
     $translation = $wrapper->getTranslation($langcode, FALSE);
     $properties = $translation->getProperties();
-    $this->assertTrue(count($properties) == 5 && count($translation->get('page'
+    $this->assertTrue(count($properties) == 7 && count($translation->get('page'
     $this->assertEqual($properties['name']->getValue(), $site_name, 'Got the ri
Jose Reyero’s picture

Issue summary: View changes

Updated issue summary.

Gábor Hojtsy’s picture

Added #1966538: Translation is not updated for configuration, when the config changes as a followup for the change tracking. Marked postponed on this one and the t() DIC issue. Updated issue summary. Now needs manual testing and reviews :)

Gábor Hojtsy’s picture

Issue summary: View changes

Update for new scope.

Gábor Hojtsy’s picture

Issue summary: View changes

Expand remaining tasks.

Gábor Hojtsy’s picture

Issue summary: View changes

Add actual examples

Gábor Hojtsy’s picture

Issue summary: View changes

More fun for testing :)

Status: Needs review » Needs work

The last submitted patch, 1905152-metadata-locale-integration-94.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
25.41 KB
26.25 KB
39.86 KB
25 KB

Applied patch in #95 and followed steps in summary and seems it is working as expected.

Screenshots of steps:

Step 2:
step-2.png
Step 3:
step-3.png
Step 4:

step-4.1.png

step-4.2.png

Gábor Hojtsy’s picture

Superb, any code complaints? :)

vijaycs85’s picture

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleImportFunctionalTest.phpundefined
@@ -471,4 +536,26 @@ function getPoFileWithMsgstr() {
+  /**
+   * Helper function that returns a .po file with configuration translations.
+   */
+  function getPoFileWithConfig() {
+    return <<< EOF
+msgid ""
+msgstr ""
+"Project-Id-Version: Drupal 8\\n"
+"MIME-Version: 1.0\\n"
+"Content-Type: text/plain; charset=UTF-8\\n"
+"Content-Transfer-Encoding: 8bit\\n"
+"Plural-Forms: nplurals=2; plural=(n > 1);\\n"
+
+msgid "@site is currently under maintenance. We should be back shortly. Thank you for your patience."
+msgstr "@site karbantartás alatt áll. Rövidesen visszatérünk. Köszönjük a türelmet."
+
+msgid "Anonymous user"
+msgstr "Névtelen felhasználó"
+
+EOF;
+  }

Minor: Can't we have test .po file?

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -6,9 +6,12 @@
+use Drupal\locale\LocaleTypedConfig;
 use Drupal\locale\Gettext;
 use Drupal\locale\PoDatabaseReader;
 use Drupal\Core\Language\Language;
+use Drupal\Core\Config\InstallStorage;
+use Drupal\Core\Config\StorageException;

Doesn't look like added classes used here?

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -400,6 +407,9 @@ function locale_translate_batch_build($files, $options) {
     // Save the translation status of all files.
     $operations[] =  array('locale_translate_batch_import_save', array());
 

Minor: Double space after '='

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -400,6 +407,9 @@ function locale_translate_batch_build($files, $options) {
     $batch = array(
       'operations'    => $operations,
       'title'         => $t('Importing interface translations'),
@@ -537,12 +547,72 @@ function locale_translate_batch_import_save($context) {

Minor: space issue in array.

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -685,3 +754,136 @@ function locale_translate_delete_translation_files($projects = array(), $langcod
+  $batch = array(
+      'operations'    => $operations,
+      'title'         => $t('Updating configuration translations'),
+      'init_message'  => $t('Starting configuration update'),
+      'error_message' => $t('Error updating configuration translations'),
+      'file'          => drupal_get_path('module', 'locale') . '/locale.bulk.inc',

Minor: array format has more than one space in left.

Attached patch fixes few spacing issues.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Re the .po file, the other tests also use an inline .po file that is saved out and imported from there. I'd not reform that here, just use what the rest does. Re the spacing fixes and the unused classes, can you include them as fixes too? (I don't think those were in the attached patch?).

As for the diff, most space fixes are good, these two look odd (the first two):

+++ b/core/includes/install.core.incundefined
@@ -960,8 +960,8 @@ function install_verify_completed_task() {
-  // Do not trigger an error if the database query fails, since the database
-  // might not be set up yet.
+    // Do not trigger an error if the database query fails, since the database
+    // might not be set up yet.
   catch (Exception $e) {

This does not seem to be an improvement? The old one is correct, no?

+++ b/core/includes/install.core.incundefined
@@ -1485,7 +1485,7 @@ function install_select_language_form($form, &$form_state, $files = array()) {
       '#markup' => '<p>Translations will be downloaded from the <a href="http://localize.drupal.org">Drupal Translation website</a>. ' .
-                   'If you do not want this, select <em>English</em> and continue the installation. For more information, see the <a href="http://drupal.org/documentation/install">online handbook</a>.</p>',
+        'If you do not want this, select <em>English</em> and continue the installation. For more information, see the <a href="http://drupal.org/documentation/install">online handbook</a>.</p>',

Put the first line on its own line then too, indent with the same, no?

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
1.92 KB
66.94 KB

Thanks @Gábor Hojtsy. Updated wrong intention on comments and removed unused classes.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ b/core/includes/install.core.incundefined
@@ -1484,8 +1484,8 @@ function install_select_language_form($form, &$form_state, $files = array()) {
+      '#markup' =>'<p>Translations will be downloaded from the <a href="http://localize.drupal.org">Drupal Translation website</a>. ' .
+      'If you do not want this, select <em>English</em> and continue the installation. For more information, see the <a href="http://drupal.org/documentation/install">online handbook</a>.</p>',

This does not seem to be an improvement with =>' (no space there), the space there should definitely not be lost.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
66.8 KB
1.88 KB

The langcode property somehow gets dropped, so the count should be 6. Not sure if this is the desired behavior.
Also fixed the space nitpick.

vijaycs85’s picture

Oh yay!!! It's green... Thanks @penyaskito.

penyaskito’s picture

    $properties = $translation->getProperties();
    foreach ($properties as $prop)
      $this->verbose($prop->getString());

One of the values shown was 'en', so the one missing should be another one: candidates are slogan or email, but I couldn't check which one of both.
Also the langcode is a special case and I wouldn't expect 'en' but the localized value 'xx', not sure of the implications.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Ok, well, so the missing key is the email address. Which is pretty intriguing. It does show up with config inspector (as in the typed data wrapper for config), but it does not appear with the locale wrapper. I think this is likely indicative of a bug...

Keys returned in $properties for non-strict locale wrapper lookup:

array (
  0 => 'name',
  1 => 'slogan',
  2 => 'page',
  3 => 'admin_compact_mode',
  4 => 'weight_select_max',
  5 => 'langcode',
)

My active store file:

name: d8mi.localhost
mail: admin@example.com
slogan: ''
page:
  403: ''
  404: ''
  front: node
admin_compact_mode: '0'
weight_select_max: '100'
langcode: en
Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
1.19 KB
66.94 KB

All right, I dug into this a lot more. So everything works well as supposed to :D The locale typed config wrapper is supposed to return items which are identical to the original configuration. See the get() method in LocaleConfigManager that this stuff is based on. In simpletest, the original vs. the installed config difference looks like this (from debugging output in my test debug sesssion):

Original/shipped config:

array (
  'name' => 'Drupal',
  'mail' => '',
  'slogan' => '',
  'page' => 
  array (
    403 => '',
    404 => '',
    'front' => 'user',
  ),
  'admin_compact_mode' => '0',
  'weight_select_max' => '100',
  'langcode' => 'en',
)

Simpletest installed config:

array (
  'name' => 'Drupal',
  'mail' => 'simpletest@example.com',
  'slogan' => '',
  'page' => 
  array (
    403 => '',
    404 => '',
    'front' => 'user',
  ),
  'admin_compact_mode' => '0',
  'weight_select_max' => '100',
  'langcode' => 'en',
)

So simpletest does not change the site name or the front page (with the minimal profile used for testing), only the site email is updated. Given that that is different from the original config, it will not show up in the locale wrapper which is designed to recognise only the default values so it can pick the translatable pieces from there.

That said, all works well, adding more comments in the patch to make it easier to understand for future reviewers.

Any more reviews/concerns? :)

Gábor Hojtsy’s picture

I also installed fresh with the patch and the following config override files were installed:

locale.config.hu.block.block.bartik.login.yml    locale.config.hu.user.mail.yml
locale.config.hu.block.block.bartik.search.yml   locale.config.hu.user.settings.yml
locale.config.hu.block.block.seven.login.yml     locale.config.hu.views.view.archive.yml
locale.config.hu.menu.menu.admin.yml             locale.config.hu.views.view.backlinks.yml
locale.config.hu.menu.menu.footer.yml            locale.config.hu.views.view.comments_recent.yml
locale.config.hu.shortcut.set.default.yml        locale.config.hu.views.view.frontpage.yml
locale.config.hu.system.maintenance.yml          locale.config.hu.views.view.glossary.yml
locale.config.hu.taxonomy.vocabulary.tags.yml    locale.config.hu.views.view.taxonomy_term.yml
locale.config.hu.tour.tour.views-ui.yml          locale.config.hu.views.view.tracker.yml

Samples of those files:

locale.config.hu.system.maintenance.yml

message: 'Ez a webhely jelenleg karbantartás alatt áll. Tartalmunk rövidesen újra elérhető lesz. Köszönjük a türelmet.'

locale.config.hu.views.view.archive.yml

display:
  page_1:
    display_title: Oldal
  block_1:
    display_title: Blokk

locale.config.hu.user.mail.yml

cancel_confirm:
  subject: '[site:name] webhelyen [user:name] felhasználó törlési kérelme'
  body: "Kedves [user:name]!\r\n\r\nA [site:name] adminisztrátorához olyan kérés érkezett, hogy a felhasználót törölje az oldalról.\r\n\r\nA felhasználó [site:url-brief] törléséhez az alábbi hivatkozásra kell kattintani vagy be kell másolni a böngésző címsorába:\r\n\r\n[user:cancel-url]\r\n\r\nFIGYELEM: A felhasználó törlése később nem visszavonható.\r\n\r\nA hivatkozás egy nap múlva lejár és semmi nem történik, ha nem kerül felhasználásra.\r\n\r\n--  [site:name] csapata"
password_reset:
  subject: 'Új bejelentkezési információk [user:name] részére [site:name] webhelyen'
  body: "Kedves [user:name]!\r\n\r\n[site:name] webhelyen ennek a felhasználónak a nevében új jelszó iránti igény lett benyújtva.\r\n\r\n!uri_brief webhelyre az alábbi linkre kattintva, vagy az böngészőcímsorába másolva lehet belépni:\r\n\r\n[user:one-time-login-url]\r\n\r\nEz egy egyszer használható belépési mód. Egy nap után az érvényessége lejár, de semmilyen következménye nem lesz, ha nem használják fel.\r\n\r\nA belépés a webhely !edit_uri oldalára visz, ahol a jelszót is meg lehet majd változtatni.\r\n\r\n-- [site:name] csapata"
register_admin_created:
[.......]

All look good and correct. Also tested updating/changing translations. When I updated the "Page" translation to "Weboldal" from "Oldal", the override file was properly upated:

locale.config.hu.views.view.archive.yml after hand-edited translation.

display:
  page_1:
    display_title: Weboldal
  block_1:
    display_title: Blokk

Status: Needs review » Needs work
Issue tags: -Regression, -Needs manual testing, -Configuration system, -D8MI, -sprint, -language-config, -VDC, -Configuration schema

The last submitted patch, 1905152-metadata-locale-integration-108.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

The prior failure was due to core checkout fails, not related to this patch. This got code style fixups from vijaycs85, two people manually testing, good test coverage and me cross-checking the code from @reyero, so I think it looks good to go! Woo, let's get this in!

YesCT’s picture

YesCT’s picture

oops. tagged wrong issue. this summary is great.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/locale/lib/Drupal/locale/LocaleConfigManager.phpundefined
@@ -0,0 +1,282 @@
+    $locale_name = 'locale.config.' . $langcode . '.' . $name;
...
+    $locale_name = 'locale.config.' . $langcode . '.' . $name;

Let's create a public static function to do this... so that we can call LocaleConfigManager::localeConfigName($langcode, $name); from outside this class (i.e. the event listener) and then we have one place to change the filename.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleConfigManager.phpundefined
@@ -0,0 +1,282 @@
+    $components = array_filter($components);

Why do we need to do this?

+++ b/core/modules/locale/lib/Drupal/locale/LocaleConfigManager.phpundefined
@@ -0,0 +1,282 @@
+    $locale_name = 'locale.config.' . $langcode;

Maybe the public static idea is bad... but actually... if $name = null and you return this then maybe... dunno

+++ b/core/modules/locale/lib/Drupal/locale/LocaleConfigManager.phpundefined
@@ -0,0 +1,282 @@
+      $translation = $this->translations[$name][$langcode][$context][$source];
+      // Return the string only when the string object had a translation.
+      return $translation->isTranslation() ? $translation->getString() : FALSE;

the assignment to $translation is unnecessary... could be...

if ($this->translations[$name][$langcode][$context][$source]->isTranslation()) {
  return $this->translations[$name][$langcode][$context][$source]->getString();
}

As FALSE is returned by default for this function.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleTypedConfig.phpundefined
@@ -0,0 +1,209 @@
+   * Implements \Drupal\Core\TypedData\TranslatableInterface::getTranslationLanguages().
...
+   * Implements \Drupal\Core\TypedData\TranslatableInterface::getTranslation().
...
+   * Implements \Drupal\Core\TypedData\TranslatableInterface::language().

Should use the new {@inheritdoc}

+++ b/core/modules/locale/lib/Drupal/locale/LocaleTypedConfig.phpundefined
@@ -0,0 +1,209 @@
+    $languages = locale_translatable_language_list();
+    if ($include_default) {
+      $default = $this->language();
+      $languages[$default->langcode] = $default;
+    }
+    else {
+      unset($languages[$this->langcode]);
+    }
+    return $languages;

This looks a bit mad... at least needs a comment as to why we need add or remove depending on the $include_default param... ie why does locale_translatable_language_list() sometimes contain the default language and sometimes not...

+++ b/core/modules/locale/lib/Drupal/locale/LocaleTypedConfig.phpundefined
@@ -0,0 +1,209 @@
+    $data = $this->getElementTranslation($this->getTypedConfig(), $options);
+    return $this->localeConfig->create($this->definition, isset($data) ? $data : array());

if getElementTranslation returned an empty array the ternary on the return would not be needed.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleTypedConfig.phpundefined
@@ -0,0 +1,209 @@
+   * @return array
+   *   Configuration data translated to the requested language.

However, If are going to return NULL we need to document it.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleTypedConfig.phpundefined
@@ -0,0 +1,209 @@
+  protected function getElementTranslation($element, array $options) {
+    $translation = NULL;
+    if ($element instanceof ArrayElement) {
+      $translation = $this->getArrayTranslation($element, $options);
+    }
+    elseif ($this->translateElement($element, $options) || empty($options['strict'])) {
+      $translation = $element->getValue();
+    }
+    if ($translation || empty($options['strict'])) {
+      return $translation;
+    }
+    else {
+      return NULL;
+    }
+  }

This looks weird... because translateElement() returns FALSE only if !empty($options['strict']) ... so I think the OR is unnecessary.

Additionally, if ($translation || empty($options['strict'])) { I think the || empty($options['strict']) is unnecessary... could just by if (!empty($translation)) {

+++ b/core/modules/locale/lib/Drupal/locale/LocaleTypedConfig.phpundefined
@@ -0,0 +1,209 @@
+  /**
+   * Translates element's value if it fits our translation criteria.
+   *
+   * For an element to be translatable by locale module it needs to be of base
+   * type 'string' and have 'translatable = TRUE' in the element's definition.
+   * Translatable elements may use these additional keys in their data
+   * definition:
+   * - 'translatable', FALSE to opt out of translation.
+   * - 'locale context', to define the string context.
+   *
+   * @param \Drupal\Core\TypedData\TypedDataInterface $element
+   *   Configuration element.
+   * @param array $options
+   *   Array with translation options that must contain the following keys:
+   *   - 'source', Source language code.
+   *   - 'target', Target language code.
+   *   - 'strict', True to return only elements that actually have translation.
+   *
+   * @return bool
+   *   Whether the element fits the translation criteria.
+   */
+  protected function translateElement(\Drupal\Core\TypedData\TypedDataInterface $element, array $options) {
+    if ($this->canTranslate($options['source'], $options['target'])) {
+      $definition = $element->getDefinition();
+      $value = $element->getValue();
+      if ($value && !empty($definition['translatable'])) {
+        $context = isset($definition['locale context']) ? $definition['locale context'] : '';
+        if ($translation = $this->localeConfig->translateString($this->name, $options['target'], $value, $context)) {
+          $element->setValue($translation);
+          return TRUE;
+        }
+      }
+    }
+    // The element does not have a translation. If strict mode we drop it.
+    return empty($options['strict']);
+  }

From the function name I would expect this function to return the element... and throw an exception if it can't... the logic looks like at least one level of nesting could be removed.

... also need to document why we have the strict option... i.e. why does the FALSE exist.

Got tired at this point... will review tests and batch stuff tomorrow... therefore changing back to needs review.

Gábor Hojtsy’s picture

1. TODO: static method for CMI key: agreed

2. TODO: array_filter(): I don't know and did not find an apparent reason, @reyero should now

3. DISCUSSION: $translations FALSE assignment: I'm not sure your repeated 4 levels of nested arrays is better vs. saving it in a temp variable; the loosing of the ternary I agree with

4. TODO: inheritdoc: ok, I looked up http://drupal.org/node/1354#inheritdoc is how to do it

5. DEBATED: getTranslationLanguages() looking mad, that is how TranslatableInterface in TypedData dictates we implement this as far as I see

6. DISCUSSION: for getElementTranslation() to return an empty array: I believe it would return NULL if in strict mode and there was no translation; otherwise it would return an array; I'm not sure if we need/want to distinguish strict mode results from non-strict results

7. TODO: on why non-strict mode exists; the point of this patch is the strict mode; so the non-strict pass filters out changed config and the strict mode filters out config that is non-translatable (due to its type) or does not have translation (in locale's db); so strict mode results in only the config that **is translatable and is translated** in the nested structure of the config file; now it seems like strict mode is the only mode used outside of tests (see locale_config_update_multiple()), so if we consider only the needs of this patch, we can get rid of non-strict mode; I agree non-strict mode is a bit strange since it filters out changed things but not non-translatable things; if it would filter out non-translatable things as well, that may be more interesting however still no use in the direct use case of this patch as-is

8. DEBATED: translateElement() actually in-place translating an element vs. returning it translated; we can change the return value; I'm not sure about throwing an exception; most config entries in a config files are not translatable, so throwing an exception for each of them sounds very expensive as we go through attempting to translate each; this method is called for each element individually when assembling the translation

Note that I don't have time right now to actually turn the items I agree with into the patch, might not have until early next week :/

Gábor Hojtsy’s picture

Moved the whitespace only changes from this patch to #1968446: Whitespace fixes in language installer and config install storage. The only change in this patch from #108 is I cut out those. Keeping at needs review for at least testbot :)

Status: Needs review » Needs work

The last submitted patch, 1905152-metadata-locale-integration-117.patch, failed testing.

vijaycs85’s picture

Thanks for the review @alexpott and @Gábor Hojtsy. Tried few of TODO in your reviews comments here:

#116.1. LocaleConfigManager::localeConfigName() - Implemented.
#116.2. array_filter() - by default we would have $components['module'] and $components['theme'] without checking to include or not. So this filter will strip out empty array.
#116.4. {@inheritdoc} - Fixed.
in #115:

This looks weird... because translateElement() returns FALSE only if !empty($options['strict']) ... so I think the OR is unnecessary.

Additionally, if ($translation || empty($options['strict'])) { I think the || empty($options['strict']) is unnecessary... could just by if (!empty($translation)) {

- Fixed.

the assignment to $translation is unnecessary... could be...

- Fixed

However, If are going to return NULL we need to document it.

- Fixed.

if getElementTranslation returned an empty array the ternary on the return would not be needed.

- Fixed.
TODO:
1.

This looks a bit mad... at least needs a comment as to why we need add or remove depending on the $include_default param... ie why does locale_translatable_language_list() sometimes contain the default language and sometimes not...

2.

From the function name I would expect this function to return the element... and throw an exception if it can't... the logic looks like at least one level of nesting could be removed.

... also need to document why we have the strict option... i.e. why does the FALSE exist.

Jose Reyero’s picture

Some comments on #115 #116:

1. TODO: static method for CMI key: agreed

That should work. Though my preferred way for this to work would be having some 'locale.config.storage' in the DIC, kind of:

 drupal_container()->get('locale.config.storage')->getConfig($name, $langcode);

The aim would be to make the whole thing replaceable, if in the future we decide to store this data somewhere else.

Anyway, the first patch that implements any (static or storage) would be fine for me.

2. TODO: array_filter(): I don't know and did not find an apparent reason, @reyero should now

The reason is $components may look like array('module' => array(), 'theme' => array()), being actually empty but not evaluating to empty if we don't 'array_filter'

3. DISCUSSION: $translations FALSE assignment: I'm not sure your repeated 4 levels of nested arrays is better vs. saving it in a temp variable; the loosing of the ternary I agree with

I don't really mind which way, though my guess is a plain FALSE may save some memory for massive update operations. There's no point in caching empty string objects IMO.

4. TODO: inheritdoc: ok, I looked up http://drupal.org/node/1354#inheritdoc is how to do it

5. DEBATED: getTranslationLanguages()...

6. DISCUSSION: for getElementTranslation()...

7. TODO: on why non-strict mode exists...

8. DEBATED: translateElement() actually in-place translating an element vs. returning it translated...

About all these, ok with the doc changes, but about TranslateInterface, either we go with it as it is or we drop the whole thing and replace it with some procedural code.

*We don't really need it*, I've just used that interface because it was there and looked like the thing to implement but at this point -the more we discuss it gets worse- I'd say cost outweights benefits.

The whole thing could be a few lines of code in LocaleConfigManager but seriously trimming down LocaleTypedConfig.

alexpott’s picture

Firstly, @reyero thanks for all the work on this

drupal_container()->get('locale.config.storage')->getConfig($name, $langcode);

That sounds great... and yep replaceability will be very useful...

The whole thing could be a few lines of code in LocaleConfigManager but seriously trimming down LocaleTypedConfig.

If this is true... simplicity++ :)

Gábor Hojtsy’s picture

Status: Needs review » Needs work

#1968446: Whitespace fixes in language installer and config install storage included one private => public change as well (documented there) that is needed for this patch, so that is why the above failed I think.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleConfigManager.phpundefined
@@ -272,11 +272,23 @@ public function translateString($name, $langcode, $source, $context) {
+   * @param string $langcode
+   * @param null $name

$name is string, also document what are these :)

+++ b/core/modules/locale/lib/Drupal/locale/LocaleConfigManager.phpundefined
@@ -272,11 +272,23 @@ public function translateString($name, $langcode, $source, $context) {
+  public static function localeConfigName($langcode, $name = NULL) {
+    return  rtrim('locale.config.' . $langcode . '.' . $name, '.');

too many spaces before rtrim

@alexpott/@reyero: simplicity as well as familiarity should be weighted. The patch uses typed data translatable interface because then that is standarized; if we make up our own API for this, then people would need to learn it just for this. So I think keeping it for now is good. We can revise later and simplify if needed.

Marked needs work for vijaycs82's cleanup issues.

alexpott’s picture

Status: Needs work » Needs review
+++ b/core/includes/install.core.incundefined
@@ -1855,6 +1861,22 @@ function install_import_translations_remaining(&$install_state) {
+  module_load_include('bulk.inc', 'locale');

+++ b/core/modules/locale/locale.moduleundefined
@@ -521,11 +532,15 @@ function locale_system_update($components) {
+    module_load_include('bulk.inc', 'locale');

@@ -539,37 +554,41 @@ function locale_system_update($components) {
+    module_load_include('bulk.inc', 'locale');

@@ -772,6 +791,12 @@ function locale_form_language_admin_add_form_alter_submit($form, $form_state) {
+  module_load_include('bulk.inc', 'locale');

@@ -1051,6 +1076,25 @@ function _locale_refresh_translations($langcodes, $lids = array()) {
+    module_load_include('bulk.inc', 'locale');

Should use Drupal::moduleHandler()->loadInclude()

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleConfigTranslationTest.phpundefined
@@ -0,0 +1,157 @@
+
+    $wrapper = locale_config()->get('system.site');
+
+    // Get strict translation and check we've got only the site name.

should use $this->container->get('locale.config.typed')->get('system.site');

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleConfigTranslationTest.phpundefined
@@ -0,0 +1,157 @@
+    // Try more complex configuration data.
+    $wrapper = locale_config()->get('image.style.medium');

should use $this->container->get('locale.config.typed')->get('image.style.medium');

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleImportFunctionalTest.phpundefined
@@ -238,6 +238,70 @@ function testEmptyMsgstr() {
+    // Translations got recorded in the config system.
+    foreach ($config_strings as $config_key => $config_string) {
+      $wrapper = locale_config()->get($config_key);
+      $translation = $wrapper->getTranslation($langcode, TRUE);

should do $locale_config = $this->container->get('locale.config.typed'); outside the foreach and then $wrapper = $locale_config->get($config_key); inside it

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -537,20 +544,80 @@ function locale_translate_batch_import_save($context) {
+ *   - 'refresh_configuration': (optional) Whether or not to refresh configuration
+ *     strings after the import. Defaults to FALSE.
+ * @param array $context
+ *   Contains a list of strings updated and information about the progress.
+ */
+function locale_translate_batch_refresh(array $options, array &$context) {

Why is this optional and javascript not? Also if this is set then it appears that Javascript translations will not be refreshed...

TBH I'm a bit confused about 'refresh configuration'... is this functon locale_translate_batch_refresh serving two different purposes?

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -537,20 +544,80 @@ function locale_translate_batch_import_save($context) {
+      // Initialize multi-step string refresh.
+      $context['message'] = t('Updating translations for JavaScript and Configuration strings.');

This message seems to need to be different if 'refresh configuration' option is set

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -537,20 +544,80 @@ function locale_translate_batch_import_save($context) {
+      if (module_exists('dblog')) {

Should use Drupal::moduleHandler()->moduleExists()

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -685,3 +751,136 @@ function locale_translate_delete_translation_files($projects = array(), $langcod
+/**
+ * Finishes callback of system page locale import batch.
+ *
+ * @param bool $success
+ *   Information about the success of the batch import.
+ * @param array $results
+ *   Information about the results of the batch import.

Add @see locale_config_batch_build()

+++ b/core/modules/locale/locale.moduleundefined
@@ -1310,3 +1354,17 @@ function _locale_rebuild_js($langcode = NULL) {
+/**
+ * Returns the locale configuration manager service.
+ *
+ * Use the locale config manager service for creating locale-wrapped typed
+ * configuration objects.
+ *
+ * @see \Drupal\Core\TypedData\TypedDataManager::create()
+ *
+ * @return \Drupal\locale\LocaleConfigManager
+ */
+function locale_config() {
+  return drupal_container()->get('locale.config.typed');
+}

It would be great to not have this procedural helper... we should copy ../core/modules/views/lib/Drupal/views/Views.php and have a Locale public static class

Gábor Hojtsy’s picture

Why have a Locale class if you are advocating we use the container directly anyway at all cases where locale_config() would be used?

alexpott’s picture

Why have a Locale class if you are advocating we use the container directly anyway at all cases where locale_config() would be used?

Because we need to access the 'locale.config.typed' service from procedural code and having one line wrapper functions is :(

Gábor Hojtsy’s picture

We can just as well use drupal_container()->get('locale.config.typed') in place, introducing a whole new class so to avoid one function seems like quite a bit of overkill. Not really pointing to simplicity in my view :D

alexpott’s picture

drupal_container() is deprecated :)

Gábor Hojtsy’s picture

So much for simplicity :(

Berdir’s picture

drupal_container() is deprecated yes, in favor of Drupal::service(). Drupal::$method() and $Module::$method() are optional IMHO :) The main downside of not having those is that you don't get autocomplete for the methods on that object. But having global functions is deprecated in favor of such methods on a Drupal/Module class.

Various people argued very strongly for static methods on classes instead of functions in the issue that deprecated drupal_container(). The advantage of having a class is that it can be autoloaded and therefore also works in real PHPUnit unit tests if we can write those (I honestly don't really understand that argument as you shouldn't use the container in unit tests in theory anyway). But you can easily look up what services a certain module provides :)

Gábor Hojtsy’s picture

Well, if we just use Drupal::service() or drupal_container() direct for that matter, we loose the documentation we have on locale_config() now that is I think useful now. If the "new simple way" is we add a class with static methods instead and trade locale_storage() to Locale::Storage() because that is so much better, then so be it. It does not really matter for this patch, and others already decided this for us, so we don't need to.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
7.06 KB
58.1 KB

Fixed both review comments in #122 and #123 except below:

1. locale_config() - has got follow up at #1968732: replace config_locale() with Locale:storage()
2.

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -537,20 +544,80 @@ function locale_translate_batch_import_save($context) {
+ *   - 'refresh_configuration': (optional) Whether or not to refresh configuration
+ *     strings after the import. Defaults to FALSE.
+ * @param array $context
+ *   Contains a list of strings updated and information about the progress.
+ */
+function locale_translate_batch_refresh(array $options, array &$context) {

Why is this optional and javascript not? Also if this is set then it appears that Javascript translations will not be refreshed...

TBH I'm a bit confused about 'refresh configuration'... is this functon locale_translate_batch_refresh serving two different purposes?

3.

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -537,20 +544,80 @@ function locale_translate_batch_import_save($context) {
+      // Initialize multi-step string refresh.
+      $context['message'] = t('Updating translations for JavaScript and Configuration strings.');

This message seems to need to be different if 'refresh configuration' option is set

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Thanks! My understanding is #1968732: replace config_locale() with Locale:storage() should be included here, not in a separate followup. That separation would mean we add use of deprecated APIs with this patch, which is a no-go. I'll review the other changes later.

vijaycs85’s picture

Adding Locale class and its changes.

Status: Needs review » Needs work

The last submitted patch, 1905152-metadata-locale-integration-133.patch, failed testing.

Gábor Hojtsy’s picture

index 8901fb9..5ede5c1 100644
--- a/core/includes/install.core.inc

--- a/core/includes/install.core.inc
+++ b/core/includes/install.core.incundefined

+++ b/core/includes/install.core.incundefined
@@ -1872,7 +1872,7 @@ function install_import_translations_remaining(&$install_state) {

@@ -1872,7 +1872,7 @@ function install_import_translations_remaining(&$install_state) {
  * @see install_tasks()
  */
 function install_update_configuration_translations(&$install_state) {
-  module_load_include('bulk.inc', 'locale');
+  Drupal::moduleHandler()->loadInclude('locale', 'inc', 'bulk');
   return locale_config_batch_update_components(array(), array($install_state['parameters']['langcode']));
 }
 

install.core.inc does not seem to have a "use Drupal" at the top. This will not work.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleConfigManager.phpundefined
@@ -284,11 +284,14 @@ public function translateString($name, $langcode, $source, $context) {
+   * @param string|NULL $name
+   *   The name of the configuration object. Set NULL to get all $langcode
+   *   overridden  configurations.

"Name of the original configuration. Set to NULL to get the name prefix for all $langcode overrides."

Would be better IMHO :)

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -836,6 +836,8 @@ function locale_config_batch_refresh_name($name, array $langcodes, array &$conte
diff --git a/core/modules/locale/locale.module b/core/modules/locale/locale.module

diff --git a/core/modules/locale/locale.module b/core/modules/locale/locale.module
index 2bfe55c..429253f 100644

index 2bfe55c..429253f 100644
--- a/core/modules/locale/locale.module

--- a/core/modules/locale/locale.module
+++ b/core/modules/locale/locale.moduleundefined

+++ b/core/modules/locale/locale.moduleundefined
+++ b/core/modules/locale/locale.moduleundefined
@@ -539,7 +539,7 @@ function locale_system_update(array $components) {

@@ -539,7 +539,7 @@ function locale_system_update(array $components) {
       $batch = locale_translation_batch_update_build($list, array(), $options);
       batch_set($batch);
     }
-    module_load_include('bulk.inc', 'locale');
+    Drupal::moduleHandler()->loadInclude('locale', 'inc', 'bulk');

locale.module also does not have "use Drupal".

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
6.29 KB
59.06 KB

Updating review comments in #135 and renaming Locale to LocaleModule as there is a class Locale in PECL intl ext http://www.php.net/manual/en/class.locale.php

Status: Needs review » Needs work
Issue tags: -Regression, -Needs manual testing, -Configuration system, -D8MI, -sprint, -language-config, -VDC, -Configuration schema

The last submitted patch, 1905152-metadata-locale-integration-136.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

vijaycs85’s picture

As #117 passing, rolled back to it and updated changes in individual comments (good to have diff files!!!). Removed changes that would make this test fails. Also implemented the Locale class the way it is used in Symfony (\Symfony\Component\Locale\Locale::getLocales()).

Status: Needs review » Needs work

The last submitted patch, 1905152-metadata-locale-integration-140.patch, failed testing.

vijaycs85’s picture

Sorry to burn down testbot... Locally this patch failing with "Maximum function nesting level of '100' reached, aborting!".. trying to issue incremental patch from different changes...

Status: Needs review » Needs work

The last submitted patch, 1905152-metadata-locale-integration-142-5.patch, failed testing.

Gábor Hojtsy’s picture

The locale class we are looking for has nothing to do with symfony. Also you did not post interdiffs so it is kind of hard to tell what are you doing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
3.54 KB
58.57 KB

@Gábor Hojtsy, sorry for no-interdiff... I was trying to update changes one by one (from #117). Found few issues:
1. 'return' missing in #119

-      return $translation->isTranslation() ? $translation->getString() : FALSE;
+      if ($this->translations[$name][$langcode][$context][$source]->isTranslation()) {
+        $this->translations[$name][$langcode][$context][$source]->getString();
+      }

2. wrong file name: trying to load locale/bulk.inc instead of locale/locale.bulk.inc in #131

-    module_load_include('bulk.inc', 'locale');
+    Drupal::moduleHandler()->loadInclude('locale', 'inc', 'bulk');

Fixed them here. Hope it reduce the number of fails from 44 to 28 or 4.

Status: Needs review » Needs work

The last submitted patch, 1905152-metadata-locale-integration-145.patch, failed testing.

vijaycs85’s picture

1 fail that was fixed in #104. Seems its back to 7 properties... giving a try with 7 and a debug patch to see what exactly getting.

Status: Needs review » Needs work

The last submitted patch, 1905152-metadata-locale-integration-147-debug.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
58.49 KB

That was wrong debug change. adding new debug file (this patch in not for review).

Status: Needs review » Needs work

The last submitted patch, 1905152-metadata-locale-integration-149-debug.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
1.54 KB
58.57 KB

Currently getting only 4 properties and 1 page from test (as per #149 result). Hoping this fix makes it green...

vijaycs85’s picture

Remaining issues to address (from #115):

1.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleTypedConfig.phpundefined
@@ -0,0 +1,209 @@
+    $languages = locale_translatable_language_list();
+    if ($include_default) {
+      $default = $this->language();
+      $languages[$default->langcode] = $default;
+    }
+    else {
+      unset($languages[$this->langcode]);
+    }
+    return $languages;

This looks a bit mad... at least needs a comment as to why we need add or remove depending on the $include_default param... ie why does locale_translatable_language_list() sometimes contain the default language and sometimes not...
2.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleTypedConfig.phpundefined
@@ -0,0 +1,209 @@
+    $data = $this->getElementTranslation($this->getTypedConfig(), $options);
+    return $this->localeConfig->create($this->definition, isset($data) ? $data : array());

if getElementTranslation returned an empty array the ternary on the return would not be needed.
3.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleTypedConfig.phpundefined
@@ -0,0 +1,209 @@
+  protected function getElementTranslation($element, array $options) {
+    $translation = NULL;
+    if ($element instanceof ArrayElement) {
+      $translation = $this->getArrayTranslation($element, $options);
+    }
+    elseif ($this->translateElement($element, $options) || empty($options['strict'])) {
+      $translation = $element->getValue();
+    }
+    if ($translation || empty($options['strict'])) {
+      return $translation;
+    }
+    else {
+      return NULL;
+    }
+  }

This looks weird... because translateElement() returns FALSE only if !empty($options['strict']) ... so I think the OR is unnecessary.

Additionally, if ($translation || empty($options['strict'])) { I think the || empty($options['strict']) is unnecessary... could just by if (!empty($translation)) {

4.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleTypedConfig.phpundefined
@@ -0,0 +1,209 @@
+  /**
+   * Translates element's value if it fits our translation criteria.
+   *
+   * For an element to be translatable by locale module it needs to be of base
+   * type 'string' and have 'translatable = TRUE' in the element's definition.
+   * Translatable elements may use these additional keys in their data
+   * definition:
+   * - 'translatable', FALSE to opt out of translation.
+   * - 'locale context', to define the string context.
+   *
+   * @param \Drupal\Core\TypedData\TypedDataInterface $element
+   *   Configuration element.
+   * @param array $options
+   *   Array with translation options that must contain the following keys:
+   *   - 'source', Source language code.
+   *   - 'target', Target language code.
+   *   - 'strict', True to return only elements that actually have translation.
+   *
+   * @return bool
+   *   Whether the element fits the translation criteria.
+   */
+  protected function translateElement(\Drupal\Core\TypedData\TypedDataInterface $element, array $options) {
+    if ($this->canTranslate($options['source'], $options['target'])) {
+      $definition = $element->getDefinition();
+      $value = $element->getValue();
+      if ($value && !empty($definition['translatable'])) {
+        $context = isset($definition['locale context']) ? $definition['locale context'] : '';
+        if ($translation = $this->localeConfig->translateString($this->name, $options['target'], $value, $context)) {
+          $element->setValue($translation);
+          return TRUE;
+        }
+      }
+    }
+    // The element does not have a translation. If strict mode we drop it.
+    return empty($options['strict']);
+  }

From the function name I would expect this function to return the element... and throw an exception if it can't... the logic looks like at least one level of nesting could be removed.

... also need to document why we have the strict option... i.e. why does the FALSE exist.

5.

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -537,20 +544,80 @@ function locale_translate_batch_import_save($context) {
+ *   - 'refresh_configuration': (optional) Whether or not to refresh configuration
+ *     strings after the import. Defaults to FALSE.
+ * @param array $context
+ *   Contains a list of strings updated and information about the progress.
+ */
+function locale_translate_batch_refresh(array $options, array &$context) {

Why is this optional and javascript not? Also if this is set then it appears that Javascript translations will not be refreshed...
TBH I'm a bit confused about 'refresh configuration'... is this functon locale_translate_batch_refresh serving two different purposes?

6.

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -537,20 +544,80 @@ function locale_translate_batch_import_save($context) {
+      // Initialize multi-step string refresh.
+      $context['message'] = t('Updating translations for JavaScript and Configuration strings.');

This message seems to need to be different if 'refresh configuration' option is set

Gábor Hojtsy’s picture

1. FIXED: Added comment on getTranslationLanguages() why do we need the funky code there. The need for $include_default comes from the typed data interface we implement. The possible missing English is coming from the language system.

2. FIXED: Made getElementTranslation() return an empty array if no translation instead of NULL. Easier on the caller.

3. FIXED: (Was already resolved earlier). Further simplified now thanks to 2.

4. NOT FIXED: Due to strict mode which is required due to typed data translatable interface implementation (also see above with $include_default in (1)), we want different behavior based on this option. So we need to transform the element to translated AND tell the level up that we did (or not), so the level up can do different things based on that. So we can introduce a property on the element but that would either require a new property (which sounds like translations property on a level where it is not used outside of locale) on elements or extend elements in locale only, which sounds like a possible disaster (all kinds of element types are possible, extensible in contrib). @alexpott already agreed we should not use exceptions here btw, this is a utility method called for each element on each level in config essentially.

5. FIXED: Removed the 'refresh_configuration' option from batches, it should not be optional. When .po files are imported, locale database storage is updated, the config translations should be updated, the locale database and config translation should not be out of sync.

6. FIXED: Slightly updated wording on the batch message pointed out by @alexpott, IMHO "Configuration" should not be capitalized, its not a brand name like "JavaScript".
--------

In short, it sounds like we should figure out if we want to keep implementing Drupal\Core\TypedData\TranslatableInterface, which is what makes the getTranslationLanguages() weird code and the strict mode in getTranslation() required. Otherwise for me locally, the config tests pass proper. I'm not aware @alexpott had any other issues identified, and the interdiffs above look good fixing issues.

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary - steps update

YesCT’s picture

Issue summary: View changes

added remaining task for last discussion of approach

Gábor Hojtsy’s picture

Looking at this more, we are not even implementing the strict mode proper. The docs for strict mode are:

   * @param $strict
   *   (optional) If the data is complex, whether the translation should include
   *   only translatable properties. If set to FALSE, untranslatable properties
   *   are included (in default language) as well as translatable properties in
   *   the specified language. Defaults to TRUE.

So strict mode should only return translated things (that worked correct), while non-strict mode should return the full original data with translations in-place as available. The non-strict mode implemented by the patch did not work like that at all. It actually filters out data in non-strict mode and did not overlay translations. That is understandable given the role of this is NOT to overlay translations, that is the job of the config override system, and implementing a parallel solution to accessing that data would cause more confusion IMHO then use. So the code works down the data to only the translatable pieces and then only deals with that. If we'd want to have it do the overlay-ing thing for the data like the non-strict mode documents, that would mean we need to maintain per-element data about translatability or have entirely different code paths for the two modes. And even then we'd duplicate what is already being done in the config override/event system.

So in this light, I think its better to actually remove our reliance on this interface (which lets us remove code and simplify this API, even though making it not a standard implementation of typed data, we win by not over-complicating this).

I also found a random bugos string inside a code comment.

Status: Needs review » Needs work

The last submitted patch, 1905152-metadata-locale-integration-154.patch, failed testing.

Gábor Hojtsy’s picture

"Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupaltestbotmysql.simpletest936821cache_tags' doesn't exist: SELECT 1 AS expression" does not seem to be related.

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

Jose Reyero’s picture

Status: Needs review » Reviewed & tested by the community

Latest changes look great to me, looks much simpler, we didn't really need the Translatable interface at all.

There are some improvements I'd like to see, though I'll set this to RTBC because most of these would need this patch committed too, #1813762: Introduce unified interfaces, use dependency injection for interface translation

Notes for future improvements (or in case this needs some re-rolls):
- Repurpose LocaleTypedConfig since it's not too clear anymore which functionality should be encapsulated on each class. It should take care of config language ('langcode'), so the whole 'canTranslate' makes sense. It should also check the data against the defaults by itself (Some method like 'checkDefaults()')
* The main reason for this is this one can only translate 'default English configuration strings' but a module in the future could provide a more powerful one.
- locale_config_update_multiple() would be better a method in LocaleConfigManager. This will simplify existing code and allow replacing the full feature by setting a different LocaleConfigManager.
- Minor performance improvement in batch updates. The number of strings to query at once, currently 100, could be raised to maybe 900 (Safe for both Mysql and Sqlite), and maybe it could be configurable (passed as options for the batch or with a variable). I'm talking about these two lines:

    // Pending strings, refresh 100 at a time, get next pack.
    $next = array_slice($context['sandbox']['refresh']['strings'], 0, 100);

Anyway, as said above, most of this should use the full locale system in DIC and the TranslationInterface provided by the other patch so we better get this feature committed first and revisit it once/if the other patch gets in, that may take a bit longer.

Gábor Hojtsy’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 91fc0d3 and pushed to 8.x. Thanks!

alexpott’s picture

Title: Integrate config schema with locale, so shipped configuration is translated » Change notice: Integrate config schema with locale, so shipped configuration is translated
Status: Fixed » Active
Issue tags: +Needs change record
Gábor Hojtsy’s picture

Title: Change notice: Integrate config schema with locale, so shipped configuration is translated » Integrate config schema with locale, so shipped configuration is translated
Status: Active » Fixed
Issue tags: -Needs change record, -sprint

Thanks a lot everyone! This is a huge step forward for us! And one critical done nonetheless :)

I coped over #159 to #1966538-3: Translation is not updated for configuration, when the config changes so we track it with the followup.

Also reopened #1936186: Make use of locale API when it becomes available and #1936208: If config is translated from shipped config or partially edited, also push translations to locale storage for the contrib config_translation module (also proposed for core at #1952394: Add configuration translation user interface module in core), so we can fix compatibility with this new solution.

Updated docs at http://drupal.org/node/1905070/revisions/view/2658980/2659994 to explain how this reflects on the schema system and http://drupal.org/node/1928898/revisions/view/2590502/2659996 on how this reflects on the config override system.

I also added a change notice at http://drupal.org/node/1977368 that explains what this means for site builder, module devs and how it relates to localize.drupal.org, the schemas and overrides.

Status: Fixed » Closed (fixed)

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

Pancho’s picture

Another followup based on #41:

Can we piggy-back the configuration translation update on the existing second translation import wizard step / batch?

see #1993452: Fix confusing UX by merging "Translate configuration" into "Finish translations" task

YesCT’s picture

Issue summary: View changes

fix comment number to be a link

YesCT’s picture

Issue summary: View changes

added the performance feedback

Gábor Hojtsy’s picture

Issue summary: View changes
Issue tags: +Configuration context