Files: 
CommentFileSizeAuthor
#49 1751348-locale-settings-49.patch18.62 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 50,593 pass(es).
[ View ]
#49 1751348-diff-47-49.txt880 bytesvijaycs85
#47 1751348-locale-settings-47.patch17.93 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 50,418 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#42 1751348-locale-settings-41.patch18.88 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 50,399 pass(es).
[ View ]
#42 1751348-diff-37-41.txt2.21 KBvijaycs85
#37 1751348-locale-settings-37.patch17.88 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] 49,682 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#30 1751348-locale-settings-29.patch18.56 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1751348-locale-settings-29.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#26 1751348-locale-settings-26.patch18.48 KBheyrocker
PASSED: [[SimpleTest]]: [MySQL] 48,647 pass(es).
[ View ]
#26 interdiff.txt2.05 KBheyrocker
#24 1751348-locale-settings-24.patch16.88 KBheyrocker
PASSED: [[SimpleTest]]: [MySQL] 48,641 pass(es).
[ View ]
#21 1751348-locale-settings-21.patch14.43 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#17 1751348-locale-settings-to-CMI-17.patch15.08 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 48,763 pass(es), 159 fail(s), and 59 exception(s).
[ View ]
#8 drupal8-convert-locale-settings-to-configuration-system-1751348.patch24.93 KBjulien
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8-convert-locale-settings-to-configuration-system-1751348.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

larowlan’s picture

Issue tags:+CMI

Tagging

penyaskito’s picture

Issue tags:+D8MI, +sprint, +language-base
tobiasb’s picture

Which form is that?

Gábor Hojtsy’s picture

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

tobiasb’s picture

Assigned:Unassigned» tobiasb
tobiasb’s picture

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

sun’s picture

julien’s picture

Status:Active» Needs work
StatusFileSize
new24.93 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8-convert-locale-settings-to-configuration-system-1751348.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

andypost’s picture

Status:Needs work» Needs review

Let's see how many tests will be broken

julien’s picture

8/14 locally, because it still need work.

Status:Needs review» Needs work
tobiasb’s picture

+++ 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

Gábor Hojtsy’s picture

Issue tags:-sprint

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

Gábor Hojtsy’s picture

Why is it not taking the tag removal?

vijaycs85’s picture

Status:Needs work» Needs review
Issue tags:-Configuration system, -D8MI, -language-base

Status:Needs review» Needs work
Issue tags:+Configuration system, +D8MI, +language-base
vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new15.08 KB
FAILED: [[SimpleTest]]: [MySQL] 48,763 pass(es), 159 fail(s), and 59 exception(s).
[ View ]

updating yml file changes.

Status:Needs review» Needs work

The last submitted patch, 1751348-locale-settings-to-CMI-17.patch, failed testing.

Berdir’s picture

+++ 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?

heyrocker’s picture

Status:Needs work» Needs review

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.

heyrocker’s picture

StatusFileSize
new14.43 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Now with actual patch.

Status:Needs review» Needs work

The last submitted patch, 1751348-locale-settings-21.patch, failed testing.

heyrocker’s picture

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.

heyrocker’s picture

Status:Needs work» Needs review
StatusFileSize
new16.88 KB
PASSED: [[SimpleTest]]: [MySQL] 48,641 pass(es).
[ View ]

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.

Berdir’s picture

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?

heyrocker’s picture

StatusFileSize
new2.05 KB
new18.48 KB
PASSED: [[SimpleTest]]: [MySQL] 48,647 pass(es).
[ View ]

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?

Status:Needs review» Needs work
Issue tags:-Configuration system, -D8MI, -language-base

The last submitted patch, 1751348-locale-settings-26.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
Issue tags:+Configuration system, +D8MI, +language-base

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

Berdir’s picture

Status:Needs review» Needs work
+++ 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.

heyrocker’s picture

StatusFileSize
new18.56 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1751348-locale-settings-29.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

heyrocker’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:-Configuration system, -D8MI, -language-base

The last submitted patch, 1751348-locale-settings-29.patch, failed testing.

heyrocker’s picture

Status:Needs work» Needs review

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

Status:Needs review» Needs work

The last submitted patch, 1751348-locale-settings-29.patch, failed testing.

heyrocker’s picture

Status:Needs work» Needs review

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

Status:Needs review» Needs work
Issue tags:+Configuration system, +D8MI, +language-base

The last submitted patch, 1751348-locale-settings-29.patch, failed testing.

heyrocker’s picture

Status:Needs work» Needs review
StatusFileSize
new17.88 KB
FAILED: [[SimpleTest]]: [MySQL] 49,682 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

Reroll

Status:Needs review» Needs work
Issue tags:-Configuration system, -D8MI, -language-base

The last submitted patch, 1751348-locale-settings-37.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review

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

Status:Needs review» Needs work
Issue tags:+Configuration system, +D8MI, +language-base

The last submitted patch, 1751348-locale-settings-37.patch, failed testing.

heyrocker’s picture

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.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new2.21 KB
new18.88 KB
PASSED: [[SimpleTest]]: [MySQL] 50,399 pass(es).
[ View ]

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.

heyrocker’s picture

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?

vijaycs85’s picture

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

heyrocker’s picture

Status:Needs review» Reviewed & tested by the community

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

Gábor Hojtsy’s picture

Status:Reviewed & tested by the community» Needs work
+++ 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.

Gábor Hojtsy’s picture

Status:Needs work» Needs review
StatusFileSize
new17.93 KB
FAILED: [[SimpleTest]]: [MySQL] 50,418 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 1751348-locale-settings-47.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new880 bytes
new18.62 KB
PASSED: [[SimpleTest]]: [MySQL] 50,593 pass(es).
[ View ]

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.

Gábor Hojtsy’s picture

Status:Needs review» Reviewed & tested by the community

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.

Dries’s picture

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. Thanks!

andypost’s picture

I filed follow-up regression #1919002: Upgrade to D8 broken when D7 has more then one language enabled
Probably this caused by config() usage #1856972: config() isn't safe to use during minor updates if there's a change to configuration storage
This points that this issue does not have upgrade path tests for the convertion

Automatically closed -- issue fixed for 2 weeks with no activity.