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
Files: 
CommentFileSizeAuthor
#37 1966538-locale_update_config-36.patch58.98 KBmtift
PASSED: [[SimpleTest]]: [MySQL] 57,977 pass(es).
[ View ]
#37 interdiff.txt708 bytesmtift
#34 1966538-locale_update_config-34.patch58.98 KBJose Reyero
PASSED: [[SimpleTest]]: [MySQL] 57,137 pass(es).
[ View ]
#34 1966538-locale_update_config-34-diff.txt7.04 KBJose Reyero
#33 1966538-locale_update_config-33.patch0 bytesJose Reyero
PASSED: [[SimpleTest]]: [MySQL] 57,609 pass(es).
[ View ]
#33 1966538-locale_update_config-33-diff.txt14.43 KBJose Reyero
#32 1966538-locale_update_config-32.patch52.9 KBJose Reyero
PASSED: [[SimpleTest]]: [MySQL] 57,554 pass(es).
[ View ]
#32 1966538-locale_update_config-32-diff.txt4.99 KBJose Reyero
#28 1966538-locale_update_config-28.patch48.26 KBJose Reyero
PASSED: [[SimpleTest]]: [MySQL] 57,394 pass(es).
[ View ]
#24 1966538-locale_update_config-24.patch45.14 KBJose Reyero
FAILED: [[SimpleTest]]: [MySQL] 56,921 pass(es), 7 fail(s), and 0 exception(s).
[ View ]
#22 1966538-locale_update_config-22.patch43.46 KBJose Reyero
FAILED: [[SimpleTest]]: [MySQL] 57,157 pass(es), 10 fail(s), and 4 exception(s).
[ View ]
#20 1966538-locale_update_config-21.patch46.69 KBJose Reyero
FAILED: [[SimpleTest]]: [MySQL] 56,940 pass(es), 13 fail(s), and 23 exception(s).
[ View ]
#17 d8mi_locale_config_dependencies02.png54.95 KBJose Reyero
#16 1966538-locale_update_config-16.patch47.71 KBJose Reyero
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/locale/lib/Drupal/locale/Tests/LocaleImportFunctionalTest.php.
[ View ]

Comments

Issue tags:+Configuration system

Also tag for config system.

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.

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

Issue tags:+language-config

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

StatusFileSize
new48.63 KB

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

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.

Title:Update translation for shipped configuration when configuration changesTranslation 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.

Title:Translation is not updated for configuration, when the config changesUpdate 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

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

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

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

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

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

Status:Active» Needs work
StatusFileSize
new47.71 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/locale/lib/Drupal/locale/Tests/LocaleImportFunctionalTest.php.
[ View ]

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

Status:Needs work» Needs review
StatusFileSize
new54.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.

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.

Status:Needs work» Needs review
StatusFileSize
new46.69 KB
FAILED: [[SimpleTest]]: [MySQL] 56,940 pass(es), 13 fail(s), and 23 exception(s).
[ View ]

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

Status:Needs work» Needs review
StatusFileSize
new43.46 KB
FAILED: [[SimpleTest]]: [MySQL] 57,157 pass(es), 10 fail(s), and 4 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new45.14 KB
FAILED: [[SimpleTest]]: [MySQL] 56,921 pass(es), 7 fail(s), and 0 exception(s).
[ View ]

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.

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.

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

Status:Needs work» Needs review
StatusFileSize
new48.26 KB
PASSED: [[SimpleTest]]: [MySQL] 57,394 pass(es).
[ View ]

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.

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

Status:Needs work» Needs review

StatusFileSize
new4.99 KB
new52.9 KB
PASSED: [[SimpleTest]]: [MySQL] 57,554 pass(es).
[ View ]

Added tests for this feature.

Still to do, see #28

StatusFileSize
new14.43 KB
new0 bytes
PASSED: [[SimpleTest]]: [MySQL] 57,609 pass(es).
[ View ]

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

StatusFileSize
new7.04 KB
new58.98 KB
PASSED: [[SimpleTest]]: [MySQL] 57,137 pass(es).
[ View ]

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

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.

Issue tags:+Needs reroll

Issue tags:-Needs reroll
StatusFileSize
new708 bytes
new58.98 KB
PASSED: [[SimpleTest]]: [MySQL] 57,977 pass(es).
[ View ]

Here's a straight re-roll.

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

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Update with headers above images to make it clear

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.

Issue tags:+Needs reroll

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

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.

Issue summary:View changes

Add more visual separation

Status:Needs review» Needs work

Issue summary:View changes

Typos

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?

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.

Assigned:Jose Reyero» Unassigned
Issue summary:View changes

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.