Interface translations directory: Can not set values on immutable configuration

Looks good. Needs test. The test could be pretty simple, just attempt to set this directory and observe no failure :)

I have checked this issue and now the directory is working well.
Let me know if you need other kind of testing :)

We would need an automated test written. Can you help with that? Just to check that setting the directory changed the configuration.

Sure! I'll be more than happy doing it :)

Still needs tests

+++ b/core/modules/locale/locale.module
@@ -680,7 +680,7 @@ function locale_form_system_file_system_settings_alter(&$form, FormStateInterfac
-    '#default_value' => \Drupal::config('locale.settings')->get('translation.path'),
+    '#default_value' => \Drupal::configFactory()->getEditable('locale.settings')->get('translation.path'),

@@ -705,7 +705,7 @@ function locale_system_file_system_settings_submit(&$form, FormStateInterface $f
-  \Drupal::config('locale.settings')
+  \Drupal::configFactory()->getEditable('locale.settings')

Wondering how did we miss this as part of #2392319: Config objects (but not config entities) should by default be immutable

There does not seem to be test coverage for this page, which is why we should add it here. IMHO

1. drupalGet() this page without locale module, verify the option is not there
2. Install locale module
3. drupalGet() this page, verify the option is there
3. drupalPostForm() a new setting
3. $this->config() assert that the new setting was saved

@mon_franco: how are you doing on this test? Do you need help?

@gábor-hojtsy sorry, I don´t need help, I just need free time to dedicate to this issue :(

The test has been added.

Strange. The error is not invoked on my side.
Value true is TRUE. Other LocaleFileSystemFormTest.php 72 Drupal\locale\Tests\LocaleFileSystemFormTest->testFileConfigurationPage()

Thanks for providing a test, although this issue is assigned to @mon_franco. In cases like this it is important to ask the person assigned if they are working on it (like I did). I think we understood that answer differently. Sorry @mon_franco on behalf of @likin. Reviewing the patch itself:

1. +++ b/core/modules/locale/src/Tests/LocaleFileSystemFormTest.php
   @@ -0,0 +1,75 @@
   + * Definition of Drupal\locale\Tests\LocaleFileSystemForm .
   

   "Contains " ... and should not have a space at the end. And should have the right class name.

2. +++ b/core/modules/locale/src/Tests/LocaleFileSystemFormTest.php
   @@ -0,0 +1,75 @@
   +      'file_temporary_path' => $file_path . '/file_config_page_test/temporary',
   +      'file_default_scheme' => 'private',
   +      'translation_path' => $translation_path
   

   NO need to set all three, just set the one you want to modify.

3. +++ b/core/modules/locale/src/Tests/LocaleFileSystemFormTest.php
   @@ -0,0 +1,75 @@
   +    $config_factory = $this->container->get('config.factory');
   +    $this->assertTrue($translation_path == $config_factory->get('locale.settings')->get('translation.path'));
   

   Use $this->config(), don't bother with the container separately.

These check the same thing by different ways.

It checks if displays the right value. It does.

+    $this->assertFieldByName('translation_path', $translation_path);

Config says that the value has not been changed.

+    $this->assertTrue($translation_path == $this->config('locale.settings')->get('translation.path'));

It seems it uses different containers.

Yeah we need to reset the config factory cache, because the one in the test does not know it changed. That fixes the fail. Also fixing all the doc lines. I think this is good to go but another quick review would be useful :)

+1 to RTBC, Just one question.

+++ b/core/modules/locale/src/Tests/LocaleFileSystemFormTest.php
@@ -0,0 +1,60 @@
+    $this->assertEqual($translation_path, \Drupal::configFactory()->reset()->get('locale.settings')->get('translation.path'));

hmm, do we really need to reset this? as the save was through UI, doesn't it get reflect automatically?

@vijaycs85: this is a webtest and the config factory has a static cache in the class. Nothing would tell it that it needs to invalidate that cache given the config update happened in the simpletest browser in a different request.

cool then. Let's get this in. I guess the issue summary is clear.

The post should refresh the variables. That reset should be unnecessary. See WebTestBase::refreshVariables().

+++ b/core/modules/locale/src/Tests/LocaleFileSystemFormTest.php
@@ -0,0 +1,60 @@
+    $module_installer = $this->container->get('module_installer');
+    $module_installer->install(['locale']);

After doing this you need $this->rebuildContainer() to bring the container in the parent inline with the child.

All right.

+++ b/core/modules/locale/src/Tests/LocaleFileSystemFormTest.php
@@ -43,6 +43,7 @@ function testFileConfigurationPage() {
+    $this->rebuildContainer();

ah, nice. We are good to go for real now :)

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 9044a06 and pushed to 8.0.x. Thanks!

yay, thanks!