Problem

With #1905152: Integrate config schema with locale, so shipped configuration is translated shipped configuration gets translation overrides saved in the configuration system from the imported translations (based on community translations on localize.drupal.org). However, when configuration changes, eg. the site name is configured or user notification emails are changed by the user, or a module update changes configuration, the translation files are not updated. This is a problem as translations get stale (they are not for the updated source string anymore) and if the structure of configuration changes, they will not properly overlay with the new configuration structure anymore.

This was removed from the original scope of #1905152: Integrate config schema with locale, so shipped configuration is translated because it was already at hundred comments with a more limited scope and solving this needs #1813762: Introduce unified interfaces, use dependency injection for interface translation resolved as well.

Proposed solution

Add a config save event listener that re-saves translation overrides appropriately reusing the existing LocaleConfigSubscriber subscriber (which currently does the runtime configuration translation only).

As the module is not always loaded when the configuration is updated we need also to remove some dependencies on the locale module or other module's libraries (locale.bulk.inc) from the locale components that are loaded into the DIC. In order to implement this in a clean way, we are also proposing some API clean up and consolidation

Some other minor API change is needed inside the Config objects to be able to tell when the object is being created or when it is being updated.

API Changes

Definition of some locale services:

New locale service, locale.config.storage (\Drupal\locale\LocaleConfigStorage), that provides the config storage backend functionality and groups all the configuration read/write operations from these locale components.

Repurposing Drupal\locale\LocaleConfigManager to be the component handling all the locale / configuration operations. Renamed the service from 'locale.config.typed' to 'locale.config.manager'.
• It doesn't need to extend TypedConfigManager, reducing its complexity, just needs to reuse the existing 'config.typed' service.
• Moved in some functions from locale.bulk.inc so they can be reused from other locale services without needing to load other files. (Such functions were using LocaleConfigManager anyway so we are not introducing any new dependency here).
• Greatly simplified LocaleTypedConfig which is used only by LocaleConfigManager atm. These simplifications allow a cleaner workflow, and moving some checks (like for which languages configuration should be translated) to LocaleConfigManager which saves dependencies, passing around a number of parameteres that are useless otherwise, and should imply some important performance improvements as languages are checked beforehand instead of creating multiple objects (one for each configuration object) just to see whether it was translatable or not.

New class \Drupal\locale\LocaleConfigTranslation that implements TranslatorInterface and is used for translating configuration strings, which adds consistency to all the translation system, instead of using a lot of specific code for translating such strings. This couldn't be done before as by the time the configuration translation feature was committed that new interface (an important rework of the translation system) was not there yet.

Moved around some methods among these classes as it better fits with the new services, in order to clean up the dependencies between services, avoiding repeated parameter passing, and providing better encapsulation.

The change in the Config object behaviour consists on setting the 'isNew' flag after it is saved and the event listeners are invoked and not after so listeners subscribing to 'config.save' events can tell when a new configuration is being written and when current configuration is being updated, which is critical for this feature and should have no impact on other modules since that flag was useless before during that event (it was always FALSE).

+++ b/core/lib/Drupal/Core/Config/Config.php
@@ -453,8 +453,9 @@ public function save() {
       $this->load();
     }
     $this->storage->write($this->name, $this->data);
-    $this->isNew = FALSE;
+    $this->overrides = array();
     $this->notify('save');
+    $this->isNew = FALSE;
     return $this;
   }

* Not providing a detailed method list since these functions are only used by locale module atm so they should have no impact anywhere else. These two schemas try to represent how the dependencies between different locale components are currently and how they look like with this patch.

Before


D8MI_Component_Dependencies.png

After


d8mi_locale_config_dependencies02.png
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Issue tags: +Configuration system

Also tag for config system.

Gábor Hojtsy’s picture

Initial solutions for this were in patches in #1905152: Integrate config schema with locale, so shipped configuration is translated from #72 onwards until #91, which rolled back the scope due to the hidden dependencies discovered. It involves adding an event listener to locale for the config save operation and ensuring no race conditions appear.

Gábor Hojtsy’s picture

#1905152: Integrate config schema with locale, so shipped configuration is translated just got committed. Woot! Copying Jose's task list from there that he said should be / can be done with this followup. Depending on how long #1813762: Introduce unified interfaces, use dependency injection for interface translation takes, we can still break some of this out to its own issues if needed, but may be simpler to resolve here:

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

Gábor Hojtsy’s picture

Issue tags: +language-config
Jose Reyero’s picture

Assigned: Unassigned » Jose Reyero

Now other missing pieces are in, jumping back into this one, first step will be to re-roll the original patch taking into account other api changes...

Jose Reyero’s picture

