Convert locale settings to configuration system

Tagging

Which form is that?

I think this might be about the locale file directory setting (form altered into the file config screen) and possibly about the default language (in a variable now) and cached number of languages (which is really a state value - which I've heard should have another system).

hmm it seems, that the part of "locale file directory setting" / locale_form_system_file_system_settings_alter() is already done in #1496480: Convert file system settings to the new configuration system

here is some changes only for the locale settings variables, haven't modified everything related to language module, so the tests fails.
maybe some variables needs to be reorganised in the yml.
there is also some tests in locale, that works with variable_get because it does provide a default_value, so the test disable a module and uninstall it, and check the variable_get value, and if it find the default_value then asserttrue, but in our case, those tests have to be modified, because the default values come on module enable i think (when it parse the yml again), not sure if it's right. see LocalUninstallTest.php line 99

8/14 locally, because it still need work.

+++ b/core/modules/locale/locale.moduleundefined
@@ -590,14 +591,15 @@ function locale_form_language_admin_edit_form_alter(&$form, &$form_state) {
+  $config = config('locale.settings');  ¶

One unnecessary whitespace

Not being worked on, can be done after Dec 1st.

Why is it not taking the tag removal?

#8: drupal8-convert-locale-settings-to-configuration-system-1751348.patch queued for re-testing.

updating yml file changes.

+++ b/core/modules/locale/config/locale.settings.yml
@@ -1,5 +1,11 @@
+  file_directory: '/sites/default/files/translations'

The leading / is wrong and this needs to have an empty default value and be calculated on demand if empty because conf_path() might be something else than sites/default.

+++ b/core/modules/locale/lib/Drupal/locale/PoDatabaseWriter.php
@@ -155,7 +155,8 @@ function getHeader() {
   function setHeader(PoHeader $header) {
     $this->_header = $header;
-    $locale_plurals = variable_get('locale_translation_plurals', array());
+    $config = config('locale.settings');
+    $locale_plurals = $config->get('translation.plurals');
 
     // Check for options.
     $options = $this->getOptions();
@@ -179,7 +180,7 @@ function setHeader(PoHeader $header) {

@@ -179,7 +180,7 @@ function setHeader(PoHeader $header) {
           'plurals' => $nplurals,
           'formula' => $formula,
         );
-        variable_set('locale_translation_plurals', $locale_plurals);
+        $config->set('translation.plurals', $locale_plurals)->save();
       }

Wondering if this is state, not config. Is this ever used/configured from the outside or just used/calculcated as needed?

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleCompareTest.php
@@ -107,6 +107,7 @@ private function makePoFile($path, $filename, $timestamp = NULL, $data = '') {
   function testLocaleCompare() {
+    $config = config('locale.settings');
     // Create and login user.
     $admin_user = $this->drupalCreateUser(array('administer site configuration', 'administer languages', 'access administration pages'));
     $this->drupalLogin($admin_user);

This doesn't seem to be used?

This is just a reroll right now.

The leading / is wrong and this needs to have an empty default value and be calculated on demand if empty because conf_path() might be something else than sites/default.

This variable is also being added by #1496480: Convert file system settings to the new configuration system where it is set to 'public://translations' which I think is a good solution. I actually think we should pull it out of that issue and into this one, so I will do that in a followup.

Wondering if this is state, not config. Is this ever used/configured from the outside or just used/calculcated as needed?

Looking at the code it doesn't appear to be set anywhere outside this function, where it is always calculated, so putting it in state seems appropriate.

Now with actual patch.

So... in looking at both issues, the implementation is far more complete in #1496480: Convert file system settings to the new configuration system so I'd rather leave it there and remove it from here. This patch actually needs quite a bit of work in addition to berdir's issues above. There are lots of missed variable calls that remain to be converted in it. Test run above canceled.

Here is a reworked version of this patch. It does the following:

- Converts the 'javascript' and 'plurals' arrays to state.
- Converts all values in locale.settings.yml to be properly formatted strings.
- Fully converts all other variables
- Removes two tests from LocaleUninstallTest. These tests only made sense in the context of a variable with a default value that got reset after locale module was uninstalled. In the current system, those variables simply disappear because the config file gets deleted, so I don't think the tests really make sense anymore.

As posted above it does not convert the translations file directory.

Tests should pass as far as I know.

This looks good to me apart from the missing upgrade path :)

+++ b/core/modules/locale/locale.installundefined
@@ -35,12 +36,10 @@ function locale_uninstall() {
   variable_del('locale_cache_length');

What's this?

What's this?

I noticed that too but wasn't sure if I should remove it. However that's its only appearance in D7 or D8, so I removed it.

This patch adds upgrade path for the variables converted to config. It also removes part of the existing upgrade path which did straight variable renames. This no longer makes sense with these variables moving to config.

One thing I noticed in past patches is that there is no upgrade path for variables to state. This makes a certain sense but also seems weird. I didn't add that here since nobody else has. However even if the variables don't get upgraded it seems we *should* at least variable_del() the old variables maybe?

#26: 1751348-locale-settings-26.patch queued for re-testing.

+++ b/core/includes/update.incundefined
@@ -469,8 +469,8 @@ function update_prepare_d8_language() {
     }
-    variable_set('locale_translation_plurals', $plurals);
-    variable_set('locale_translation_javascript', $javascript);
+    state()->set('locale.translation.plurals', $plurals);
+    state()->set('locale.translation.javascript', $javascript);
     variable_set('locale_language_negotiation_url_prefixes', $prefixes);
     variable_set('locale_language_negotiation_url_domains', $domains);

Not exactly sure what we want to do with those. At the minimum, we need to use update_variable_set().

Or we might just want to convert it right to config()/state here.

That's a good call, I just changed them to config there, and removed those variables from the update hook. That's the only change. No interdiff because I also merged HEAD.

#30: 1751348-locale-settings-29.patch queued for re-testing.

#30: 1751348-locale-settings-29.patch queued for re-testing.

Reroll

#37: 1751348-locale-settings-37.patch queued for re-testing.

This upgrade path test is, for a change, not a random bot fail. While looking into it I found a couple more irregularities in upgrade path. New patch later.

Attached patch contains:
1 - $config variable change - from locale.settings to language.negotiation.
2 - language domain URL fix (protocol/port not allowed) in language upgrade.

It fixed test case failure in #37.

Yup, that's what I found as well, the swapping of locale.settings and language.negotiation in a couple places.

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/LanguageUpgradePathTest.phpundefined
@@ -143,7 +143,7 @@ public function testLanguageUpgrade() {
-    db_update('languages')->fields(array('domain' => 'http://' . $language_domain . ':8888'))->condition('language', 'ca')->execute();
+    db_update('languages')->fields(array('domain' => $language_domain))->condition('language', 'ca')->execute();

I'm curious about this change. The test passes before the patch, shouldn't it pass after with no changes? Why do we have to change this line to make things work?

@heyrocker, I tried to set this URL with protocol and port manually at admin/config/regional/language/detection/url and got the validation error that we shouldn't have them. not sure when we got this validation in place, but removing of them did the trick.

Verified this change with Gabor so I think this is good to go.

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/LanguageUpgradePathTest.phpundefined
+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/LanguageUpgradePathTest.phpundefined
@@ -143,7 +143,7 @@ public function testLanguageUpgrade() {

@@ -143,7 +143,7 @@ public function testLanguageUpgrade() {
    */
   public function testLanguageUrlUpgrade() {
     $language_domain = 'ca.example.com';
-    db_update('languages')->fields(array('domain' => 'http://' . $language_domain . ':8888'))->condition('language', 'ca')->execute();
+    db_update('languages')->fields(array('domain' => $language_domain))->condition('language', 'ca')->execute();
     $this->variable_set('locale_language_negotiation_url_part', 1);
 

Ok, slept a bit on this. Sorry for jumping in late. This test is supposed to ensure that prior settings for domains would migrate properly in the upgrade path. When this was added, the upgrade path definitely made sure to remove protocol and ports from the domain.

Drupal 7 allows protocol and ports but recent Drupal 7 versions were changed to not actually use the value. Drupal 8 does not allow for protocol and port, so it is more compatible.

See locale_update_8003() for where the setting is updated.

The patch should pass upgrade tests without removing this.

Reuploading with that hunk removed. the fail above was preceded by an "Undefined index: ca", so that should have been a warning sign that the issue is somewhere else. Eg. actually in the upgrade path(?).

Updating patch to get the $domains from config instead of variable. Though, it fixes the upgrade test issue locally, need to confirm the use of config() in hook_update_N.

Looks like one of the ways to solve this, thanks! Although there is #1856972: config() isn't safe to use during minor updates if there's a change to configuration storage as @catch says there, other upgrade functions use config() too.

Committed to 8.x. Thanks!