Skip to content
Snippets Groups Projects

Resolve #2845437 "Process translation config"

Open Resolve #2845437 "Process translation config"
issue/drupal-2845437:2845437-process-translation-config into 11.x
9 unresolved threads
Open Ilya Novikov requested to merge issue/drupal-2845437:2845437-process-translation-config into 11.x
9 unresolved threads

Closes #2845437

Merge request reports

Members who can merge are allowed to add commits.

Merge request pipeline #357645 passed

Stage: 🪄 Lint
Stage: 🗜️ Test
Pipeline: drupal-2845437

#357659

    Merge request pipeline passed for 373d758a

    Code Quality is loading
    Test summary results are being parsed
    Full report
    Ready to merge by members who can write to the target branch.

    Merge details

    • The source branch is 1192 commits behind the target branch.
    • 1 commit will be added to 11.x.
    • Source branch will not be deleted.

    Activity

    Filter activity
    • Approvals
    • Assignees & reviewers
    • Comments (from bots)
    • Comments (from users)
    • Commits & branches
    • Edits
    • Labels
    • Lock status
    • Mentions
    • Merge request status
    • Tracking
    core/lib/Drupal/Core/Config/ConfigInstaller.php
    160 160 $optional_install_path = $extension_path . '/' . InstallStorage::CONFIG_OPTIONAL_DIRECTORY;
    161 $collection_info = $this->configManager->getConfigCollectionInfo();
    161 162 if (is_dir($optional_install_path)) {
    162 163 // Install any optional config the module provides.
    163 164 $storage = new FileStorage($optional_install_path, StorageInterface::DEFAULT_COLLECTION);
    164 $this->installOptionalConfig($storage, '');
    165 foreach ($collection_info->getCollectionNames() as $collection) {
    166 $this->installOptionalConfig($storage, '', $collection);
    167 }
    165 168 }
    166 169 // Install any optional configuration entities whose dependencies can now
    167 170 // be met. This searches all the installed modules config/optional
    168 171 // directories.
    169 172 $storage = new ExtensionInstallStorage($this->getActiveStorages(StorageInterface::DEFAULT_COLLECTION), InstallStorage::CONFIG_OPTIONAL_DIRECTORY, StorageInterface::DEFAULT_COLLECTION, FALSE, $this->installProfile);
    170 $this->installOptionalConfig($storage, [$type => $name]);
    173 $dependency = $name == 'update' ? [] : [$type => $name];
  • Alex Pott @alexpott started a thread on the diff
    • core/lib/Drupal/Core/Config/ConfigInstaller.php
    211 221
    212 222 // Filter the list of configuration to only include configuration that
    213 223 // should be created.
    214 $list = array_filter($list, function ($config_name) use ($existing_config) {
    224 $list = array_filter($list, function ($config_name) use ($existing_config, $collection) {
    225 if ($collection != StorageInterface::DEFAULT_COLLECTION) {
    226 return TRUE;
    227 }
    • Comment on lines +225 to +227
      A
      Alex Pott @alexpott
      Maintainer

      This does not feel correct. We should only install translations for optional configuration that will exist.

      This also requires test additions.

    • Ilya Novikov
      Ilya Novikov @ilya_no
      Author

      Thanks for the catch! I've updated logic and added comments above new lines of code. I've decided to keep this change, but add code for the part, where missing dependencies are checked. For configurations from non-default collection I replace $data with data from configuration, loaded from default collection, since it always has full data, while non-default collection's configuration usually contains only translations.

      I've also updated test - added configuration with missing dependency and assertion, that it has empty data after module's configuration is installed.

    • Please register or sign in to reply
  • Ilya Novikov added 197 commits

    added 197 commits

    Compare with previous version

  • Ilya Novikov added 90 commits

    added 90 commits

    Compare with previous version

  • Ilya Novikov added 1 commit

    added 1 commit

    • ef7015ed - Process translation config files for custom modules

    Compare with previous version

  • Lee Rowlands
    Lee Rowlands @larowlan started a thread on the diff
    • core/lib/Drupal/Core/Config/ConfigInstaller.php
    158 158 $profile_installed = in_array($this->drupalGetProfile(), $this->getEnabledExtensions(), TRUE);
    159 159 if (!$this->isSyncing() && (!InstallerKernel::installationAttempted() || $profile_installed)) {
    160 160 $optional_install_path = $extension_path . '/' . InstallStorage::CONFIG_OPTIONAL_DIRECTORY;
    161 $collection_info = $this->configManager->getConfigCollectionInfo();
    161 162 if (is_dir($optional_install_path)) {
    162 163 // Install any optional config the module provides.
    163 164 $storage = new FileStorage($optional_install_path, StorageInterface::DEFAULT_COLLECTION);
    164 $this->installOptionalConfig($storage, '');
    165 foreach ($collection_info->getCollectionNames() as $collection) {
    166 $this->installOptionalConfig($storage, '', $collection);
    • Lee Rowlands
      Lee Rowlands @larowlan
      Maintainer

      Ugh - the second param here is type-hinted as an array, so this is jarring to see a string passed. Can we get a follow-up issue created to fix that as it's out of scope here.

      I think we should use a named argument here, i.e.

      $this->installOptionalConfig($storage, collection: $collection)

      That way we fix at least one instance of passing a string when an array is expected (there are others though, hence the follow-up)

    • Please register or sign in to reply
  • Lee Rowlands
    Lee Rowlands @larowlan started a thread on the diff
    • core/lib/Drupal/Core/Config/ConfigInstaller.php
    177 182 /**
    178 183 * {@inheritdoc}
    179 184 */
    180 public function installOptionalConfig(?StorageInterface $storage = NULL, $dependency = []) {
    185 public function installOptionalConfig(?StorageInterface $storage = NULL, $dependency = [], $collection = StorageInterface::DEFAULT_COLLECTION) {
  • Lee Rowlands
    Lee Rowlands @larowlan started a thread on the diff
    • core/lib/Drupal/Core/Config/ConfigInstaller.php
    177 182 /**
    178 183 * {@inheritdoc}
    179 184 */
    180 public function installOptionalConfig(?StorageInterface $storage = NULL, $dependency = []) {
    185 public function installOptionalConfig(?StorageInterface $storage = NULL, $dependency = [], $collection = StorageInterface::DEFAULT_COLLECTION) {
    181 186 $profile = $this->drupalGetProfile();
    182 187 $enabled_extensions = $this->getEnabledExtensions();
    183 188 $existing_config = $this->getActiveStorages()->listAll();
    184 189
    190 // Get proper storage for non-default collection.
    191 if (!empty($storage) && $storage->getCollectionName() != $collection) {
    192 $default_storage = $storage;
    193 $storage = $storage->createCollection($collection);
  • Lee Rowlands
    Lee Rowlands @larowlan started a thread on the diff
    • core/lib/Drupal/Core/Config/ConfigInstaller.php
    238 253 }
    239 254
    240 255 foreach ($config_to_create as $config_name => $data) {
    256 // For non-default collection's config load data from default one
    257 // to get proper dependencies.
    258 if (!empty($default_storage) && $collection != StorageInterface::DEFAULT_COLLECTION) {
  • Lee Rowlands
    Lee Rowlands @larowlan started a thread on the diff
    • core/lib/Drupal/Core/ProxyClass/Config/ConfigInstaller.php
    79 79 * {@inheritdoc}
    80 80 */
    81 81 public function installOptionalConfig(?\Drupal\Core\Config\StorageInterface $storage = NULL, $dependency = array (
    82 ))
    82 ), $collection = '')
  • Lee Rowlands
    Lee Rowlands @larowlan started a thread on the diff
    • core/modules/config_translation/tests/src/Kernel/ConfigTranslationTest.php 0 → 100644
    57 'locales_source',
    58 'locales_target',
    59 'locales_location',
    60 ];
    61 $this->installSchema('locale', $locale_tables);
    62 \Drupal::service('theme_installer')->install(['stark']);
    63 $language = ConfigurableLanguage::createFromLangcode('fr');
    64 $language->save();
    65 \Drupal::service('module_installer')->install(['config_install_optional_test']);
    66 $this->installConfig(['config_install_optional_test']);
    67 // Check, if block 'test_translate' has proper translation.
    68 $config_translation = \Drupal::languageManager()->getLanguageConfigOverride('fr', 'block.block.test_translate');
    69 $this->assertTrue($config_translation->get('settings.label') == 'Title (fr)');
    70 // Check, if block with missing dependency has empty config.
    71 $config_translation = \Drupal::languageManager()->getLanguageConfigOverride('fr', 'block.block.test_translate_unmet');
    72 $this->assertTrue(empty($config_translation->getRawData()));
  • Lee Rowlands
    Lee Rowlands @larowlan started a thread on the diff
    • core/modules/locale/src/LocaleConfigManager.php
    630 630 protected function filterOverride(array $override_data, array $translatable) {
    631 631 $filtered_data = [];
    632 632 foreach ($override_data as $key => $value) {
    633 if (isset($translatable[$key])) {
    633 if (isset($translatable[$key]) && is_array($value)) {
    634 634 // If the translatable default configuration has this key, look further
    635 // for subkeys or ignore this element for scalar values.
    636 if (is_array($value)) {
    637 $value = $this->filterOverride($value, $translatable[$key]);
    638 if (!empty($value)) {
    639 $filtered_data[$key] = $value;
    640 }
    635 // for subkeys.
    636 $value = $this->filterOverride($value, $translatable[$key]);
    637 if (!empty($value)) {
  • Mithun S added 1 commit

    added 1 commit

    • 373d758a - 2845437: Add strict comparision for conditioning statement.

    Compare with previous version

    • Please register or sign in to reply