Problem/Motivation

It's possible to define a custom content type by placing necessary files into a custom module's directory structure under config/[install | optional]. One might also want that content type to be translatable and may have even gone through the work to define some of the translations. If those translations are placed in config/[install | optional]/language/[language-code], then they should be pulled into the system when the module is enabled. Currently it's not the case, because for `install` folder system ignores scalar values and processes only array values and for the `optional` folder system installs config only in default collection, ignoring all other available collections.

Proposed resolution

Process translation files correctly, when module is installed separately or along with profile installation, by processing all values in configs and taking all available collections for optional configs.

Remaining tasks

Review the patch, give feedback and fix the patch,

API changes

Within current patch function installOptionalConfig() receives 3rd parameter with collection name.

CommentFileSizeAuthor
#70 2845437-nr-bot.txt90 bytesneeds-review-queue-bot
#66 interdiff-63-66.txt8.92 KBilya.no
#66 2845437-66.patch18.11 KBilya.no
#63 interdiff-61-63.txt4.41 KBilya.no
#63 2845437-63.patch17.65 KBilya.no
#61 interdiff-60-61.txt865 bytesilya.no
#61 2845437-61.patch11.58 KBilya.no
#60 interdiff-59_60.txt1.11 KBGauravvvv
#60 2845437-60.patch11.52 KBGauravvvv
#59 interdiff-57_59.txt764 bytesGauravvvv
#59 2845437-59.patch11.23 KBGauravvvv
#57 interdiff-55-57.txt5.53 KBilya.no
#57 2845437-57.patch11.15 KBilya.no
#55 diff-51-55.txt1.09 KBsorlov
#55 2845437-55.patch5.23 KBsorlov
#51 interdiff_2845437_49-51.txt812 bytesankithashetty
#51 2845437-51.patch5.25 KBankithashetty
#49 interdiff_2845437_47-49.txt2.82 KBankithashetty
#49 2845437-49.patch5.13 KBankithashetty
#47 Screen Shot 2023-01-18 at 10.43.53 PM.png240.53 KBankithashetty
#47 Screen Shot 2023-01-19 at 10.16.14 AM.png364.53 KBankithashetty
#47 interdiff_2845437_45-47.txt521 bytesankithashetty
#47 2845437-47.patch2.13 KBankithashetty
#45 interdiff_2845437_43-45.txt1.36 KBankithashetty
#45 2845437-45.patch1.45 KBankithashetty
#43 2845437-43.patch1.04 KBankithashetty
#32 2845437-32.patch1.2 KBdxvargas

Issue fork drupal-2845437

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vegantriathlete created an issue. See original summary.

Gábor Hojtsy’s picture

Thanks! Just tagging up as a start, will flesh this out a bit later.

vegantriathlete’s picture

Issue summary: View changes
vegantriathlete’s picture

Steps to reproduce:

Setup

You must have the Language module enabled, as well as Content Translation. You must also have some language installed.

Actions

  1. Create a new content type through the web interface, ensuring that it is set to be translatable (Language Settings have Enable translation checked).
  2. Add fields to the content type.
  3. Choose to translate the content type and translate the name.
  4. Manage the fields and translate them.
  5. Export the site configuration.
  6. Delete the content type.
  7. Create a custom module (including its .info.yml file) that places the necessary files inside of config/install/optional and config/install/optional/language/[language-code]
  8. Enable the custom module

Expected behavior

You see the new content type, including all its fields and the translated values.

Current behavior

You see the new content type, including all its fields. However none of the translations appear. If you go to a Translate area, you just see the installed language with the Add button.

vegantriathlete’s picture

Title: Process translation config files » Process translation config files for custom modules
vegantriathlete’s picture

Issue summary: View changes
vegantriathlete’s picture

I think we've got the same issue with things like tour.tour.some-tour.yml. Each of those YAML files has the language key. It makes sense that translations of tours should go inside of config/[install | optional]/lanuage/[language-code] as well.

