Problem/Motivation

LocaleConfigManager (Drupal\\locale\\LocaleConfigManager) manages localized configuration producing some specially wrapped typed configuration data used for translating the configuration through localization system.

It extends atm TypedConfigManager, which means it also reads plug-in information (configuration schema) and produces typed configuration objects instead of reusing plug-in definitions and configuration objects from TypedConfigManager.

This causes some duplication (definitions are read and processed and cached by both managers) while it could very well just reuse the ones in TypedConfiguration Manager, which are the same plugin definitions.

Proposed resolution

Instead of extending it and creating yet another plugin manager, LocaleConfigManager should use the existing TypedConfigManager taking it as a dependency.

Remaining tasks

Review.

User interface changes

None.

API changes

Minimal, internal to locale / configuration translation.
Some LocaleConfigManager methods which were duplicated are replaced by TypedConfigManager ones.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jose Reyero’s picture

Status: Active » Needs review
FileSize
13.06 KB

Posting first patch and running tests.

Gábor Hojtsy’s picture

Makes sense to me. This also allows swapping out the typed data manager without the need to add a custom implementation of the locale manager as well. Not having two layers with the same data cached / managed is useful.

Status: Needs review » Needs work

The last submitted patch, 1: 0829-locale-config-manager-01.patch, failed testing.

Jose Reyero’s picture

Status: Needs work » Needs review
FileSize
2.77 KB
15.82 KB

Updated 2 failing tests, which were not in sync with the constructor changes in the patch.

Jose Reyero’s picture

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +sprint

Looks good to me.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 2b9631a and pushed to 8.x. Thanks!

diff --git a/core/modules/locale/src/LocaleConfigManager.php b/core/modules/locale/src/LocaleConfigManager.php
index feacd0c..07a6f41 100644
--- a/core/modules/locale/src/LocaleConfigManager.php
+++ b/core/modules/locale/src/LocaleConfigManager.php
@@ -7,7 +7,6 @@
 
 namespace Drupal\locale;
 
-use Drupal\Core\Cache\CacheBackendInterface;
 use Drupal\Core\Config\TypedConfigManagerInterface;
 use Drupal\Core\Config\StorageInterface;
 use Drupal\Core\Config\ConfigFactoryInterface;
@@ -84,7 +83,6 @@ class LocaleConfigManager {
    *   The language manager.
    */
   public function __construct(StorageInterface $configStorage, StorageInterface $installStorage, StringStorageInterface $localeStorage, ConfigFactoryInterface $config_factory, TypedConfigManagerInterface $typed_config, ConfigurableLanguageManagerInterface $language_manager) {
-    $this->typedConfigManager = $typed_config;
     $this->configStorage = $configStorage;
     $this->installStorage = $installStorage;
     $this->localeStorage = $localeStorage;

Fixed on commit. Unused use and a duplicate line of code.

  • alexpott committed 2b9631a on 8.x
    Issue #2300829 by Jose Reyero: Fixed Clean up LocaleConfigManager /...
Gábor Hojtsy’s picture

Issue tags: -sprint

Superb, thanks!

Status: Fixed » Closed (fixed)

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