Language negotiator weights should be changeable in settings.php

We ran into this issue today. And I would like to use

$definitions = array_intersect_key($enabled_methods, $definitions);

instead of

$definitions = array_merge($enabled_methods, array_intersect_key($definitions, $enabled_methods));

since array_intersect_key respects the sorting of the first array given.

Also, I wonder, if the configuration from $this->configFactory->get('language.types')->get('negotiation.' . $type . '.enabled') is not already sorted correctly - but I am not sure.

@smustgrave : I added a comment for #17 2) .

+++ b/core/modules/language/src/LanguageNegotiator.php
@@ -197,7 +200,7 @@ public function getNegotiationMethods($type = NULL) {
+      $definitions = array_merge($enabled_methods, array_intersect_key($definitions, $enabled_methods));

Concerning #21 : it is correct, that the test does not test this config overwrite from settings.php. It is more important to test the sorting of the methods, which is already implemented.

@simonbaese : the configuration form does not sort the configuration overwrite from settings.php, they are just appended without sorting before getEnabledNegotiators(). getNegotiationMethods() in #26 does not return the method definitions, it only returns the weights. The method definitions are required in LanguageNegotiatorInterface .

  * @return array[]
   *   An array of language negotiation method definitions keyed by method id.

Thanks @sleitner for all the info and followup. From what I can see every was addressed so think this is ready for committer reveiw.

The patch #28 is verified, works well as the requirement and is in proper standards. can be moved to fixed.

The last submitted patch, 28: 3007386-28.patch, failed testing. View results →

Patch #28 failed only temporarily

The last submitted patch, 28: 3007386-28.patch, failed testing. View results →

Patch #28 failed only temporarily

The last submitted patch, 28: 3007386-28.patch, failed testing. View results →

The last submitted patch, 28: 3007386-28.patch, failed testing. View results →

Random

1. +++ b/core/modules/language/src/LanguageNegotiator.php
   @@ -199,7 +202,8 @@ class LanguageNegotiator implements LanguageNegotiatorInterface {
   -      $definitions = array_intersect_key($definitions, $enabled_methods);
   +      // Reduce the definitions to the enabled ones und sort them by the weight of the enabled methods.
   +      $definitions = array_merge($enabled_methods, array_intersect_key($definitions, $enabled_methods));
   

   Why was this change needed?

2. +++ b/core/modules/language/tests/src/Kernel/LanguageNegotiatorTest.php
   @@ -0,0 +1,74 @@
   +    drupal_flush_all_caches();
   

   Why was this needed? can we be more discerning here and only invalidate the required services/bins?

3. It looks like #21 hasn't been addressed

@larowlan:
1. Without this change the definitions stay sorted by the original definitions weight, but not in order of the enabled_methods.
2. drupal_flush_all_caches is not needed
3. in #22 alecsmrekar explained why sorting has to be done while reading. I do not know any automated test which tests config override from settings.php, this is for manual testing only.

the patch in #40 failed due to drupal PHP standards, just modifying the PHP standards.

making more correction to the #40

understand the /dev/null importance, thanks

Hi @rckstr_rohan
Please try to run commit-code-check sh script before creating any patch or re-roll.
Here are the steps to do that:

visit `core` directory on your local setup
run `yarn install`. This will install required js dependency
Run script using `sh ./core/scripts/dev/commit-code-check.sh`
Also, you can run test cases on local and resolve test case failures if any. Please refer documentation here https://www.drupal.org/docs/automated-testing/phpunit-in-drupal/running-... →

thanks for the information, yes will go through it and start using

Sorry, I was in the wrong directory.

From #39.3 as far as testing the settings override I searched for "['system.site']['name']" one of the options currently in settings.php. And found two test files ConfigFormOverrideTest and ConfigOverridesPriorityTest. In ConfigFormOverrideTest they do

    $settings['config']['system.site']['name'] = (object) [
      'value' => $overridden_name,
      'required' => TRUE,
    ];
    $this->writeSettings($settings);

Wonder if a test could be written in a similar manner.

If we think we should have coverage for the settings override that is.

Please review. I added a test simular to the ConfigOverridesPriorityTest with the GLOBAL variable.

The additional test looks good.

1. +++ b/core/modules/language/src/LanguageNegotiator.php
   @@ -162,10 +162,13 @@ public function initializeType($type) {
   -    return $this->configFactory->get('language.types')->get('negotiation.' . $type . '.enabled') ?: [];
   +    $types = $this->configFactory->get('language.types')->get('negotiation.' . $type . '.enabled') ?: [];
   +    asort($types, SORT_NUMERIC);
   +    return $types;
   

   I don't think we need this.

   \Drupal\language\LanguageNegotiator::saveConfiguration makes sure to

   a) Convert the weights to integers using intval
   b) Sort them before saving

2. +++ b/core/modules/language/src/LanguageNegotiator.php
   @@ -199,7 +202,8 @@ public function getNegotiationMethods($type = NULL) {
   -      $definitions = array_intersect_key($definitions, $enabled_methods);
   +      // Reduce the definitions to the enabled ones und sort them by the weight of the enabled methods.
   +      $definitions = array_merge($enabled_methods, array_intersect_key($definitions, $enabled_methods));
   

   This isn't correct

   At this point $enabled_methods looks like this

   [
     'language-url' => -8,
     'language-user' => -4,
     'language-selected' => 12,
   ]
   

   And array_intersect_key($definitions, $enabled_methods) is an array of negotiators keyed by ID.

   When we merge the two we get this

   [
     'language-url' => -8,
     'language-user' => -4,
     'language-selected' => 12,
     'language-url' => [ /* definition */],
     'language-user' => [ /* definition */],
     'language-selected' => [ /* definition */],
   ]
   
   

3. +++ b/core/modules/language/tests/src/Kernel/LanguageNegotiatorTest.php
   @@ -0,0 +1,85 @@
   +    $config->set('negotiation.language_url.enabled', [
   +      LanguageNegotiationUrl::METHOD_ID => 2,
   +      LanguageNegotiationUrlFallback::METHOD_ID => 1,
   +      LanguageNegotiationUI::METHOD_ID => -1,
   

   This test is not reflective of how the configuration behaves in the real word, because the configuration should never end up like this if it was saved via \Drupal\language\LanguageNegotiator::saveConfiguration

So in summary, I think this is a case of bad configuration on the behalf of the reporter rather than a bug in core.

I agree with larowlan, that this use case is not covering a real world scenario. It does not use the corect methods. Until now LanguageNegotiator was not intended to be configured by direct configuration changes or in settings.php.

In LanguageNegotiationSessionTest direct configuration changes are used. This should be changed to avoid high expectations.

    $config->set('negotiation.language_interface.method_weights', [
      'language-user-admin' => -10,
      LanguageNegotiationUrl::METHOD_ID => -8,
      LanguageNegotiationSession::METHOD_ID => -6,
      'language-user' => -4,
      LanguageNegotiationBrowser::METHOD_ID => -2,
      LanguageNegotiationSelected::METHOD_ID => 12,
    ]);

Maybe there should be a warning in \Drupal\language\LanguageNegotiator::getNegotiationMethods that this array is not sorted (patch attached).

LanguageNegotiator lacks a test which covers a multi negotiator scenario with weights.

@sleitner #52 appears to just be updating the comment.

Also can you please include an interdiff between the patches.

Sure, but there are no similarities. It is just updating the comment.

We should open separate issues for the LanguageNegotiationSessionTest and the additional multi negotiator test

Oh that's my mistake I thought some fixes got left out.

Sorry for the back n forth. If we are updating the comment only can we update the issue summary please. Once that's done can go ahead and move to RTBC.

Issue summary updated

Hiding all but #52

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

#52 converted in merge request 5354

I don't think that just changing the comment as suggested in the most recent patch/MR actually solves the bug that was described in the issue summary.

This test is not reflective of how the configuration behaves in the real word, because the configuration should never end up like this if it was saved via \Drupal\language\LanguageNegotiator::saveConfiguration

I'm not sure what this is supposed to mean, @larowlan? I have run into the same issue described in this IS, and it seems a few others on this thread have as well... is that not the real world evidence? Why should there be a configuration, language.types, that is NOT overridable, the way every other config in Drupal core is...? How could a developer possibly know that for this one special config, you can't override it, and instead have to call the LanguageNegotiator::saveConfiguration()?

I'm not sure what the opposition to introducing a fix here that allows you to set this with configuration overrides would be? "It wasnt intended to do that" seems like a very poor reason...

I agree with larowlan, that this use case is not covering a real world scenario. It does not use the correct methods. Until now LanguageNegotiator was not intended to be configured by direct configuration changes or in settings.php.

So a user of Drupal is only allowed to override some configuration...?

I don't think that just changing the comment as suggested in the most recent patch/MR actually solves the bug that was described in the issue summary.

I agree completely.

Rewinding to 2023-04-02. Please review again

1) Drupal\Tests\language\Kernel\LanguageNegotiatorTest::testNegotiatorPrioritiesOrder
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     0 => Array (
-        'id' => 'language-interface'
+        'id' => 'language-url-fallback'
     )
     1 => Array (
-        'id' => 'language-url-fallback'
+        'id' => 'language-url'
     )
     2 => Array (
-        'id' => 'language-url'
+        'id' => 'language-interface'
     )
 )
/builds/issue/drupal-3007386/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:94
/builds/issue/drupal-3007386/core/modules/language/tests/src/Kernel/LanguageNegotiatorTest.php:70
/builds/issue/drupal-3007386/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
2) Drupal\Tests\language\Kernel\LanguageNegotiatorTest::testLanguageNegotiatorWithOverrides
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'language-user-admin'
+'language-url'
/builds/issue/drupal-3007386/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:94
/builds/issue/drupal-3007386/core/modules/language/tests/src/Kernel/LanguageNegotiatorTest.php:82
/builds/issue/drupal-3007386/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
FAILURES!

Reran test-only feature.

Thanks for updating issue summary to stay inline.

LGTM

FWIW the code in the issue summary feels incorrect as this would affect the result on getNegotiationMethods() because you are assigning the entire enabled array.

$config['language.types']['negotiation']['language_url']['enabled'] = ['language-custom' => -1];

If think to cause the problem outlined in the issue summary you'd need to have the following configuration:

negotiation:
  language_url:
    enabled:
      language-url: 0
      language-custom: 1
      language-url-fallback: 2

And then do:

$config['language.types']['negotiation']['language_url']['enabled']['language-custom'] = -1;

Which would not work because \Drupal\language\LanguageNegotiator::getNegotiationMethods() does not sort by weights. So fun fact - we do an asort in \Drupal\language\LanguageNegotiator::saveConfiguration() which guarantees that the sequence is sorted correctly in config.

I'm tempted to close this as won't fix because it is possible to override the weights in settings.php by doing.

$config['language.types']['negotiation']['language_url']['enabled'] = [
  'language-custom ' => -1
  'language-url' => 0
  'language-url-fallback' => 1
];

Additionally we could replace the asort in \Drupal\language\LanguageNegotiator::getNegotiationMethods() with a sort defined in configuration schema. Which opens an interesting question. Should we somehow detect that configuration is being overridden and apply the config sort. That'd be a more general fix to this class of issue where if you are overriding some piece of configuration in settings.php you need to consider how the override affects sorting.

A further question though is what is the usecase of using settings.php to override this configuration?

I have to mention a related issue #2752859: LanguageNegotiators weights not respected when implementing LanguageSwitcherInterface → , which was closed as duplicate in favor of this issue. If this issue is closed as won't fix and the responsibility is handed to the settings.php configuration, it will be very hard to provide extended language switcher functionality in contrib without having users go through a cumbersome setup.

To not cause confusion here. This issue works on language_url negotiation. Sorting in LanguageNegotiator::getEnabledNegotiators() as originally proposed would have solved #2752859 as well. Now this issue shifted away from that approach, which leaves #2752859 unsolved.