Support from Acquia helps fund testing for Drupal Acquia logo

Comments

n3or’s picture

Status: Active » Needs review
FileSize
5.79 KB

Converts language_negotiation_configure_session_form().

Status: Needs review » Needs work

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

sun’s picture

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

n3or’s picture

Status: Needs work » Needs review
FileSize
5.32 KB
7.4 KB

Fixed #3.

sun’s picture

FileSize
7.43 KB

Renamed language.settings into language.negotiation.

sun’s picture

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.

sun’s picture

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.

n3or’s picture

Status: Needs work » Needs review
FileSize
3.19 KB
6.55 KB

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

xjm’s picture

aspilicious’s picture

Status: Needs review » Needs work

there is no update function for the variable

n3or’s picture

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

aspilicious’s picture

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

n3or’s picture

Status: Needs work » Needs review
FileSize
3.3 KB
15.75 KB

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.

sun’s picture

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

n3or’s picture

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

n3or’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

n3or’s picture

Status: Needs work » Needs review
FileSize
1.79 KB
14.05 KB

Should become green..

n3or’s picture

Gábor Hojtsy’s picture

Issue tags: +D8MI
andypost’s picture

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

andypost’s picture

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

sun’s picture

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

Gábor Hojtsy’s picture

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.

n3or’s picture

Assigned: n3or » Unassigned
Status: Needs work » Needs review
FileSize
2.68 KB
14.15 KB

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

Gábor Hojtsy’s picture

Title: Convert language settings to configuration system » Convert 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!

n3or’s picture

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

n3or’s picture

Status: Needs work » Needs review
FileSize
728 bytes
14.67 KB

Fixed failing test.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

sun’s picture

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

ianthomas_uk’s picture

ianthomas_uk’s picture

Issue summary: View changes

Updated issue summary.