Configuration of configurable language types reset after module install

Since this is easily-reproduced data loss (fortunately configuration rather than content though), bumping to critical.

Looked through git blame, if it is language_modules_installed(), the last time it was changed was in 2014, and that was a straight port of previous logic which I didn't track down, so looks like the bug has been here for a long time.

Reading the code, this patch might be enough - untested.

With the test coverage from #2.

I'll have a look to this ASAP...

On this

Lets' try this

Marked #2281211: Content language detection settings gets reverted to default as a duplicate.

I can't say I fully understand that language configuration/purging stuff but the fix seems to make sense, the test looks good and fails as expected.

Let's get back to 0 criticals :)

That's what I wanted to do...

#22 looks right to me.

Fixable on commit:

+++ b/core/modules/system/src/Tests/Module/ConfigLangTypeTest.php
@@ -0,0 +1,69 @@
+   * Tests alteration config of configurable language types.

alteration of

or just altering.

@Berdir:

I can't say I fully understand that language configuration/purging stuff

You are not the only one: #2166879: Clean-up code managing language type configurability ;)

This is a clear case where a straight conversion from D7 was not the right move. Or at least we got it wrong :)

I think the test should be a part of LanguageNegotiationInfoTest

Here

Back to RTBC

I think this was supposed to stay RTBC

Not sure how to please the bot/issue integration.

(@cilefen maybe related to #2598438: Improve logic and/or UI around retesting patches in RTBC issues. hiding previous patches)

was reviewing the interdiff in #34 and it moves the test fine. rtbc++ for that.

but while I was at it, I fixed the nit in #31 and another nit.

I'm not sure if the following questions are enough to block this...
I also read the entire patch, and

1. +++ b/core/modules/language/language.module
   @@ -311,8 +311,13 @@ function language_negotiation_url_domains() {
   +    // Since newly (un)installed modules may change the default settings for
   

   this comment makes it seem like uninstalling could also cause the problem.

   do we need a change in the hook for uninstalling modules? ah, no. because the uninstall hook is a wrapper for the install hook.

   function language_modules_uninstalled($modules) {
     language_modules_installed($modules);
   }

2. +++ b/core/modules/language/language.module
   @@ -311,8 +311,13 @@ function language_negotiation_url_domains() {
   -    $negotiator->updateConfiguration(array());
   +    $configurable = \Drupal::config('language.types')->get('configurable');
   +    $negotiator->updateConfiguration($configurable);
   

   why do we do this here instead of a change in updateConfiguration()

3. +++ b/core/modules/language/src/Tests/LanguageNegotiationInfoTest.php
   @@ -174,4 +174,37 @@ protected function checkFixedLanguageTypes() {
   +    // After installing another module, the config should be the same.
   +    $this->drupalPostForm('admin/modules', ['modules[Core][field][enable]' => 1], t('Install'));
   +    $this->assertTrue($this->isLanguageTypeConfigurable($test_type), 'Language type is still configurable.');
   

   this test uses the field module. but what is it about the field module that makes this test fail. is it that it does not try and make the language configurable?

   I'm wondering if we should have a test module we use that has exactly the conditions we need instead of re-using field.

   Also, I wonder if, as the comment in the fix states, uninstalling a module could have cause the same problem, if we should include that in the test also.

why do we do this here instead of a change in updateConfiguration()

I think we should look at that in a follow-up, my original patch was hoping that would work already (clearly it doesn't), but it'd also be a behaviour change so something we should change in 8.1.x.

Let's add the test coverage for uninstalling a module though - that's the sort of thing that could easily get missed if this code gets refactored later.

added note to the issue summary to
add these people from the older duplicate issue #2281211: Content language detection settings gets reverted to default to the commit message (for opening the issue, and steps to reproduce):
steelsector, matsbla, SiliconMind, vodde83
(I know we can't at this time add them to the database, but we can to the commit message)

@catch just to clarify, are we ok with using the field module in the test vs another test module? Or is that followup material also?

opened the followup @catch asked for in #45. it is postponed on this issue. #2623788: updateConfiguration() should be able to override configurable of language types when modules are installed or uninstalled it probably needs a better title and an update to the issue summary.

@YesCT I think it's probably fine to use field module in the test. It shouldn't matter which module is enabled in the test - just that any module gets enabled. However a comment explaining that would be good.

New module is fine too, just didn't want to block on it.

why do we do this here instead of a change in updateConfiguration()

I think we should look at that in a follow-up, my original patch was hoping that would work already (clearly it doesn't), but it'd also be a behaviour change so something we should change in 8.1.x.

That would be an API change. The PHP docs clearly state the argument should be an array of configurable language type names, so the previous code was plain wrong and the current one should be correct.
Or are you suggesting to make the parameter optional and default to that code?

The interdiff looks good to me.

Yes was thinking optional parameter - but would be 8.1.x change.

Interdiff looks good to me too, so ommitted/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!

Thanks, created #2623906: Make specifing the configurable language types for LanguageNegotiatorInterface::updateConfiguration() optional.