Gábor Hojtsy’s picture

Just found #2831126: Translation strings from install profile config language/[langcode]/*.yml files don't appear in locale_target which seems to be a duplicate of this one. We should decide to close that one or this one.

Gábor Hojtsy’s picture

Closed that one. Bringing over this comment from @andypost there:

Steps to reproduce
1) create install profile with some additional language (ru)
2) add language/ru dir with some translated config
3) use drush to install site (default en locale used)
4) config overrides from language/ru are not imported

This because $override is not new in http://cgit.drupalcode.org/drupal/tree/core/modules/locale/src/LocaleCon... so it deleted

So according to that the config overrides may in fact be imported but then deleted because they don't end up in locales target.

Gábor Hojtsy’s picture

Issue tags: +SprintWeekend2017, +sprint
andypost’s picture

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

ckaotik’s picture

As an ugly workaround, we've been putting optional configuration's translations into config/install/language/{langcode}, but that of course creates other problems.
Update: Configuration translation in config/optional works if instead of a directory structure you preprend the collection to the config name, e.g. confg/optional/language.{langcode}.{configuration name}.

After some digging around, it seems to me this might be located in ConfigInstaller:installDefaultConfig(). Files in the config/install directory get special collection handling (e.g for language.{langcode})

    // Gets profile storages to search for overrides if necessary.
    $profile_storages = $this->getProfileStorages($name);

    // Gather information about all the supported collections.
    $collection_info = $this->configManager->getConfigCollectionInfo();
    foreach ($collection_info->getCollectionNames() as $collection) {
      $config_to_create = $this->getConfigToCreate($storage, $collection, $prefix, $profile_storages);
      // If we're installing a profile ensure configuration that is overriding
      // is excluded.
      if ($name == $this->drupalGetProfile()) {
        $existing_configuration = $this->getActiveStorages($collection)->listAll();
        $config_to_create = array_diff_key($config_to_create, array_flip($existing_configuration));
      }
      if (!empty($config_to_create)) {
        $this->createConfiguration($collection, $config_to_create);
      }
    }

while optional config gets handled separately using ConfigInstaller::installOptionalConfig()

   if (is_dir($optional_install_path)) {
      // Install any optional config the module provides.
      $storage = new FileStorage($optional_install_path, StorageInterface::DEFAULT_COLLECTION);
      $this->installOptionalConfig($storage, '');
    }
    // Install any optional configuration entities whose dependencies can now
    // be met. This searches all the installed modules config/optional
    // directories.
    $storage = new ExtensionInstallStorage($this->getActiveStorages(StorageInterface::DEFAULT_COLLECTION), InstallStorage::CONFIG_OPTIONAL_DIRECTORY, StorageInterface::DEFAULT_COLLECTION, FALSE);
    $this->installOptionalConfig($storage, [$type => $name]);

which then does not check for collections at all: $config_to_create = $storage->readMultiple($list);.

wesleydv’s picture

Cross posting because I originally posted it in the duplicate of this issue
--

We encountered this issue when developing a multilingual distribution.
We copied to code from .profile file in https://www.drupal.org/project/multilingual_demo to overcome it.

We also applied this patch https://www.drupal.org/files/issues/2650434-84.patch from this issue https://www.drupal.org/node/2650434

However we’ve noticed that field label translations kept loosing their translation after we did a locale-update

The only solution we could find is to also add the strings in a .po file that is imported.

I’m not sure if it is related to this issue, if not we can still create a separate issue.
But my colleagues and I spend way too much time on this, to not document it here ;-).

We plan to opensource this distribution, so I will add a new comment as soon as that is done (normally next week) so everybody can see exactly what we did.

andypost’s picture

Version: 8.3.x-dev » 8.5.x-dev

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

aralnoth’s picture

A possible workaround when you have some override locales in 'install/language/{langcode}' folders is to add an update function in the module that has the files with the following line:

\Drupal::service('language.config_factory_override')->installLanguageOverrides( '{langcode}' );

