language.negotiation configuration can have overrides bleed in

language_negotiation_url_prefixes()
language_negotiation_url_prefixes_update()
language_negotiation_url_prefixes_save()
language_negotiation_url_domains()
language_negotiation_url_domains_save()

Suppose all this API functions doc-blocks should be updated to point about "careful usage because of overrides"
Also there's 2 @todo in code for that issue

At least they should not be used in setting config. The functions to set may go away. The forms (should) ensure to load editable config and save back to it.

Suppose we should remove all *save() and language_negotiation_url_prefixes_update()

Also it looks that this config should have no overrides because it's language independent (no reason to translate a list of domains or url prefixes) so site-wide

Patch adds comments and initial usage clean-up while I skimmed code

Usage:

language_negotiation_url_prefixes_update()
1) new language saved \Drupal\language\Entity\ConfigurableLanguage::postSave()
2) site default language changed \Drupal\language\EventSubscriber\ConfigSubscriber::onConfigSave

language_negotiation_url_prefixes_save()
language_configurable_language_delete and from update above

language_negotiation_url_domains_save() when configurable language added or deleted

1. +++ b/core/modules/language/language.module
   @@ -332,7 +332,10 @@ function language_language_types_info() {
    function language_negotiation_url_prefixes() {
   -  return \Drupal::config('language.negotiation')->get('url.prefixes');
   +  // Should always return data without overrides.
   +  return \Drupal::configFactory()
   +    ->getEditable('language.negotiation')
   +    ->get('url.prefixes');
    }
    
    /**
   @@ -368,7 +371,10 @@ function language_negotiation_url_prefixes_save(array $prefixes) {
   
   @@ -368,7 +371,10 @@ function language_negotiation_url_prefixes_save(array $prefixes) {
     * Reads language domains.
     */
    function language_negotiation_url_domains() {
   -  return \Drupal::config('language.negotiation')->get('url.domains');
   +  // Should always return data without overrides.
   +  return \Drupal::configFactory()
   +    ->getEditable('language.negotiation')
   +    ->get('url.domains');
    }
   

   There may still be other overrides based on domain or organic group or whatever other condition, overrides are not necessarily language specific only. The getters should still return possibly overridden data.

2. +++ b/core/modules/language/src/Form/NegotiationUrlForm.php
   @@ -209,12 +209,11 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +      // Save new domain and prefix values.
   +      ->set('url.prefixes', $form_state->getValue('prefix'))
   +      ->set('url.domains', $form_state->getValue('domain'))
          ->save();
    
   -    // Save new domain and prefix values.
   -    language_negotiation_url_prefixes_save($form_state->getValue('prefix'));
   -    language_negotiation_url_domains_save($form_state->getValue('domain'));
   

   These look great. IMHO we should remove the save functions entirely to avoid overridden data saved.

Reverted getters.
Removed the save functions, thinking to remove language_negotiation_url_prefixes_update()

The rest of usage is

core/modules/language/src/Entity/ConfigurableLanguage.php:136:    language_negotiation_url_prefixes_update();
core/modules/language/src/EventSubscriber/ConfigSubscriber.php:63:        language_negotiation_url_prefixes_update();

Also looks that language_configurable_language_insert() & language_configurable_language_delete() could be moved to \Drupal\language\Entity\ConfigurableLanguage::postSave and postDelete() methods

The proposed changes all look great now. AFAIS language_negotiation_url_prefixes_update() may still very well leak overrides in. Although you fixed how it saves now, it still gets its source data from language_negotiation_url_prefixes(). It should either be removed or at least get its data from the editable configuration.

No concerns with the patch other than that.

Here's re-roll and clean-up - no reason to set default language when the same, but maybe it's for separate issue too?

@Gabor no, it uses editable now but maybe we really should get prefixes after override?

+++ b/core/modules/language/language.module
@@ -324,7 +324,8 @@ function language_negotiation_url_prefixes() {
-  $prefixes = language_negotiation_url_prefixes();
+  $config = \Drupal::configFactory()->getEditable('language.negotiation');
+  $prefixes = $config->get('url.prefixes');

is fine?

1. +++ b/core/modules/language/language.module
   @@ -324,7 +324,8 @@ function language_negotiation_url_prefixes() {
    function language_negotiation_url_prefixes_update() {
   -  $prefixes = language_negotiation_url_prefixes();
   +  $config = \Drupal::configFactory()->getEditable('language.negotiation');
   +  $prefixes = $config->get('url.prefixes');
   

   Right this looks good, sorry for not figuring it out from the diff earlier.

2. +++ b/core/modules/language/src/EventSubscriber/ConfigSubscriber.php
   @@ -57,7 +57,7 @@ public function onConfigSave(ConfigCrudEvent $event) {
   -      if ($language) {
   +      if ($language && $language->getId() != $saved_config->getOriginal('default_langcode')) {
   

   This change is not even remotely related to config overrides.

So #8 looks like a good patch but the changes in #10 are too much for this scope AFAIS. Wanna post it again? :)

Yep, so here's a re-roll of #8
I see no way to add tests for that
Also filed #2462729: Move ConfigurableLanguage hook implementations in language module to the entity

As for tests, I think the test can set $GLOBAL['config'] value for language.negotiation to something custom and call the language_negotiation_url_prefixes_update() function and assert that the override did not end up in config.

As for testing the forms, you can save a language override (it does not matter that you override things that are otherwise not translatable) and call the form to observe the values are not used. That would also prove that NegotiationUrlForm is not yet immune to overrides, language_negotiation_url_prefixes() and language_negotiation_url_domains() are still used in the buildform (just found while looking at the code to suggest tests).

Marking as needs work for fixing the use of language_negotiation_url_prefixes() and language_negotiation_url_domains() in NegotiationUrlForm and for tests.

Fixed usage in form, will try tests

Amazing, thanks! Sending to testbot.

Back to needs work for lack of new tests then.

added templates to issue summary.

Saw this one sitting idle for a while, so I updated issue summary (proposed resolution) according to what appears to be happening in the patch.

For the tests, I could use some help in documenting some steps to reproduce:

For setting and then checking the global config value for language negotiation, I'm trying things like this:

$GLOBALS['config']['language.negotiation'] = array('url.prefixes' => array('en' => 'whatever'));
language_negotiation_url_prefixes_update();
dpm(\Drupal::config('language.negotiation')->get('url.prefixes'));

I think I need a different data type to initialize the language-negotiation global config variable. I haven't been able to see the way to make runtime changes also modify stored configuration.

@robertdbailey: you can write a test that puts that to settings.php on install and then invokes the URL of NegotiationUrlForm to verify that those overrides don't show up there anymore. Without the patch if there is some kind of override for language settings, that will show up in the form (and get saved back when you hit save on the form). There is no UI to reproduce the bug because language.negotiation config values are not translatable and the only UI to edit config overrides in core is for languages. What you can also do in the test is to add a foreign language, set an override in that language of language.negotiation and then access the language URL settings page in that language, and check the overrides are not applied.

created tests for checking English language.negotiation overrides and then Spanish language.negotiation overrides. Please review.

Tests in #23 looks good (though it's missing second part of @Gábor Hojtsy suggestion in #22) and the main fix has been in review since #14. I think this is good to go.

Thanks for reviewing, @vijaycs85! I've updated the tests to include the second part of #22. Please review.

Patches from #25 did not seem to be queued properly for testing. Retrying as #26.

Agreed with @alexpott.

Deprecating both language_negotiation_url_prefixes() and language_negotiation_url_domains().
I'm wondering if we should do the same with language_negotiation_url_prefixes_update()

There is no use of language_negotiation_url_prefixes_update() in core, only one mention in comments. Not sure we can deprecate it though unless we have some other place where its implemented. The ConfigSubscriber::onConfigSave() (where a copy of its code resides) in language is not a reusable place. I don't think moving that around would be in scope for this issue anyway. Let's get this in then.

@Gábor: There is one use of language_negotiation_url_prefixes_update() inConfigurableLanguage::postSave. But yeah, we can leave that for a follow-up anyway.

There is one use of language_negotiation_url_prefixes_update() inConfigurableLanguage::postSave. But yeah, we can leave that for a follow-up

This follow-up still needed

Amazing, thanks all!

@andypost Thanks for the reminder, created #2548079: Move language negotiation prefix logic update to ConfigurableLanguage.