Part of #1696224: Remove system_settings_form()
Convert different language forms to use system_config_form().

Related to #1751348: Convert locale settings to configuration system

Files: 
CommentFileSizeAuthor
#30 drupal8.config-language-negotiation.30.patch14.67 KBn3or
PASSED: [[SimpleTest]]: [MySQL] 46,232 pass(es).
[ View ]
#30 interdiff.txt728 bytesn3or
#28 drupal8.config-language-negotiation.28.patch14.39 KBn3or
FAILED: [[SimpleTest]]: [MySQL] 46,221 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#28 interdiff.txt8.34 KBn3or
#26 drupal8.config-language-negotiation.26.patch14.15 KBn3or
PASSED: [[SimpleTest]]: [MySQL] 42,804 pass(es).
[ View ]
#26 interdiff.txt2.68 KBn3or
#19 drupal8.config-language-negotiation.19.patch14.05 KBn3or
PASSED: [[SimpleTest]]: [MySQL] 40,244 pass(es).
[ View ]
#19 interdiff.txt1.79 KBn3or
#16 drupal8.config-language-negotiation.16.patch14.33 KBn3or
FAILED: [[SimpleTest]]: [MySQL] 39,996 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#16 interdiff.txt2.25 KBn3or
#13 drupal8.config-language-negotiation.13.patch15.75 KBn3or
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#13 interdiff.txt3.3 KBn3or
#8 drupal8.config-language-negotiation.8.patch6.55 KBn3or
PASSED: [[SimpleTest]]: [MySQL] 39,900 pass(es).
[ View ]
#8 interdiff.txt3.19 KBn3or
#6 drupal8.config-language-negotiation.6.patch7.11 KBsun
PASSED: [[SimpleTest]]: [MySQL] 39,811 pass(es).
[ View ]
#5 config-language.5.patch7.43 KBsun
PASSED: [[SimpleTest]]: [MySQL] 39,823 pass(es).
[ View ]
#4 config-language.4.patch7.4 KBn3or
PASSED: [[SimpleTest]]: [MySQL] 39,811 pass(es).
[ View ]
#4 interdiff.txt5.32 KBn3or
#1 config-language.1.patch5.79 KBn3or
FAILED: [[SimpleTest]]: [MySQL] 39,813 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new5.79 KB
FAILED: [[SimpleTest]]: [MySQL] 39,813 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Converts language_negotiation_configure_session_form().

Status:Needs review» Needs work

The last submitted patch, config-language.1.patch, failed testing.

+++ b/core/modules/language/config/language.settings.yml
@@ -0,0 +1,3 @@
+negotiation:
+    session:
+        parameter: language
+++ b/core/modules/language/language.negotiation.inc
@@ -171,7 +171,7 @@ function language_from_user($languages) {
+  $param = config('language.settings')->get('negotiation.session_parameter');

1) You have a underscore vs period difference in the config key there ;) It should be a dot/sub-key.

2) How about we name the config object language.negotiation, essentially removing the top-level 'negotiation' key?

Status:Needs work» Needs review
StatusFileSize
new5.32 KB
new7.4 KB
PASSED: [[SimpleTest]]: [MySQL] 39,811 pass(es).
[ View ]

Fixed #3.

StatusFileSize
new7.43 KB
PASSED: [[SimpleTest]]: [MySQL] 39,823 pass(es).
[ View ]

Renamed language.settings into language.negotiation.

StatusFileSize
new7.11 KB
PASSED: [[SimpleTest]]: [MySQL] 39,811 pass(es).
[ View ]

Simplified the locale module update -- no need to re-invent the wheel ;)

We should also convert the other language negotiation settings in this issue/patch into the same config object.

Status:Needs review» Needs work

+++ b/core/modules/language/language.install
@@ -49,6 +48,8 @@ function language_uninstall() {
+  config('language.negotiation')->delete('session.parameter')->save();

Erm, ->delete()->save() ...? ;)

This can be removed entirely. ;)