This fixes the problem because as a commented here, the locale-update command no works. I’m not sure if it is related to this issue but I leave this possible solution here in case it is helpful to someone else.

mpp’s picture

When providing translations in a module's config/install directory we encounter an error during config import on install.

See #2955457

alexpott’s picture

Priority: Normal » Major

This feels like a major bug.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

iampuma’s picture

Currently we are having the same issue with our multilingual profile installation. An additional language module is provided that enables the language modules and contains translations configuration. These are however not imported.

spiderman’s picture

I've been tracking this bug for awhile now, trying to get to the bottom if it- I've attempted most of the fixes mentioned on this ticket and several others. I've managed to confirm (in my debugger) that Gábor's comment in #9 appears to be correct, and none of the patches/fixes mentioned here or on the (apparently duplicate) issue #2831126 seem to fix it. After quite a bit of digging on this, I'm fairly stumped!

Note that #2831126 might have been different in the sense that it relied on an install profile, whereas the original Steps to reproduce just describe enabling a custom module. In my testing, it seems that the custom modules' config translation gets installed/updated properly if I "manually" enable the relevant modules *after* the site is already installed. So it appears to be something about the installer itself, or at least the way the installer handles custom profiles. I'm unclear how much of a role Features module plays in this difference, at this point, or whether that's a clue to the fix. (see notes at the end of my comment re: test setup)

Tracing the install.php process in my debugger, I can see exactly this section of code from LocaleConfigManager::updateConfigTranslations(), right at the end of the installer, in a step called Finish Translations.

As described in the comments of that function, this is called after the initial update of interface translation strings from l.d.o. What I don't yet understand is why the config overrides seem to be deleted, after earlier being (apparently) installed properly.