There are some other things we could try fix with this patch, as currently, dependencies among components are a bit messed up. This diagram reflects current dependencies, red line for the one that should be removed, green line for the one to add instead.

Some ideas:
1. LocaleConfigSubscriber and LocaleConfigManager both access config storage to retrieve translated configuration. This causes some duplications like localeConfigName() and getLocaleConfigName(). The solution could be LocaleConfigSubscriber getting the translated configuration from LocaleConfigManager.
2. LocaleConfigManager uses directly StringStorage for translating the configuration. It should be using instead the newly added TranslationManager (that was a result of #1813762) That way, we'd be using string overrides and any other translation method that is enabled for translating configuration strings.
3. This latter one is not that straight forward, as LocaleConfigManager needs some limited writing to locale storage (to add new strings found or associate existing ones with configuration names) and that would need some extensions on the TranslationManager.

So I will be attempting to fix 1. in this patch as I am updating the code, possibly leave 2. and 3. for a better time. This is the current picture:

D8MI_Component_Dependencies.png

Jose Reyero’s picture

Still working on cleaning out these dependencies, so far I've found out LocaleConfigSubscriber cannot just use LocaleConfigManager because when the latter one is actually instantiated (that is every page if it is needed by the Subscriber), there are some missing functions.

We get errors like these, caused when LocaleConfigManager is created, and then it creates (from the constructor) a SchemaDiscovery instance....:

Fatal error: Call to undefined function Drupal\Core\Config\drupal_get_profile() in /var/workspace/drupal8/core/lib/Drupal/Core/Config/InstallStorage.php on line 116

So I'm thinking we either rework how these constructors work (ugly) or we move the 'locale config storage' feature into a new separated class used by LocaleConfigManager and LocaleConfigSubscriber. I'll try the second one first.

Gábor Hojtsy’s picture

Title: Update translation for shipped configuration when configuration changes » Translation is not updated for configuration, when the config changes
Category: task » bug
Priority: Normal » Major

I think we need to consider this a major bug from now on. When you edit your config, the translation for shipped config is not updated. This should be fixed.

Jose Reyero’s picture

Title: Translation is not updated for configuration, when the config changes » Update translation for shipped configuration when configuration changes
Category: bug » task
Priority: Major » Normal

Finally tracked that error to some problem with SchemaDiscovery that is being fixed here, patch #34 includes the fix #1966538: Translation is not updated for configuration, when the config changes

Gábor Hojtsy’s picture

Title: Update translation for shipped configuration when configuration changes » Translation is not updated for configuration, when the config changes
Category: task » bug
Priority: Normal » Major
Gábor Hojtsy’s picture

@Jose: crosspost I guess. Also wrong link? You link to this same issue.

Gábor Hojtsy’s picture

Jose: are you still working on this? It is an important bug in core IMHO.

Jose Reyero’s picture

Yes, I am, I'll post some more patches this week.

Jose Reyero’s picture

Related, will fix the issue with SchemaDiscovery, #2030859: Improve TypedConfigManager

Jose Reyero’s picture

Status: Active » Needs work
FileSize
47.71 KB

This is a pretty rough version of the patch, updated for latest head, includes some important changes for cleaning up the object dependencies:
- LocaleConfigManager not being a TypedConfigManager anymore, doesn't need to, and also encapsulating all the configuration/translation logic.
- New LocaleConfigStorage that encapsulates the access to the configuration data.
- New LocaleConfigTranslation that implements TranslatorInterface
- Moved some logic from locale.bulk.inc into LocaleConfigStorage so update operation don't need the full locale module loaded.
- Better checking for whether a config object is translatable (has langcode, it is english, not changed from default). This saves a lot of unneded checks and should avoid some crashes when attempting to translate low level configuration (like system.modules), which was causing some race conditions.
- LocaleTypedConfig objects not created anymore when not needed (empty configuration sets), this should speed up the whole thing.
These two latter ones should also really improve performance. I'll post an updated version of the picture in #7

Jose Reyero’s picture

Status: Needs work » Needs review
FileSize
54.95 KB

New picture, and moving to "needs review" jus to trigger testbot.

d8mi_locale_config_dependencies02.png

Status: Needs review » Needs work

The last submitted patch, 1966538-locale_update_config-16.patch, failed testing.

Gábor Hojtsy’s picture

Issue tags: +API change, +API clean-up

There are massive API changes proposed here. I asked @Jose Reyero to talk to maintainer to validate the possibility of these changes before working further on this. It may be possible to fix the bug with changes that are not as wide (even if the resulting API is not the best). On the other hand, it is very unlikely that contrib modules rely on this API much yet. It is a very new and pretty obscure API.

Jose Reyero’s picture

Status: Needs work » Needs review
FileSize
46.69 KB

@Gabor,
Yup, you are right, though more than 'massive api changes' I'd call it a 'massive cleanup' of an API that was twisted around itself with no other external dependencies.
Also we are implementing existing APIS instead of creating new ones, like in the case of the TranslatorInterface that is used now for translating configuration too.

Anyway, this is an updated patch with some obvious fixes that still will not pass tests.
(One of the problems I'm stuck with is the imposibility of retrieving translatable languages from the container without using external apis like language_list())

Status: Needs review » Needs work

The last submitted patch, 1966538-locale_update_config-21.patch, failed testing.

Jose Reyero’s picture

Status: Needs work » Needs review
FileSize
43.46 KB

Minor updates -getting rid of some hacks- since this one got committed, #2030859: Improve TypedConfigManager

(Nothing really new to look at, just for the testbot to run again)

Status: Needs review » Needs work

The last submitted patch, 1966538-locale_update_config-22.patch, failed testing.

Jose Reyero’s picture

Status: Needs work » Needs review
FileSize
45.14 KB

Updated patch:
- Fixed multiple issues, funcion names, etc...
- Simplified logic for translatable langcodes, language updates, etc..

Status: Needs review » Needs work

The last submitted patch, 1966538-locale_update_config-24.patch, failed testing.

Jose Reyero’s picture

Still working on this one, got almost all tests passing -will post updated patch- but stuck atm with date formats. There are some issues with date formats that make them not-consistent with the rest of localization and we need to fix it somehow:
- Date formats localization is handled by system module, shouldn't it be in locale?
- Should it respect the setting 'locale_translate_english' like the rest of Drupal?
- Still worse, when a localized date format is updated, locale's config updating will clash with the system module (that writes the localized objects straight to 'locale.config.xx...') so they will overwrite each other's config. It looks like we may need to code some specific handling for date formats in LocaleConfigManager.. ?

I don't know whether there's any related issue, will create a new one if not.

Jose Reyero’s picture

About date formats:

- From @Gábor Hojtsy (who's away and cannot post here right now):
"I think better fix date format localization, not work around it again :) The translate English flag would be important to respect IMHO as well."

- I've done some research on the issue tracker, where there were a lot of related issues, and created a new (meta) issue: #2052193: [META] Date format localisation is a huge mess, conflicts, does not work, regressed

Jose Reyero’s picture

Status: Needs work » Needs review
FileSize
48.26 KB

New version of the patch which addresses some inconsistencies with date formats and locale:
- Add 'langcode' to date formats config when upgrading from D7 so they are localized later.
- Updated some tests enabling 'locale_translate_english'

This may pass tests, but still to do:
- Add specific tests for this feature.
- Specific handling for translated configuration created by other ways than string translations (aka date formats). Otherwise they may get overridden/deleted by locale when updating configuration.
- (May be a different issue). Move 'locale_translate_english' or locale translatable language list into configuration. Not having it in configuration means a few workarounds in this patch like duplicating functionality in locale_translatable_language_list().

Status: Needs review » Needs work

The last submitted patch, 1966538-locale_update_config-28.patch, failed testing.

Jose Reyero’s picture

About the last point (Move 'locale_translate_english' into cmi), I've just posted some other patch that will fix the problems (needed workarounds to access translatable language list) in this issue.

See #2052233: Ensure list of translatable languages is available early in bootstrap

Jose Reyero’s picture

Status: Needs work » Needs review
Jose Reyero’s picture

Added tests for this feature.

Still to do, see #28

Jose Reyero’s picture

Really simplified LocaleConfigManager and LocaleTypedConfig objects:
- Passing a pre-built TranslatorInterface to the wrapper saves a lot of parameters.
- There's also not reason for it to return a TypedConfig object from getTranslation(). Returning a simple array now.

(This may need some more docs cleanup).

Jose Reyero’s picture

Improved patch:
- Fixed: Specific handling for translated configuration created by other ways than string translations (aka date formats). Otherwise they may get overridden/deleted by locale when updating configuration.
- Minor documentation updates.

We can consider the patch complete now, ready for review. Other pending issues that may impact this one, and if fixed will make it cleaner are:
- #2052233: Ensure list of translatable languages is available early in bootstrap
- #2052193: [META] Date format localisation is a huge mess, conflicts, does not work, regressed

webchick’s picture

I can't make sense from the issue summary of what the API impact of this would be for module developers in order to approve it or not. Tagging accordingly.

jair’s picture

Issue tags: +Needs reroll
mtift’s picture

Issue tags: -Needs reroll
FileSize
708 bytes
58.98 KB

Here's a straight re-roll.

This patch would need to change if #1827112: Convert 'install_profile' variable to Settings gets in first.

mtift’s picture

Issue summary: View changes

Updated issue summary.

Gábor Hojtsy’s picture

Issue summary: View changes

Update with headers above images to make it clear

Gábor Hojtsy’s picture

The issue summary was updated nicely earlier, it includes a before/after graph for example for dependencies and classes. Need committer review for approval IMHO before putting more time into it.

pfrenssen’s picture

Issue tags: +Needs reroll

This needs a reroll again. 6 hunks fail to apply.

Gábor Hojtsy’s picture

I think it would be important to figure out whether this is an approach the committers are comfortable with given the scale of API changes proposed. Just pushing in work without discussing it with committers will not make it land.

Gábor Hojtsy’s picture

Issue summary: View changes

Add more visual separation

Gábor Hojtsy’s picture

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

Issue summary: View changes

Typos

Gábor Hojtsy’s picture

While writing tests for #2117711: Add tests to date format translation UI I noticed the LocaleConfigManager cannot be used with config files which are not in installer storage as well. I think that is a problem. Why would we not want to let people access a typed config version of a config that was not shipped with a project. I think this issue is supposed to resolve that, no?

Jose Reyero’s picture

Why would we not want to let people access a typed config version of a config that was not shipped with a project. I think this issue is supposed to resolve that, no?

Well, that depends on whether we want the locale system to jump into custom configuration strings or not, which I think is 'not' for D8 core. And for that we need to compare current config with the shipped config, which won't work if there's no 'shipped config'.

So atm it goes like:
- If String == Default string (shipped version), Then locale can translate it.
- If not, keep hands off...(maybe for some future contrib module).

Hope that helps.

Btw, busy atm with other stuff, I don't think I'll work more on this one. Unassigning the issue.

Jose Reyero’s picture

Assigned: Jose Reyero » Unassigned
Issue summary: View changes
Gábor Hojtsy’s picture

Priority: Major » Critical
Issue tags: +Needs issue summary update

Elevating to critical but based on discussion with Alex Pott, not beta blocker (until proven to need API changes). Will need to start with an issue summary update.

Gábor Hojtsy’s picture

Issue tags: -sprint

Not being worked on actively obviously. Unfortunately.

Jose Reyero’s picture

Looking at the status of this one, revisiting the intended workflow:

Things have changed a little bit since I last worked on this one. We have now a configuration translation module so you can say configuration translations now have a 'life of its own', I mean they can be updated manually independently of locale translations.

Now I think the system doesn't need to -and must not- update translations automatically when the source changes.
Say I have a long string in the configuration and its translation by locale system. Then I change a comma in the source language text... Would I want the translation to be dropped? I think not.

So I think this doesn't make sense anymore, now it's up to site administrators to update configuration translations or not when the source changes, and they have an UI for that.

What we may need instead is some timestamp on the configuration table, to track configuration updates and be able to tell whether a translation may need updating or not. This is a different feature though, and given the time frame possibly we should leave it for contrib. (Though a timestamp on configuration updates would help many other config related workflows, but this is not the place to discuss it anyway).

In summary, I suggest we just close this one or mark it as 'postponed'.

Gábor Hojtsy’s picture

Status: Needs work » Active

Yeah if we consider a change in the source string is not to mean the translation is not anymore applicable, which makes sense then that part of the scope does not fit anymore. There was one more thing in the summary that I just re-read though:

if the structure of configuration changes, they will not properly overlay with the new configuration structure anymore

So if a module changes the config structure then the translation structure is not applicable anymore. Eg. someone edits a view, removes a display and add 2 new fields. The translation still has the items for the display and when the translation is merged, those items will appear in the resulting configuration, which will probably be totally invalid. I think at least when we save a config file, we should check all the translations and remove items that are at nesting levels not present in the original file.

I think that may in itself be a critical bug as well since it will result in a view which is in a totally unexpected structure, it has some random bits of a display that is far from fully defined.

Moving back to active since we'll need to restart this either way... We may want to close this as duplicate and open another issue for this scope though so we have a clean slate.

Jose Reyero’s picture

if the structure of configuration changes, they will not properly overlay with the new configuration structure anymore

Yeah, you're right, I missed that part, but I think this is not really 'translation specific', as it will happend with any kind of overrides. But anyway, since the ConfigFactory is now handling language overrides itself it should handle that updates too, and I'd say it is a critical bug if it doesn't.

I think we should create a new issue and drop this one, to save people working on it some useless reading.

Gábor Hojtsy’s picture

Status: Active » Closed (duplicate)