+++ b/core/modules/language/language.install
@@ -102,3 +103,12 @@ function language_schema() {
+/**
+ * Moves language settings from variables to config.
+ */
+function language_update_8000() {
+  update_variables_to_config('language.negotiation', array(
+    'language_negotiation_session_param' => 'session.parameter',
+  ));
+}
+++ b/core/modules/locale/locale.install
@@ -568,6 +568,10 @@ function locale_update_8008() {
+  update_variables_to_config('language.negotiation', array(
+    'locale_language_negotiation_session_param' => 'session.parameter',
+  ));

Hm... the added language update duplicates the existing locale module update...?

We should keep the existing locale module update only.

Status:Needs work» Needs review
StatusFileSize
new3.19 KB
new6.55 KB
PASSED: [[SimpleTest]]: [MySQL] 39,900 pass(es).
[ View ]

Contains compatibility update to current core and changes of comment #7 (hopefully I did it right).

EDIT: Can't remove the following code:

config('language.negotiation')->delete('session.parameter');

(-> The tests "LocaleUninstallTest" and "LocaleUninstallFrenchTest" in module "locale" failed.)

Status:Needs review» Needs work

there is no update function for the variable

Do you mean the code I removed because of comment #7?

sun didn't asked to remove both update functions, just to delete one and keep the locale.install update function.

Status:Needs work» Needs review
StatusFileSize
new3.3 KB
new15.75 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

You're right! Thank you. Added update function in this patch again. Additionally I converted language_negotiation_configure_url_form().

The variables "language_negotiation_url_prefixes" and "language_negotiation_url_domains" are part of this transformation and their structure doesn't fit cleanly for the config system. It would be fine to find a structure optimized for language and config system.

It could be a cleaner solution to create a config object for every negotiation handler (e. g. language.negotiation.url). That means we have to iterate through all config objects if we want to create/delete a language and have to add/remove the relevant keys. But at the moment there isn't a possibility to select config objects by prefix (e. g. config_by_prefix(language.negotiation.) ). In addition there is no possibility to automate the create/delete process because there is no specification in which keys/subkeys we have to create/remove a language specific entry.

As an alternative we could create a config object for every language, but there is the same problem by creating/removing negotiation handlers. Thus there is a kind of three dimensional structure, which can't resolved cleanly by config system.

In a first step (this issue) I converted the mentioned variables (as is) into a single configuration object (language.negotiation) with default values for D8 default negotiation handlers and language. That makes it possible to fix this issue. In a follow up issue we can concentrate on finding and creating a structure which handles this dependencies better.

(I talked with sun about this problem and I hope I summarized this problem correct and understandable.)

The attached patch contains the first version for transformation of mentioned form and there are some parts missing in attached interdiff.

Status:Needs review» Needs work

The last submitted patch, drupal8.config-language-negotiation.13.patch, failed testing.

+++ b/core/modules/language/language.install
@@ -49,6 +45,10 @@ function language_uninstall() {
+  config('language.negotiation')
+    ->delete('session.parameter')
+    ->delete('url.determination_part');

::delete() deletes the entire configuration object. You want ::clear($key). :)

StatusFileSize
new2.25 KB
new14.33 KB
FAILED: [[SimpleTest]]: [MySQL] 39,996 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

Fixed #15 and a fatal error in "language upgrade test". Need now a complete recheck.

Some code I removed is necessary for a successful upgrade so I added this code again. (The language/locale upgrade path is really hard to understand!)

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, drupal8.config-language-negotiation.16.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.79 KB
new14.05 KB
PASSED: [[SimpleTest]]: [MySQL] 40,244 pass(es).
[ View ]

Should become green..

Issue tags:+D8MI

Status:Needs review» Needs work

It still neds a work for other variables and will need for variables that are introduced with #1191488: META: Integrate l10n_update functionality in core

Suppose we need to consistency in naming and probably postpone this

Filed separate issue #1751348: Convert locale settings to configuration system to focus this on language module only

Can someone explain whether the l10n_update variables are in core already and whether we really need to incorporate them in this issue/conversion? It's a bit confusing, since this issue is about Language module settings, but l10n_update is about Locale module, I think?

+++ b/core/modules/language/config/language.negotiation.yml
@@ -0,0 +1,9 @@
+url:
+    determination_part: '0'
+++ b/core/modules/language/language.admin.inc
@@ -662,7 +665,7 @@ function language_negotiation_configure_url_form($form, &$form_state) {
       LANGUAGE_NEGOTIATION_URL_PREFIX => t('Path prefix'),
       LANGUAGE_NEGOTIATION_URL_DOMAIN => t('Domain'),
...
+    '#default_value' => config('language.negotiation')->get('url.determination_part'),

It looks like we want to change the LANGUAGE_NEGOTIATION_URL_* constants from integers to strings like we did for other conversions (e.g., 'prefix', 'domain', etc).

When doing so, I think we also want a better config key than url.determination_part -- url.source might work.

+++ b/core/modules/language/language.admin.inc
@@ -782,31 +780,40 @@ function language_negotiation_configure_url_form_validate($form, &$form_state) {
function language_negotiation_configure_url_form_submit($form, &$form_state) {
...
+  config('language.negotiation')->set('url.determination_part', $form_state['values']['language_negotiation_url_part'])->save();

Can we write the chained method calls as usual (on separate lines)?

Can someone explain whether the l10n_update variables are in core already and whether we really need to incorporate them in this issue/conversion? It's a bit confusing, since this issue is about Language module settings, but l10n_update is about Locale module, I think?

Indeed, l10n_update is in the locale module, this issue should be focused on language module I think.

Assigned:n3or» Unassigned
Status:Needs work» Needs review
StatusFileSize
new2.68 KB
new14.15 KB
PASSED: [[SimpleTest]]: [MySQL] 42,804 pass(es).
[ View ]

Patch should apply to current core and I changed chained method calls (#24).

Title:Convert language settings to configuration systemConvert language negotiation settings to configuration system

Thanks for the continued work! I'm not fully up to date in terms of how config standards apply now, so consider this my best thinking only :)

+++ b/core/modules/language/config/language.negotiation.ymlundefined
@@ -0,0 +1,9 @@
+# @todo Consider to split negotiation settings into language-specific objects.
+session:
+    parameter: language
+url:
+    determination_part: '0'
+    prefixes:
+      en: ''
+    domains:
+      en: ''

Negotiation settings are not language specific, they are global to the site, so I'm not sure about that @todo?

Also, the '0' looks pretty odd here. It sounds like it would make sense to swap this to be 'prefixes' and 'domains' and change the constants to use those names too, no?

+++ b/core/modules/language/language.installundefined
@@ -37,10 +37,6 @@ function language_uninstall() {
-  variable_del('language_negotiation_url_part');
-  variable_del('language_negotiation_url_prefixes');
-  variable_del('language_negotiation_url_domains');
-  variable_del('language_negotiation_session_param');
   variable_del('language_content_type_default');
   variable_del('language_content_type_negotiation');
@@ -49,6 +45,10 @@ function language_uninstall() {
@@ -49,6 +45,10 @@ function language_uninstall() {
     variable_del("language_negotiation_methods_weight_$type");
   }
+  config('language.negotiation')
+    ->clear('session.parameter')
+    ->clear('url.determination_part');
+

The actual prefixes and domains are not cleared out?

Otherwise looks good to me. Thanks again!

StatusFileSize
new8.34 KB
new14.39 KB
FAILED: [[SimpleTest]]: [MySQL] 46,221 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

- Replaced url.determination_part with url.source (see #22)
- Now we use string-identifiers in url.source (see #22/#24)
- Removed unnecessary "clear" in language_uninstall (see #24)

There are some other forms related to language negotiation. But I think if we put every (language negotiation) form conversion into this patch, it will become a patch from hell. So can we first make this part rtbc?

Status:Needs review» Needs work

The last submitted patch, drupal8.config-language-negotiation.28.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new728 bytes
new14.67 KB
PASSED: [[SimpleTest]]: [MySQL] 46,232 pass(es).
[ View ]

Fixed failing test.

Status:Needs review» Reviewed & tested by the community

Looks good to me.

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

@n3or and I discussed this patch to some lengths, and we definitely intended to keep the scope very limited, and I'm glad those parts landed now.

We also considered to simply re-open and keep this issue in order to perform the additional conversions, but in hindsight, that might not be a good idea, since that always presents a confusing situation for reviewers and other contributors.

Thus, @n3or, or whoever else wants to tackle the further language negotiation variable conversions: Let's make sure to create an issue for those and link to it from here. Thanks! :)

Good job!

Status:Fixed» Closed (fixed)

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

Issue summary:View changes

Updated issue summary.