Language module vs. content translation module interaction exposes content translation bug

Well, the bug is due to this hook, which rebuilds the config IF language module was not among the ones enabled, and resets it to defaults (updateConfiguration() checks for locked state and resets the locked ones to where they should be). When you enable language module with content translation, this does not run and the modified config prevails. Not a systemic bug and has nothing to do with config or module installer.

/**
 * Implements hook_modules_installed().
 */
function language_modules_installed($modules) {
  if (!in_array('language', $modules)) {
    $negotiator = \Drupal::service('language_negotiator');
    $negotiator->updateConfiguration(array());
    $negotiator->purgeConfiguration();
  }
  // ...
}

Starting with throwing a plain exception for now where it should not actually save this config. Should introduce a custom exception for this but this is a first step.

So LanguageNegotiator::saveConfiguration() is used as an API to persist locked settings too for easier lookup later. Let's not introduce unneeded API changes then and just fix the cause of this bug in content_translation_install(). Even if we would throw exceptions in saveConfiguration() it is still perfectly possible to update the config with the config API itself in whatever way. So there is no real foolproof way to protect that. We should just not do silly things ourselves at least.

Not sure the fixes in #10 are correct? There are changes to many asserts that now assert the opposite compared to what they used to be. The patch should not make that happen, no?

So I see some tests were assuming that the URL method was set for language negotiation. What about fixing what is asserted then? I see you did that fix in the path language test, so why not do that in the block language test too? Unless we loose some important tested feature?

+++ b/core/modules/path/src/Tests/PathLanguageTest.php
@@ -150,7 +150,7 @@ function testAliasTranslation() {
+    $this->assertText($french_node->body->value, 'English alias, french preffered, french node.' . $french_node->body->value);

@@ -162,7 +162,7 @@ function testAliasTranslation() {
+    $this->assertText($french_node->body->value, 'English alias, french preffered, french node.');

french => French

Also let's remove the added debug string.

In two tests I used the same fix.

@likin: if you are fixing the two tests the same way then I must be terribly mistaken. What I see is:

1. BlockLanguageTest: you updated the content language to be URL based, therefore practically redoing what the install code did that we remove
2. In PathLanguageTest you modified what is being asserted rather than changing the content language setting

Are you doing the same fix in the two tests then? What am I missing?

Yay, looks amazing. Thanks for walking through that with me. Looks like my instinct was very right that we either need to change config or the assertions but definitely not both. I think the changes are fine now, especially the path tests, there are very lengthy comments there as to why the found node should be French, so its strange it was not before. Now the world order is restored.

Updated issue summary.

@likin: no magic involved. Asserting that accessing a French node when using an alias for an English node is confusing, and that is not what the test does. It asserts that it leads to the same node in a different language. Therefore $french_node and $english_node is confusing because they are both the same node. That change is not required to fix the bug but helped a tremendous amount to understand what is being tested and why the fix makes sense.

@likin: no, probably not. The changes you made before or Wim's changes are needed to pass, because those two tests assumed the (broken) configuration that content language had earlier due to its install hook.

Thanks @likin, @Wim Leers!