Steps to reproduce:

  1. Install D8 in default install profile in English
  2. Enable all 4 multilanguage modules
  3. Add a second language (e.g. Dutch)
  4. Goto admin/config/media/file-system and submit settings
  5. bam! -> error (see below)

Remarks:

  • This does only happen for a multilingual site and is triggered by the additional field "Interface translations directory" that appears in admin/config/media/file-system.
  • Settings other than "Interface translations directory" are still saved.

Error message:

<em class="placeholder">Drupal\Core\Config\ImmutableConfigException</em>: Can not set values on immutable configuration locale.settings:translation.path. Use \Drupal\Core\Config\ConfigFactoryInterface::getEditable() to retrieve a mutable configuration object in <em class="placeholder">Drupal\Core\Config\ImmutableConfig-&gt;set()</em> (line <em class="placeholder">34</em> of <em class="placeholder">/Users/albert/Sites/drupal8-dev/core/lib/Drupal/Core/Config/ImmutableConfig.php</em>).
Files: 
CommentFileSizeAuthor
#33 interdiff.txt1.14 KBGábor Hojtsy
#33 locale-settings-immutable-2414279-33.patch3.08 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,094 pass(es). View
#28 interdiff.txt2.58 KBGábor Hojtsy
#28 locale-settings-immutable-2414279-28.patch3.07 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,999 pass(es). View
#26 locale-settings-immutable-2414279-18-25.txt1.47 KBlikin
#26 locale-settings-immutable-2414279-25.patch3.24 KBlikin
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,035 pass(es), 1 fail(s), and 0 exception(s). View
#18 locale-settings-immutable-2414279-3-18.txt2.24 KBlikin
#18 locale-settings-immutable-2414279-18.patch3.46 KBlikin
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,035 pass(es), 1 fail(s), and 0 exception(s). View
#2 locale-settings-immutable-2414279.2.patch1.08 KBaskibinski
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,888 pass(es). View

Comments

askibinski’s picture

This is probably introduced by this change record "Configuration objects by default are immutable.":
https://www.drupal.org/node/2407153

Working on a patch.

askibinski’s picture

FileSize
1.08 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,888 pass(es). View

Here you go!

askibinski’s picture

Status: Active » Needs review

change status

Gábor Hojtsy’s picture

Issue tags: +sprint, +language-ui, +Needs tests

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

Gábor Hojtsy’s picture

mon_franco’s picture

Assigned: Unassigned » mon_franco
mon_franco’s picture

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

Gábor Hojtsy’s picture

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

askibinski’s picture

Thanks for testing :) But Gabor was referencing to an automated test which has to be written in code, see details:
https://www.drupal.org/contributor-tasks/write-tests

mon_franco’s picture

Assigned: mon_franco » Unassigned
mon_franco’s picture

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

mon_franco’s picture

Assigned: Unassigned » mon_franco
rodrigoaguilera’s picture

Status: Needs review » Needs work

Still needs tests

vijaycs85’s picture

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

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

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

mon_franco’s picture

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

likin’s picture

Status: Needs work » Needs review
FileSize
3.46 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,035 pass(es), 1 fail(s), and 0 exception(s). View
2.24 KB

The test has been added.

Status: Needs review » Needs work

The last submitted patch, 18: locale-settings-immutable-2414279-18.patch, failed testing.

The last submitted patch, 18: locale-settings-immutable-2414279-18.patch, failed testing.

likin’s picture

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

Gábor Hojtsy’s picture

Assigned: mon_franco » Unassigned

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.

The last submitted patch, 18: locale-settings-immutable-2414279-18.patch, failed testing.

likin’s picture

Status: Needs work » Needs review
FileSize
3.24 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,035 pass(es), 1 fail(s), and 0 exception(s). View
1.47 KB

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.

Status: Needs review » Needs work

The last submitted patch, 26: locale-settings-immutable-2414279-25.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
3.07 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,999 pass(es). View
2.58 KB

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

vijaycs85’s picture

+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?

Gábor Hojtsy’s picture

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

vijaycs85’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
3.08 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,094 pass(es). View
1.14 KB

All right.

vijaycs85’s picture

Status: Needs review » Reviewed & tested by the community
+++ 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 :)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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!

  • alexpott committed 9044a06 on 8.0.x
    Issue #2414279 by Gábor Hojtsy, likin, askibinski: Interface...
Gábor Hojtsy’s picture

Issue tags: -sprint

yay, thanks!

Status: Fixed » Closed (fixed)

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