The key section if that function is this loop:

      foreach ($langcodes as $langcode) {
        $processed = $this->processTranslatableData($name, $active, $translatable, $langcode);
        // If the language code is not the same as the active storage
        // language, we should update the configuration override.
        if ($langcode != $active_langcode) {
          $override = $this->languageManager->getLanguageConfigOverride($langcode, $name);
          // Filter out locale managed configuration keys so that translations
          // removed from Locale will be reflected in the config override.
          $data = $this->filterOverride($override->get(), $translatable);
          if (!empty($processed)) {
            // Merge in the Locale managed translations with existing data.
            $data = NestedArray::mergeDeepArray([$data, $processed], TRUE);
          }
          if (empty($data) && !$override->isNew()) {
            // The configuration override contains Locale overrides that no
            // longer exist.
            $this->deleteTranslationOverride($name, $langcode);
            $count++;
          }
          elseif (!empty($data)) {
            // Update translation data in configuration override.
            $this->saveTranslationOverride($name, $langcode, $data);
            $count++;
          }
          ... elseif that doesn't seem to apply here

As Gábor noted, the $this->deleteTranslationOverride() call happens for these custom config translation overrides, which removes the previously installed (apparently because they haven't yet made it to locale_target?). However, I'm wondering if perhaps the problem arises because the $this->saveTranslationOverride() is never called, since $data is empty.

This seems to be the result of the $this->filterOverride() call above, which is where I get lost. I'm not sufficiently savvy about the CMI and Multilingual components of core here (yet!)

I'd love to write a patch for this, but I'm having trouble understanding the internal logic of the key functions, so I'm hoping someone can help shed some light on it. :)

My test setup

To be clear, the test setup I have to reproduce this is like this:

  • custom install profile, pared down to bare minimum "multilingual" setup over core Drupal
  • hook_install_tasks() implementation that batch installs a couple of custom modules from within the profile ([webroot]/profiles/custom/[translate-profile]/modules/)
  • the two Features modules to create and install custom translated *configuration* strings from [webroot]/profiles/custom/[custom-translate-profile]/modules/*/config/install/language/[langcode]/*.yml
    1. first one has config for the language entities (en/fr, in my case), locale.settings, language.negotiation, etc.
    2. second one creates a content type entity with one text field, associated language/translation settings, and config translation overrides for the entity and field.
spiderman’s picture

Quick update: In the interest of making it easy to see exactly what I've done, I've posted the minimal test setup codebase here:

https://www.drupal.org/sandbox/spiderman/3018656

I'll also note that in my digging on this, I noticed this closed issue: https://www.drupal.org/project/drupal/issues/1905152

which seems to be where the LocaleConfigManager code was initially introduced (I don't think the patch committed there actually included this bug, mind you)- just in case that's helpful to someone who knows more of the history of this stuff to jog our collective memory :)

geek-merlin’s picture

This really needs an IS update with a thorough definition of the problem.

From the above comments i get the feeling we have 2 problems. As i understand it:

* Manually enabling MYMODULE, language overrides are picked up from MYMODULE/config/install/language/foo/some.config.yml: WORKS
* Manually enabling MYMODULE, language overrides are picked up from MYMODULE/config/optional/language/foo/some.config.yml: DoesNotWork(?)
* Installing a profile that enables MYMODULE, language overrides are picked up from MYMODULE/config/install/language/foo/some.config.yml: DoesNotWork
* Installing a profile that enables MYMODULE, language overrides are picked up from MYMODULE/config/optional/language/foo/some.config.yml: DoesNotWork

So if this is correct, we have 2 issues
* Language overrides from config/optional are not picked up
* Language overrides are not picked up (or picked up and instantly deleted) if installing a profile

geek-merlin’s picture

To document it, i tested the first claim above
* Manually enabling MYMODULE, language overrides are picked up from MYMODULE/config/install/language/foo/some.config.yml: WORKS

like so:

* On a clean install
* $ drush en language
* Add language de
* $ echo "$settings['extension_discovery_scan_tests'] = TRUE;" >> sites/default/settings.php # So we can enable test modules
* $ drush en config_test
* $ drush cex

Now we can verify that a german language override is installed in language/de/config_test.system.yml

geek-merlin’s picture

Addressing the install profile issue mentioned in #22, my gut feeling is this: If for some reason the schema is not yet picked up, there is no metadate for translatable properties, and the code fragment

$data = $this->filterOverride($override->get(), $translatable);

surely will return empty. Just a gut feeling.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joelstein’s picture

I can confirm that translatable configuration in mymodule/config/install/language/LANGCODE/mymodule.yml is not being installed when installing Drupal with a config profile.

This little workaround in mymodule.install seems to at least get this exported configuration installed:

/**
 * Implements hook_install().
 */
function mymodule_install() {
  \Drupal::service('language.config_factory_override')->installLanguageOverrides('es');
}

EDIT: Actually, sorry, that didn't work. Probably for the same reason that @alex.rutz mentioned in #26... but I don't know for sure.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

CKIDOW’s picture

Edit: I was totally wrong.

dxvargas’s picture

Status: Active » Needs work
FileSize
1.2 KB

Usually locale (aka interface aka string) translations and active configuration translations are up to date.
But when installing the site, we have the special case where the config can be translated but there are no updates to the locale translations. As mentioned in #22, translations don't get into locale_target and this will later cause the removal of the configuration translations in $this->deleteTranslationOverride() inside the loop.

The solution I'm proposing is to also update the locale translations when installing the site, by removing the check to InstallerKernel::installationAttempted(). Please see the attached patch.
There is also the need to get the langcode in the event, not sure why it's not there. I suppose this should be fixed when the event is fired but in the patch, for simplicity, I get it from the collection name.

Finally, I found this problem when installing the site with the --existing-config option, having the translations in the sync config. Patch fixes the problem with this installation process. I've also tested the patch with steps given in #9 with success.

ckaotik’s picture

Thanks @dxvargas for the patch. This gets the config/install translation overrides imported, however on a mono-lingual non-English site (in my case: de) things break.

  • default en config values are installed, but incorrectly labeled with a de langcode 😱
  • a language de config override is installed and used

The user is then always confronted with the translated config (de), but the config forms always display the original en values. Fun fact: This is without using the config_translation module, so it makes no sense to the unsuspecting user.

I think there was another issue somewhere, that was talking about the automatism on non-English monolingual sites. There, the appropriate translation (if it exists) was supposed to be merged into active configuration.
Edit: Found some issues about that: #2806009: Installing a module causes translations to be overwritten, #2905295: Configuration language being overwritten during module install, #2910353: Prevent saving config entities when configuration overrides are applied, #2925203: LocaleConfigSubscriber can result in data loss during install. So my comment here is probably only partially related.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

mxh’s picture

Is there any guide out there regards how to create multilingual installation profiles? How do other distributions handle the import of config translations - only via .po files? If config translations are not supposed to be included in installation profiles, I think that this documentation needs some more love.

Edit: It seems most distributions & modules handle it with .po translation files. My use case was a site with a configuration set that used to be imported via drush cim, but for reusage purposes I wanted to convert this set into a installation profile. The problem was that there were a lot of translated configurations, and these were not imported properly during site installation process. For such a situation I've found two possible ways (and both ways only seem to be workarounds to me):

1. Create a custom translation (.po) file out of all existing translations. Luckily, after performing a drush cim of that given configuration, I could run a locale:export de --types=customized > de.po and could use that file as translation file placed in myprofile/translations/de. During site installation and after enabling the corresponding language, the configuration translations were created with given translations from that .po file. Note that you might need to manually import this file to make sure those translations are being used, e.g. via drush locale:import --override=all de /path/to/translation.de.po. After having that translation file, I could delete all translated config files (i.e. deleting the whole config/install/language/de folder).

2. As mentioned in #14, the multilingual_demo distribution handles this situation with an installation task. Implement an installation task in your profile that takes care to ensure that the given translation is saved. If you'd only want to take care of the translations to be saved, then have a look at https://git.drupalcode.org/project/multilingual_demo/-/blob/8.x-6.x/mult.... I implemented an installation task that makes sure that every configuration (including non-translations) is being saved consistently to the configuration files of my installation profile. The code for the .profile implementation looks like this:

<?php
/**
 * @file
 * The MyInstallProfile profile file.
 */

use \Drupal\Core\Config\FileStorage;

/**
 * Implements hook_install_tasks().
 */
function myinstallprofile_install_tasks(&$install_state) {
  return ['_myinstallprofile_reimport_configuration' => []];
}

/**
 * Implements hook_install_tasks_alter().
 */
function myinstallprofile_install_tasks_alter(&$tasks, $install_state) {
  if (!isset($tasks['_myinstallprofile_reimport_configuration'])) {
    return;
  }

  // Ensure the config import is run as the last task step.
  $task = $tasks['_myinstallprofile_reimport_configuration'];
  unset($tasks['_myinstallprofile_reimport_configuration']);
  $tasks += ['_myinstallprofile_reimport_configuration' => $task];
}

/**
 * Reimports all configuration.
 *
 * Ensures that profile configuration is not
 * changed by module installation defaults.
 */
function _myinstallprofile_reimport_configuration(&$install_state) {
  $config_directory = __DIR__ . '/config/install';
  $file_storage = new FileStorage($config_directory);
  $config_factory = \Drupal::configFactory();

  foreach ($file_storage->readMultiple($file_storage->listAll()) as $name => $data) {
    if (empty($data)) {
      continue;
    }

    $config = $config_factory->getEditable($name);
    if (!$config->isNew()) {
        $data['uuid'] = $config->get('uuid');
        $data['_core'] = $config->get('_core');
    }
    $config->setData($data);
    $config->save();
  }
}
?>

For modules, the above example function _myinstallprofile_reimport_configuration could be used within the hook_install implementation instead I guess.

If it was only to solve the translation problem, I'd choose option 1. For my use case I actually use both options, because I did not want to have modules override configurations during installation process.

Maybe there is still a catch on this, but if others agree with it I could add such proposals to the above mentioned documentation page. Nevertheless, both options don't feel to be real solutions for this problem.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

tijsdeboeck’s picture

I trying to get custom config translations to work on custom module, in a non-English monolingual site, and I can confirm that the behaviour described by @ckaotik in #33 still exists in Drupal 9.3.

dxvargas’s picture

@tijsdeboeck can you specify if the problem you have in your non-English monolingual site occurs after or before applying the patch #32? Or it's the same before and after?
Are you sure that problem is the same being described in the description of this issue? Sorry to ask but it seems to me we are diverting into another issue, different from the initial one.

tijsdeboeck’s picture

@dxvargas, sorry, I am indeed mixing a couple of things indeed. I didn't install patch #32, when I had the issue... Please ignore my previous comment.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

ankithashetty’s picture

Status: Needs work » Needs review
FileSize
1.04 KB

Uploading a new patch with a change that might fix the config overrides issue for a different language during fresh installation.

The reason some configs were removed/deleted was the scalar elements in a config were ignored.

These changes were introduced in #2212069: Non-English Drupal sites get default configuration in English, edited in English, originals not actually used if translated. Any idea why we don't consider scalar values while importing a config? I don't see any reason here, if anyone could help here, that would be great.🙌🏼

Many related issues I came across:

  1. https://www.drupal.org/project/drupal/issues/2831126
  2. https://www.drupal.org/project/drupal/issues/2806009

Status: Needs review » Needs work

The last submitted patch, 43: 2845437-43.patch, failed testing. View results

ankithashetty’s picture

Status: Needs work » Needs review
FileSize
1.45 KB
1.36 KB

Uploading a new patch to avoid repetitive lines in #43.

Thanks!

Status: Needs review » Needs work

The last submitted patch, 45: 2845437-45.patch, failed testing. View results

ankithashetty’s picture

Adding a new patch by fixing PHPUnit test errors in core/modules/locale/tests/src/Kernel/LocaleConfigSubscriberForeignTest.php.

Changes made:
As we are deleting the locale translation, won't change the active/override config. Hence we are supposed to assertNoTranslation after deleteLocaleTranslationData as per my understanding. Which is similar to assertTranslation after saveLocaleTranslationData in testEnglish() function.

Before fix:

After fix:

Status: Needs review » Needs work

The last submitted patch, 47: 2845437-47.patch, failed testing. View results

ankithashetty’s picture

Uploading a patch by fixing PHPUnit test errors in core/modules/locale/tests/src/Functional/LocaleConfigTranslationImportTest.php along with core/modules/locale/tests/src/Kernel/LocaleConfigSubscriberForeignTest.php.

There is a contradiction between both test files actually. Like testLocaleDeleteActiveTranslation() function in LocaleConfigSubscriberForeignTest.php

Assuming that making any change to local translations, doesn't affect the active/override config translations, have updated the patch accordingly.

Requesting a review from maintainers.

Thanks!

Status: Needs review » Needs work

The last submitted patch, 49: 2845437-49.patch, failed testing. View results

ankithashetty’s picture

Status: Needs work » Needs review
FileSize
5.25 KB
812 bytes

Test fixes.

smustgrave’s picture

Status: Needs review » Needs work

Moving to NW for the issue summary update. Specifically what's the proposed solution for this?

ankithashetty’s picture

Let's take a simple example:
We have a module called test, which gets installed during profile installation. It has a configuration for both default language (EN) and another language (for ex: AR)

test/config/install/test.settings.yml => EN version

title: 'Contact us'
// 'title' and 'description' is a scalar value.
settings:
    confirmation_message: 'Thanks for contacting us!'
    ....
// 'settings' is an array.

test/config/install/language/ar/test.settings.yml => AR version

title: 'اتصل بنا'
settings:
  confirmation_message: 'شكرا لإتصالك بنا. سنقوم بالرد عليك في أقرب وقت ممكن!'
    ....

During profile installation, when the test is getting installed, the config translations are also imported. But noticed that the scalar values of the config (i.e., title in the above example) are skipped and only array values are considered (like settings)
So after installation, try doing drsuh cget test.settings for both language, you would notice:

  • Both 'title' and 'settings' in EN version (works fine)
  • Only 'settings' in AR version (issue here)

Code for ref: https://git.drupalcode.org/project/drupal/-/blob/10.1.x/core/modules/locale/src/LocaleConfigManager.php#L636

Proposed solution (just a suggestion):
With the patch in #51, we are trying to process all the config values ('array' and 'string').

As I raised in #43, I still don't get why 'scalar' values are ignored.
I might be missing wrong here, but the issue is valid🤔

Thanks!

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

sorlov’s picture

Status: Needs work » Needs review
FileSize
5.23 KB
1.09 KB

Fixed code check issues on 11.x

FILE: ...ocale/tests/src/Functional/LocaleConfigTranslationImportTest.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 198 | ERROR | Doc comment short description must be on a single
     |       | line, further text should be a separate paragraph
     |       | (Drupal.Commenting.DocComment.ShortSingleLine)
 250 | ERROR | Doc comment short description must be on a single
     |       | line, further text should be a separate paragraph
     |       | (Drupal.Commenting.DocComment.ShortSingleLine)
----------------------------------------------------------------------
smustgrave’s picture

Status: Needs review » Needs work

#53 made some suggestions that I think may be worth responding.

Issue summary still needs to be updated.

ilya.no’s picture

Status: Needs work » Needs review
FileSize
11.15 KB
5.53 KB

I'm not sure in what way should we update summary, because for me it looks clear. Should we just add text from comment #24?

I'm attaching draft patch, which fixes the case, when we install module on already existing site. So current patch fixes 3/4 cases. Need to fix the case, when module is enabled during site installation.

I've noticed, that ConfigInstaller manages all available collections, when installs default config, but when it comes to optional config, then only default collection is used. So, I've added code to use all available collections.

smustgrave’s picture

Status: Needs review » Needs work

CC failure in #57

Issue summary states the problem but doesn't clearly states the solution to the problem.

Is this changing the API or Data model? If not could leave those sections blank or put NA

Gauravvvv’s picture

Fixed CC failure. Attached interdiff for same.
Leaving NW for summary update.

Gauravvvv’s picture

Fixed failures, attached interdiff for same

ilya.no’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
11.58 KB
865 bytes

Attaching the patch which fixes all 4 cases for me. I test with custom module, which has settings config in install folder and view config in optional folder. For both cases there are translations in language/fr folder.
Also updated summary.

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: -sprint, -Needs issue summary update +git sprint 8

Issue summary updated in #61

think we need a test case

If those translations are placed in config/[install | optional]/language/[language-code], then they should be pulled into the system when the module is enabled

From what I can tell the current test changes don't cover that.

ilya.no’s picture

Status: Needs work » Needs review
FileSize
17.65 KB
4.41 KB

Attaching patch with test. Not sure, what is the best place for it, so I've chosen locale module, because I haven't found tests for `ConfigInstaller`.

needs-review-queue-bot’s picture

Status: Needs review » Needs work

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

andypost’s picture

ilya.no’s picture

Status: Needs work » Needs review
FileSize
18.11 KB
8.92 KB

I've found, that my new test module's name is already occupied, so I've renamed and relocated it from locale to config_translation module.

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: -git sprint 8

New tests should have typehints and returns

May need a CR for installOptionalConfig, just to announce the new parameter.

Can this be converted to an MR for quicker/easier reviews.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
FileSize
90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

sorlov’s picture

Status: Needs work » Needs review

Rebased fork branch and rerolled patch. Need review

andypost’s picture

Issue tags: +Needs change record

Not sure the new method is the best solution but it will need CR anyway

smustgrave’s picture

Status: Needs review » Needs work

For the CR