Problem/Motivation

There is this great language selection configuration feature on content types that we recently improved greatly:
gen-before-s01-article-2012-10-13_1222.png
However, with language supported on vocabularies, taxonomy terms, files and other entities, we need a way to configure this kind of thing on other entities. Taxonomy terms or vocabularies do not have bundles, but it would make tons of sense to have this kind of setting on vocabularies for their children terms.

Before of the vocab edit:
gen-before-s02-vocab-edit-2012-10-13_1226.png

Proposed resolution

The main complication in this is how the values are stored and applied. Likely we'll need to keep some custom wrapper / accessor code for node types and vocabularies and reuse the UI from one place.

Remaining tasks

Remaining: Verify the issue summary is correct in terms of api changes, and update issue summary saying if documentation needs to be written
Done: code style looks good
Done: have gotten a few code reviews in #25 by fago and #50 by planch, and #102 (plus some early looks by webflo, Berdir and LoMo). Items from those reviews were addresses. And a final look by Gabor in #126
Done: ui review. in #33 has a UI RTBC from Bojhan

User interface changes

Currently the ui with patch looks like:
In the content type edit, there is an improvement to add (TheLanguage) which shows what the current site default language is in the drop down select and also in the summary in the vertical tab.
gen-s01-contenttype-2012-10-12_2238.png
and:
In the vocab edit (for example), it reuses the generalized ui, and has a label the entity sets.
gen-s04-vocab-2012-10-12_2244.png
(from #127)

API changes

(API changes/additions that would affect module, install profile, and theme developers, including examples of before/after code if appropriate)

Postponed on this issue

#1810386: Create workflow to setup multilingual for entity types, bundles and fields is postponed on this issue.

CommentFileSizeAuthor
#162 drupal-language_config-1739928-162.patch5.28 KBplach
#136 language_configuration-1739928-136.patch53.74 KBvasi1186
#136 interdiff_135_136.txt805 bytesvasi1186
#135 language_configuration-1739928-135.patch53.74 KBvasi1186
#135 interdiff_133_135.txt787 bytesvasi1186
#133 language_configuration-1739928-132.patch53.73 KBvasi1186
#133 interdiff_125_132.txt5.03 KBvasi1186
#128 gen-before-s01-article-2012-10-13_1222.png86.54 KBYesCT
#128 gen-before-s02-vocab-edit-2012-10-13_1226.png55.41 KBYesCT
#127 gen-s01-contenttype-2012-10-12_2238.png135.86 KBYesCT
#127 gen-s02-contenttype-select-2012-10-12_2240.png90.91 KBYesCT
#127 gen-s03-contenttype-nondef-nonhidden-2012-10-12_2242.png94.31 KBYesCT
#127 gen-s04-vocab-2012-10-12_2244.png78.31 KBYesCT
#127 gen-s05-vocab-dropdown-2012-10-12_2249.png75.56 KBYesCT
#125 language_configuration-1739928-125.patch54.55 KBvasi1186
#125 interdiff_91_125.txt4.26 KBvasi1186
#119 language_configuration-1739928-119.patch54.94 KBYesCT
#119 interdiff-kinda-115-119.txt1.09 KBYesCT
#115 language_configuration-1739928-115.patch54.94 KBvasi1186
#115 interdiff_91_115.txt4.29 KBvasi1186
#91 language_configuration-1739928-91.patch52.68 KBYesCT
#91 interdiff-87-91.txt503 bytesYesCT
#87 language_configuration-1739928-87.patch52.94 KBYesCT
#87 interdiff-81-87.txt2.45 KBYesCT
#87 interdiff-70-87.txt9.34 KBYesCT
#82 language_configuration-1739928-81.patch52.99 KBYesCT
#82 interdiff-80-81.txt9.41 KBYesCT
#80 language_configuration-1739928-80.patch51.4 KBYesCT
#80 interdiff-70-70-reroll.txt853 bytesYesCT
#80 language_configuration_1739928_70-fixed.patch51.4 KBYesCT
#80 interdiff-70-fixed-80.txt2.4 KBYesCT
#70 language_configuration_1739928_70.patch51.37 KBvasi1186
#70 interdiff_1739928_67_70.txt1013 bytesvasi1186
#67 language_configuration-1739928-67.interdiff.do_not_test.patch3.46 KBplach
#67 language_configuration-1739928-67.patch50.38 KBplach
#66 language_configuration-1739928-66.patch50.38 KBYesCT
#66 interdiff-1739928-63-66.txt3.03 KBYesCT
#64 language_configuration-1739928-63.patch50.33 KBTransitionKojo
#64 interdiff-59-64.txt1.25 KBTransitionKojo
#59 interdiff_1739928_57_59.txt3.81 KBvasi1186
#59 language_configuration_1739928_59.patch50.84 KBvasi1186
#57 with-langcode-js-change-2012-09-04_0406.png45.66 KBYesCT
#57 language_configuration-1739928-57.patch49.92 KBYesCT
#57 interdiff_1739928_54_57.txt859 bytesYesCT
#56 some-weirdness-2012-09-04_0247.png126.85 KBYesCT
#56 vocab-edit-looks-good-2012-09-04_0256.png76.41 KBYesCT
#56 without-patch-shows-default-language-2012-09-04_0321.png117.18 KBYesCT
#54 language_configuration_1739928_54.patch49.92 KBvasi1186
#54 interdiff_1739928_52_54.txt2.08 KBvasi1186
#52 interdiff_1739928_48_52.txt22.84 KBvasi1186
#52 language_configuration_1739928_52.patch49.92 KBvasi1186
#48 language_configuration_1739928_48.patch50.62 KBvasi1186
#48 interdiff_1739928_45_48.txt1.24 KBvasi1186
#45 language_configuration_1739928_45.patch49.55 KBvasi1186
#45 interdiff_1739928_39_45.txt15.98 KBvasi1186
#39 language_configuration_1739928_39.patch38.35 KBvasi1186
#39 interdiff_1739928_36_39.txt3.68 KBvasi1186
#36 language_configuration_1739928_36.patch37.5 KBvasi1186
#36 interdiff_1739928_30_36.txt12.29 KBvasi1186
#32 content-type-edit-2012-08-26_1116.png114.12 KBYesCT
#32 content-type-drop-down-2012-08-26_1134.png39.99 KBYesCT
#32 vocabulary-edit-2012-08-26_1118.png93.1 KBYesCT
#32 node-add-2012-08-26_1127.png41.72 KBYesCT
#32 content-add-with-drop-down-2012-08-26_1135.png52.15 KBYesCT
#32 tag-add-drop-down-2012-08-26_1135.png48.09 KBYesCT
#30 language_configuration_1739928_29.patch25.75 KBvasi1186
#30 interdiff_1739928_22_29.txt17.69 KBvasi1186
#26 ui-before-vocab-edit-2012-08-25_1730.png64 KBYesCT
#26 ui-before-term-edit-2012-08-25_1731.png76.53 KBYesCT
#26 ui-after-vocab-edit-2012-08-25_1740.png70.61 KBYesCT
#26 ui-after-term-add-2012-08-25_1741.png59.1 KBYesCT
#26 ui-after-term-add-with-lang-2012-08-25_1811.png64.48 KBYesCT
#22 language_configuration_1739928_22.patch24.83 KBvasi1186
#22 interdiff_1739928_20_22.txt2.19 KBvasi1186
#20 language_configuration_1739928_20.patch22.63 KBvasi1186
#19 language_configuration_1739928_19.patch9.47 KBwebflo
#19 language_configuration_1739928_16_19.interdiff.txt439 byteswebflo
#16 language_configuration_1739928_16.patch9.46 KBvasi1186
#16 interdiff_1739928_14_16.txt1017 bytesvasi1186
#14 language_configuration_1739928_14.patch9.5 KBvasi1186
#14 interdiff_1739928_12_14.txt4.87 KBvasi1186
#12 language_configuration_1739928_12.patch9.76 KBvasi1186
#12 interdiff_8_12.txt3.6 KBvasi1186
#8 language_configuration_1739928_8.patch10 KBvasi1186
LanguageSettings.png51.23 KBGábor Hojtsy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vasi1186’s picture

Component: language system » other

I have one idea of how we can implement this. We can provide a new form element type: "language_setting" (or something like that) that will have these two attributes (among others maybe):
- #entity_type: specifies for what entity type is the setting. This should be mandatory.
- #bundle: optional, specifies for what bundle is the setting (if the entity type has bundles).

We also have to add somehow an additional submit handler in the form (I think this can be done in the process callback of the element). On the submit handler, we will just save the values (we should be able to get the entity type and the bundle, so we can generate unique variable names using them).
The last thing would be to actually create some function that would act as a wrapper to get a specific language setting value for an entity type and bundle.

And of course, then we will have to just use this new form element on the forms where we need to add language settings, and use those settings when we print the language_select form element on the entity add/edit form.

vasi1186’s picture

And for some reason I don't know why I don't have the language system option in the Component select field... that's why I had to change the Component.

Gábor Hojtsy’s picture

Just using the bundle is not going to be enough since we want to have this setting per vocabulary for terms. Vocabularies are not bundles. Not sure a form element is good for this, but I see the appeal. We can start with that and rework it later if need be.

vasi1186’s picture

Yes, that's true, vocabularies are not bundles, but for them we could use the machine name of the vocabulary, instead of the bundle. Basically, the form API would not impose any restriction of what values will the entity_type and bundle properties contain. Maybe it is not such a good idea to name them like this in that case. The main goal I think it is to find something that can uniquely identity a set of entities (and the (entity_type, bundle) combination is a good example).

Gábor Hojtsy’s picture

Naming things is hard. Bundle is an easy choice, it might become misleading when not used for bundle names, but we can use this name for now, yeah :)

vasi1186’s picture

Assigned: Unassigned » vasi1186
vasi1186’s picture

Component: other » language system
vasi1186’s picture

Status: Active » Needs review
FileSize
10 KB

Attached a first patch to be reviewed. It defines a new element type to configure language on entity types. Things that have to be implemented further:
- use this form element on the node bundle configuration form.
- use this form element on the other entity cofiguration forms (like user, vocabularies, etc...)
- write more tests.

But, most important, a feedback about the approach to solve this issue would be very useful.

Berdir’s picture

Status: Needs review » Needs work
+++ b/core/modules/language/language.moduleundefined
@@ -200,6 +200,150 @@ function language_process_language_select($element) {
+  // Some other module may want to alter the available options. For example, the
+  // on the vocabulary edit form, we may want to add the option that the term

The "the" on the first line is to much: "..., the on the..."

+++ b/core/modules/language/language.moduleundefined
@@ -200,6 +200,150 @@ function language_process_language_select($element) {
+  // Add a new submit handler, but only if it was not already added to the form.
+  if (!isset($form['#submit'])) {
+    $form['#submit'] = array();
+  }
+  if (!isset($form['#language_configation_submit_added'])) {
+    array_unshift($form['#submit'], 'language_configuration_element_submit');
+    $form['#language_configation_submit_added'] = TRUE;

I'm wondering if this could become a problem.

In case we have multiple submit buttons, we often use submit-level submit callbacks, which will skip the global ones, that could become a nasty problem.

There is only #element_validate and no #element_submit, unfortunately.

Not sure how to handle that, maybe we can just document that if you don't use global submit handlers, then you need to call the submit callback yourself in your submit function?

+++ b/core/modules/language/language.moduleundefined
@@ -200,6 +200,150 @@ function language_process_language_select($element) {
+ * @param array $entity_type
+ *   An array that contains information about the entity type (and bundle) for
+ *   which the settings are saved. It can have the following keys:
+ *   - entity_type: the type of the entity
+ *   - bundle: if the setting is attached to a bundle of an entity type, then
+ *   this should be specified by this value.

Is there a reason why we can't split this up into two arguments? $entity_type is everywhere else a string , it's quite confusing that it's an aray with type and bundle in this case.

+++ b/core/modules/language/language.moduleundefined
@@ -200,6 +200,150 @@ function language_process_language_select($element) {
+  $variable_root_name = language_get_configuration_variable_root_name($entity_type);
+  variable_set($variable_root_name . '_language', $values['language']);
+  variable_set($variable_root_name . '_language_hidden', $values['language_hidden']);

I think we shouldn't add new code that relies on variable_*() functions, this should use config() instead. Can be very similar otherwise I think, it's just the config($name) that is dynamic, with the fixed actual setting names.

One thing that is also missing is the handling of deleted bundles, I think there is a hook for that. You should implement that and delete the corresponding config.

I'm not sure what the long-term bundle plans are, I know that e.g. node type will become a configurable thingy and that is still heavily discussed, not sure if there will be any standardization. I'm for example wondering if it makes sense to add this to the global entity-bundle configuration file/thingy or if it should be separate. You might want to talk with @timplunkett, @sun or someone else that is working on the configurable thingy stuff.

webflo’s picture

+++ b/core/modules/language/language.moduleundefined
@@ -200,6 +200,150 @@ function language_process_language_select($element) {
+    'language_hidden' => variable_get($variable_root_name . '_language_hidden', ''),

A empty string as default value? I think a boolean makes more sense.

+++ b/core/modules/language/language.moduleundefined
@@ -200,6 +200,150 @@ function language_process_language_select($element) {
+function language_get_configuration_variable_root_name($entity_type) {
+  $root_name = 'language_configuration';
+  if (isset($entity_types['entity_type'])) {
+    $root_name .= '_' . $entity_types['entity_type'];
+  }
+  if (isset($entity_types['bundle'])) {
+    $root_name .= '_' . $entity_types['bundle'];
+  }
+  return $root_name;
+}

This can´t work. $entity_types is undefined. Its $entity_type.

vasi1186’s picture

In case we have multiple submit buttons, we often use submit-level submit callbacks, which will skip the global ones, that could become a nasty problem.

There is only #element_validate and no #element_submit, unfortunately.

Yes, because there is no #element_submit I had to add the submit callback on the form itself. What do you think the impact would be to introduce an #element_submit handler in the form API. Was there a special reason why, when the #element_validate was introduced, the #element_submit was not?

Is there a reason why we can't split this up into two arguments? $entity_type is everywhere else a string , it's quite confusing that it's an aray with type and bundle in this case.

There was not actually a particular reason for that, just thought that it would be better to have the parameters more compact. Probably the name of the parameter should be different and not $entity_type. Also, it could happen that the number of the parameters can grow (although in this particular case, it is more probable that it will not happen).

For the variable_*() function, it is true, I will just replace them.

vasi1186’s picture

Status: Needs work » Needs review
FileSize
3.6 KB
9.76 KB

Attached a patch that solves the issues in #11, and some of the issues from #9:
- replaces the variable_*() calls with config() calls.
- removes the additional "the" in a comment.

Still pending:
- the submit handler and the parameter issues from #9.

Berdir’s picture

Not sure about #element_submit, the problem is that #element_validate is relatively easy to implement as we already need to go through the form recursively anyway for the validation, but we'd need to do that again to execute the submit handlers once the validation is done and successful. Alternatively keep a reference to them somewhere globally and then loop through that. Would definitely be a separate issue to add support for that.

About the parameter functions, I think that language_save($entity_type, $bundle, $values) is much easier to understand instead of trying to bring it into a single variable. And as said before, it's possible that there will be some sort of standardization and there will be some sort of object representation for a bundle, that would make things easier.

vasi1186’s picture

Replaced the $entity_type array() with $entity_type and $bundle strings.

Status: Needs review » Needs work

The last submitted patch, language_configuration_1739928_14.patch, failed testing.

vasi1186’s picture

Attached a patch that should pass the tests.

vasi1186’s picture

Status: Needs work » Needs review
LoMo’s picture

Status: Needs review » Needs work

I'm working on reviewing this patch and have started by just giving it a quick read-through. It applies cleanly and looks good so far. I'd only point out a couple minor typos:
1) "configation" should be "configuration" ($form['#language_configation_submit_added']).

+++ b/core/modules/language/language.module
@@ -200,6 +200,142 @@ function language_process_language_select($element) {
+  if (!isset($form['#language_configation_submit_added'])) {
+    array_unshift($form['#submit'], 'language_configuration_element_submit');
+    $form['#language_configation_submit_added'] = TRUE;
+  }
+  return $element;

2) "Returns the root name…"

+/**
+ * Returns that root name of the variables used to store the configuration.
+ *

Now I'll continue to work on testing functionality. :-)

webflo’s picture

Status: Needs work » Needs review
FileSize
439 bytes
9.47 KB

Fixed the typos.

vasi1186’s picture

Attached a patch that adds the UI for the default language configuration of taxonomy terms. This can be checked by visiting the edit page of a vocabulary. Also, uses the new form element for the node type edit page.

YesCT’s picture

The nice ui is already in place on content types.

This could make it reusable on vocabularies, files, and other entities.
In core, we have vocabularies.

When this works, someone could implement re-using it on files and other entities in contrib (or if files gets into core, etc.)

So for now, in the ui, I'll look at:

all the places the patch will be changing things in the ui

content type edit
/#overlay=admin/structure/types/manage/article
controls defaults when making a node like
#overlay=node/add/article

edit vocabulary
/#overlay=admin/structure/taxonomy/tags/edit
controls defaults when make a term
/#overlay=admin/structure/taxonomy/tags/add

vasi1186’s picture

A new version that actually applies the settings from the edit vocabulary page into the term add/edit form.

Status: Needs review » Needs work

The last submitted patch, language_configuration_1739928_22.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review

when I save a content type language change I get a red error message

Notice: Undefined index: node_type_language_default in translation_node_type_language_translation_enabled_validate() (line 139 of core/modules/translation/translation.module).

fago’s picture

Status: Needs review » Needs work
+++ b/core/modules/language/language.module
@@ -200,6 +200,204 @@ function language_process_language_select($element) {
+    '#process' => array('language_configuration_process'),

language_configuration_element_process() would be a little better self-speaking function name I think.

+++ b/core/modules/language/language.module
@@ -200,6 +200,204 @@ function language_process_language_select($element) {
+  $lang_options = array();

We should not do abbreviations like this. Use the full wording.

+++ b/core/modules/language/language.module
@@ -200,6 +200,204 @@ function language_process_language_select($element) {
+  // Some other module may want to alter the available options. For example, on
+  // the vocabulary edit form, we may want to add the option that the term of
+  // that vocabulary have as default language the language of the entity to
+  // which they are assigned.
+  drupal_alter('language_configuration_element_options', $lang_options, $element['#config']);

I don't think we want to have a drupal_alter() for that, it's something that just the caller should be able to take care of. Thus I'd suggest having an #options parameters that gets populated by the element info defaults. Then have a function for getting them so it's easier to override: e.g.
#options => array(
'my-stuff' => 'value',
) + language_configuration_element_default_options(),

+++ b/core/modules/language/language.module
@@ -200,6 +200,204 @@ function language_process_language_select($element) {
+  if (!isset($form['#language_configuration_submit_added'])) {
+    array_unshift($form['#submit'], 'language_configuration_element_submit');
+    $form['#language_configuration_submit_added'] = TRUE;
+  }

Personally, I'd prefer not having that kind of magic and just add in your submit handler manually. That makes it more obvious how this works.

+++ b/core/modules/language/language.module
@@ -200,6 +200,204 @@ function language_process_language_select($element) {
+      language_save_configuration($values['entity_type'], $values['bundle'],  $form_state['values'][$element_name]);

save configuration about what? I think we should come up with a more-specific name here. Also, I don't know how we plan to handle saving that. I guess it should all go via the configurable save's routine independently whether it's stored to multiple cmi files or not. But I guess we could go with that for now and revisit it once the bundle objects become converted to configurables.

+++ b/core/modules/language/language.module
@@ -200,6 +200,204 @@ function language_process_language_select($element) {
+ * @param string $entity_type
+ *   A string representing the entity type.
+ *
+ * @param string $bundle

There should not be a new line between the @param lines.

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.php
@@ -97,12 +97,13 @@ class NodeFormController extends EntityFormController {
+      '#access' => !is_null($language_configuration['language_hidden']) && !$language_configuration['language_hidden'],

We use isset() instead of is_null()-

+++ b/core/modules/taxonomy/taxonomy.admin.inc
@@ -522,7 +521,8 @@ function theme_taxonomy_overview_terms($variables) {
-  $term = entity_create('taxonomy_term', array('vid' => $vocabulary->vid, 'vocabulary_machine_name' => $vocabulary->machine_name));
+  $lang_code = module_invoke('language', 'get_default_langcode', 'vocabulary', $vocabulary->machine_name);
+  $term = entity_create('taxonomy_term', array('vid' => $vocabulary->vid, 'vocabulary_machine_name' => $vocabulary->machine_name, 'langcode' => $lang_code));

Why does this use module_invoke()?

YesCT’s picture

looked at all the places the patch will be changing things in the ui #21

Before patch Vocab edit page
ui-before-vocab-edit-2012-08-25_1730.png

Before patch Term add page
ui-before-term-edit-2012-08-25_1731.png

testing.

from clean drupal install
drush -y en translation language locale
apply patch from #22
enable new language, french
/#overlay=admin/config/regional/language

edit content type and uncheck hide language selector

look at node add or edit
these are still the same as before the patch
ok

edit vocab and save

a little weird to see:
Language selector
and then in a field set, another Language selector

The first is the language of the vocab.
The second one, a field set grouped together of a language select and the hide checkbox is to set the defaults for the terms.
This is correct and ok, but was just a bit startling when I noticed two languages.
ok
ui-after-vocab-edit-2012-08-25_1740.png

look at term add

a bit wierd because since I know languages choices are possible I sort of feel like I need to know what language I should be writing in. But Schnitzel explained to me, that this is ok for now. There is another effort to redo the node edit page conpletely, and it might have an option to have something on the side like: "Information about this node. Language: French (forced)." And if that is good, could have a config option to show that on a term edit. And there are a lot of cases where the person adding or editing a term does not need to see the forced language value.
ok
ui-after-term-add-2012-08-25_1741.png

edit vocab and uncheck hide language selector, look at term add

ok
ui-after-term-add-with-lang-2012-08-25_1811.png

Side note: if there was a contrib module that allowed permissions by role (say) to restrict the languages a user could select, and the hide was unchecked, then it would be a bit weird that they would see a select with only one choice.

YesCT’s picture

Gabor pointed out that #1314250: Allow filtering/configuration of which languages apply to what (UI, nodes, files, etc) is related to my note about restricting what languages show in the language selector.

Bojhan’s picture

Alright so a short review:

Fieldset label, its a bit weird to duplicate label information with the field just below it. So lets just rename this to "Language" - unless this is collapsable.

Site default language, needs additional information what that language is; "Site default language (English)".

The label "Hide language selector" is overly technical, the term selector is too technical and should be selection. In addition to that we need to add a description, where this selection is done.

YesCT’s picture

Bojhan suggest in person for " In addition to that we need to add a description, where this selection is done."

To say:
... on creating and editing.

How about instead of
Hide language selector
or
Hide language selection

use:
Hide language selecting.

vasi1186’s picture

Attached a patch that should solve the issues in #25 and implement the suggestions from #28.

Why does this use module_invoke()?

@fago: the problem is that the language module is not a required core module, so using any direct call to a function in the language module, without this module being installed, will generate a fatal error. To avoid this, I used the module_invoke() solution. The other one would be to wrap that code inside a module_exists() check (which I did in this patch). If you have some other approach on how to solve this, just let me know.

Status: Needs review » Needs work

The last submitted patch, language_configuration_1739928_29.patch, failed testing.

YesCT’s picture

patch from #30

incorporates Bohjan feedback from #28

content type edit
content type edit

content type edit with dropdown
content type edit with dropdown

vocabulary edit
vocabulary edit

vocabulary edit with drop down
vocabulary edit with drop down

content piece (node) add
node add

content piece (node) add with drop down
content piece add (node) with drop down

Bojhan’s picture

Assigned: vasi1186 » Unassigned
Status: Needs work » Needs review

Looks good to me! RTBC from a UX perspective.

attiks’s picture

Assigned: vasi1186 » Unassigned

Fatal error: Call to a member function label() on a non-object in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/path/lib/Drupal/path/Tests/PathLanguageTest.php on line 89
FATAL Drupal\path\Tests\PathLanguageTest: test runner returned a non-zero error code (255).
- Found database prefix 'simpletest618472' for test ID 335.
[26-Aug-2012 09:28:17] PHP Fatal error: Call to a member function label() on a non-object in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/path/lib/Drupal/path/Tests/PathLanguageTest.php on line 89

YesCT’s picture

Status: Needs review » Needs work

Needs fix for problem in #34 and #24

vasi1186’s picture

Status: Needs work » Needs review
FileSize
12.29 KB
37.5 KB

Attached a new patch that should fix #24 and hopefully #34. Also, makes some changes in some tests to use the new form element on the node type edit pages.

Status: Needs review » Needs work

The last submitted patch, language_configuration_1739928_36.patch, failed testing.

YesCT’s picture

My testing steps in #26 were in the wrong order. Should be:

apply most recent patch
from clean drupal install
drush -y en translation language locale
enable new language, french
/#overlay=admin/config/regional/language

vasi1186’s picture

Attached a patch that tries to solve the #36.

vasi1186’s picture

Status: Needs work » Needs review
vasi1186’s picture

This patch probably needs some more Tests. Should there be a follow-up issue for that, or should I implement them in this patch (which is starting to get pretty big...)

Gábor Hojtsy’s picture

@vasi1186: The node type functionality should have test coverage already. Similar coverage for terms would be good to ensure that this works across. I'm not sure we can viably argue it being committed without unfortunately :/

vasi1186’s picture

Ok, I'll do that then.

vasi1186’s picture

Status: Needs review » Needs work
vasi1186’s picture

Attached a patch that contains also some tests.

vasi1186’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, language_configuration_1739928_45.patch, failed testing.

vasi1186’s picture

Let's see if the test bot returns green now.

vasi1186’s picture

Status: Needs work » Needs review
plach’s picture

Status: Needs review » Needs work

This looks pretty solid, great work!

Here is my code review, I didn't read the full thread, so sorry if any of the following issues have already been discussed. Didn't test the patch yet.

+++ b/core/modules/language/language.module
@@ -200,6 +199,216 @@ function language_process_language_select($element) {
+  if (!isset($element['#options'])) {
+    $element['#options'] = array();
+  }
...
+  // This is done to avoid a form validation error that checks that, if the
+  // element has an #options key, then the value submitted must be found in the
+  // #options array.
+  unset($element['#options']);

This code looks a bit confusing at first sight: we may want to introduce a new $option variable instead of initializing the '#options' key and unsetting it short afterwards. Something like:

<?php
  $options = !isset($element['#options']) ? $element['#options'] : array();
  // Avoid validation failure since we are moving the '#options' key in the nested 'language' select element.
  unset($element['#options']);
?>
+++ b/core/modules/language/language.module
@@ -200,6 +199,216 @@ function language_process_language_select($element) {
+  // @todo: #cofig is maybe not a very good key name...
+  if (!isset($form['#language_configuration_elements'])) {
+    $form['#language_configuration_elements'] = array();
+  }
+  $form['#language_configuration_elements'] += array(
+    $element['#name'] => array(
+      'entity_type' => $element['#config']['entity_type'],
+      'bundle' => $element['#config']['bundle'],
+    ),
+  );

This information should rather go into the form state. This use of the form array to store data is deprecated. According to the best practices we should introduce a form state key named after the module the code belongs to and nest all the needed info there. Maybe $form_state['language']['entity_type'] and the same for bundles?

+++ b/core/modules/language/language.module
@@ -200,6 +199,216 @@ function language_process_language_select($element) {
+ *   An array with values to be saved. It has the following keys:

Missing "the" before values. Can we slightly reword it?

"An array holding the values to be saved having the following keys:"

+++ b/core/modules/language/language.module
@@ -200,6 +199,216 @@ function language_process_language_select($element) {
+ *   - language: the language code.
...
+ *   - language: the language code.
...
+  return $configuration['language'];

If we are dealing with a language code, the key name should be langcode.

+++ b/core/modules/language/language.module
@@ -200,6 +199,216 @@ function language_process_language_select($element) {
+ * Based on the entity type (and if needed, bundle), the variables used to store
+ * the configuration will have a common root name.

The bundle parameter is not optional, the text in parenthesis is a bit confusing: is the bundle required or not? (looking at the code I'd say it is)

+++ b/core/modules/language/language.module
@@ -200,6 +199,216 @@ function language_process_language_select($element) {
+  return 'language_default_configuration.' . $entity_type . '.' . $bundle;

Wouldn't it make sense to store everything under the main language settings, using the language. prefix?

language.default_configuration.$entity_type.$bundle

+++ b/core/modules/language/language.module
@@ -200,6 +199,216 @@ function language_process_language_select($element) {
+      global $user;

We should use the drupal_container() here. I guess this should solve also the @todo in the tests below.

+++ b/core/modules/language/language.module
@@ -200,6 +199,216 @@ function language_process_language_select($element) {
+  // If we did not found a default value so far, invoke all other modules that
+  // may provide a special default language.
+  $default_value = module_invoke_all('language_default_langcode', $entity_type, $bundle, $configuration);
+  $default_value = array_shift($default_value);
+  if ($default_value) {
+    return $default_value;
+  }

Not sure whether we actually need a hook here: perhaps it would be enough to leverage plugins to have custom default providers here and just go with the selected one. Can totally be a follow-up, however, but if we go this way I'd just skip the hook for now.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/VocabularyFormController.php
@@ -72,6 +86,12 @@ class VocabularyFormController extends EntityFormController {
+      // the submit buttons has custom submit handlers...

Please remove the trailing ellipsis, we don't use it in comments.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/VocabularyFormController.php
@@ -100,6 +120,23 @@ class VocabularyFormController extends EntityFormController {
+   *

Unneeded empty line, should be removed.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/VocabularyFormController.php
@@ -100,6 +120,23 @@ class VocabularyFormController extends EntityFormController {
+    // Because the bundle is not yet known and moreover, it can be changed for a
+    // vocabulary, we have to also update the language_configuration_elements in
+    // order to have the correct bundle value.

The first sentence is a bit convolute, what about something like: "Since the machine name is not known yet, and it can be changed anytime, we also have to ..."

vasi1186’s picture

@plach: thank you very much for the review, I will try to solve them as soon as possible, but I think I will be able to do this only on Sunday evening, I will be pretty much offline until then.

vasi1186’s picture

Status: Needs work » Needs review
FileSize
49.92 KB
22.84 KB

Attached a patch that should solve the issues in #50, except this one:

We should use the drupal_container() here. I guess this should solve also the @todo in the tests below.

Is there any example in core where drupal_container() is used instead of global $user? I still see lots of places in drupal core where "global $user" is used.

Status: Needs review » Needs work

The last submitted patch, language_configuration_1739928_52.patch, failed testing.

vasi1186’s picture

Let's see if this patch will pass the tests.

vasi1186’s picture

Status: Needs work » Needs review
YesCT’s picture

Patch in #54 looks good afaik with regards to coding style

Applies to clean 8.x:
something like...
git branch 8.x
git clean -f -x
git clean -f -d
rm -rf some stuff
git checkout the stuff
git reset --hard
git pull --rebase
git branch generalize-lui-1739928
git checkout generalize-lui-1739928
curl -O http://drupal.org/files/language_configuration_1739928_54.patch
git apply language_configuration_1739928_54.patch
(install d8... probably a drush command to do that)
drush -y en translation language locale
drush cc all
add a language at /#overlay=admin/config/regional/language

edit article language settings:
some weirdness with text:
an extra space near commas (might not be a result of this patch)
and missing "current interface language" (or site default language, or whatever that default value is) when compared to the screen shot in the issue summary (http://drupal.org/files/LanguageSettings.png)
some-weirdness-2012-09-04_0247.png

Vocabulary edit still looks good:
vocab-edit-looks-good-2012-09-04_0256.png
Term and node edit still looks good.

I compared it to a clean install without the patch...
505 git checkout 8.x
506 git branch -D generalize-lui-1739928
507 git reset --hard
508 git pull --rebase
509 git status
510 git rm -r --cached
511 git clean -f -x
512 git clean -f -d
513 sudo rm -rf sites/default
514 git checkout -- sites/default
515 git status
516 history
517 git pull --rebase
(install it)
518 drush -y en translation language locale
519 drush cc all
(add a language)

and the weird space before (some of the) commas is still there, so not this patch's problem

But the missing default value looks like it's a result of this patch.
without-patch-shows-default-language-2012-09-04_0321.png

YesCT’s picture

Status: Needs work » Needs review
FileSize
859 bytes
49.92 KB
45.66 KB

in irc vasi helped me find the place in the js that was responsible for the default setting showing.

rerolled. shows the setting again.
with-langcode-js-change-2012-09-04_0406.png

YesCT’s picture

Issue summary: View changes

Updated issue summary.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Discussed on IRC that @vasi1186 would look into the user preferred language testing and eliminate the todo from the test.

vasi1186’s picture

Status: Needs work » Needs review
FileSize
50.84 KB
3.81 KB

Attached a new patch that also tests the user preferred language.

YesCT’s picture

Status: Needs review » Needs work
+++ b/core/modules/language/language.moduleundefined
@@ -200,6 +199,206 @@ function language_process_language_select($element) {
+  // Add the entity type and bundle information to the form if they are set.
+  // They will be used, in the submit handler, to generate the names of the
+  // variables that will store the settings.
+  // @todo: #cofig is maybe not a very good key name...
+  if (!isset($form_state['language'])) {
+    $form_state['language'] = array();
+  }
+  $form_state['language'] += array(
+    $element['#name'] => array(
+      'entity_type' => $element['#config']['entity_type'],
+      'bundle' => $element['#config']['bundle'],
+    ),
+  );

We need a different key name than config?

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageConfigurationElementTest.phpundefined
@@ -0,0 +1,113 @@
+    // @todo: Implement a follow-up for this feature, after
+    // https://drupal.org/node/1739928 is commited.

Maybe we should create the issue now, and mark it postponed on this one (this issue is https://drupal.org/node/1739928). So, I dont think we need this @todo in the patch.

YesCT’s picture

Status: Needs work » Needs review

Also needs someone to look in detail to do a code review.

YesCT’s picture

Issue summary: View changes

Updated issue summary.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

@YesCT/@vasi1186: indeed, the second @todo should have an issue opened and the commented code removed from the patch. We don't put @todo's like that in the code usually, we open and track followup issues instead :)

TransitionKojo’s picture

TransitionKojo’s picture

I applied the patch from #59, then removed the commented code as suggested in #62. That code was the source of the new follow-up issue #1775522: Create mechanism to add custom default values for the Language Config element

TransitionKojo’s picture

Status: Needs work » Needs review
YesCT’s picture

this patch addresses the last todo:
+ // @todo: #cofig is maybe not a very good key name...

#config was part of an array that contained the entity type and the bundle information. According to #1 and #4 and verified with vasi in irc, the purpose was to be the information to uniquely identify the entity. #config is replaced with #entity_information

plach’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
50.38 KB
3.46 KB

This looked almost RTBC to me so I just fixed the couple of issues below:

+++ b/core/modules/language/language.module
@@ -223,6 +222,206 @@ function language_process_language_select($element) {
+  // Avoid validation failure since we are moving the '#options' key in the
+  // nested 'language' select element.
+  unset($element['#options']);

Moved this up to improve readbility.

+++ b/core/modules/language/language.module
@@ -223,6 +222,206 @@ function language_process_language_select($element) {
+ *   - language: the language code.
...
+ *   - language: the language code.

These should be 'langcode'.

+++ b/core/modules/node/node.pages.inc
@@ -89,11 +89,12 @@ function node_add($node_type) {
+  $lang_code = module_invoke('language', 'get_default_langcode', 'node', $type);

This should be $langcode.

Setting to RTBC since I just performed minor code clean-ups.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, language_configuration-1739928-67.patch, failed testing.

vasi1186’s picture

Assigned: Unassigned » vasi1186

A new test class seems to use old variables, I will re-roll the patch.

vasi1186’s picture

Status: Needs work » Needs review
FileSize
1013 bytes
51.37 KB

Let's see now the test bot.

vasi1186’s picture

Status: Needs review » Reviewed & tested by the community

tests passed, so I think it can get back to RTBC

plach’s picture

Yes, thanks

catch’s picture

Assigned: vasi1186 » gdd
+++ b/core/modules/language/language.moduleundefined
@@ -223,6 +222,207 @@ function language_process_language_select($element) {
+function language_get_default_configuration($entity_type, $bundle) {
+  $configuration = config('language.settings')->get(language_get_default_configuration_settings_path($entity_type, $bundle));
+  if (is_null($configuration)) {
+    $configuration = array();
+  }
+  $configuration += array('langcode' => 'site_default', 'language_hidden' => TRUE);
+  return $configuration;

This makes me uncomfortable. We're dynamically storing information about entity types and bundles in CMI here, which is the first place that's going to happen. But at least bundles are likely to become ConfigEntity instances at some point (whether a generic Bundle class or node types. etc. individually).

So, if I want to export a node type, I have no way to know that there is a language.settings config object which is storing information about them somewhere. This is an issue that hasn't been resolved in CMI yet, so it doesn't block this patch going in, but I'd really, really like a @todo pointing to a concrete issue that will try to deal with this so we can revisit it later.

+++ b/core/modules/node/node.installundefined
@@ -601,6 +600,21 @@ function node_update_8004() {
 /**
+ * Move the language default values to config.
+ */
+function node_update_8005() {
+  $types = db_query('SELECT type FROM {node_type}')->fetchCol();
+  foreach ($types as $type) {
+    $language_default = update_variable_get('node_type_language_default_' . $type, NULL);
+    $language_hidden = update_variable_get('node_type_language_hidden_' . $type, NULL);
+    if (isset($language_default) || isset($language_hidden)) {
+      $values = array('langcode' => $language_default, 'language_hidden' => $language_hidden);
+      config('language.settings')->set('language.default_configuration.node.' . $type, $values)->save();
+    }
+  }
+}

And this hunk shows the problem. Node type information is currently split between the {node_type} table, random variables (or config), and sometimes custom tables in contrib modules. Are we going to move more stuff out of the {node_type} table and into separate config silos? Or try to do something else entirely?

I'm assigning to heyrocker for an opinion and hopefully he'll know where this discussion is located.

Rest of the patch looks OK to me so leaving RTBC.

Gábor Hojtsy’s picture

Unless node types and vocabularies are converted to be pluggable in some way for config storage, this is not really possible to store there. I think that can happen independently and we can integrate the two seamlessly after feature freeze. However, applying this feature to vocabularies might be something we would not be able to do later, given the generalisation that we need to do here. So it does not sound like we should postpone this entirely on the introduction of extensibility on vocabularies and node types.

catch’s picture

Yes that's why I said this:

This is an issue that hasn't been resolved in CMI yet, so it doesn't block this patch going in, but I'd really, really like a @todo pointing to a concrete issue that will try to deal with this so we can revisit it later.

Berdir’s picture

I think the idea (one idea?) is to build this on top of property API once available. Basically, a module could define additional properties for the node type ConfigEntity and because the save() implementation of the config controller would rely on those properties, they would be saved together with the default configuration.

This would also mean that those additional properties would be documented so that you can check what configuration exists.

I don't think there is an issue for this already.

gdd’s picture

There is not an issue for this specifically, but it touches on a lot of other issues that have similar problems where we discuss pieces of config that extend other pieces of config. This has come up a lot in a variety of places in the Views work I know. I've asked swentel to chime in here since he knows the most specifically about what future plans are for bundles and nodes in CMI, and I need to read up on this in more detail personally.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

I can't tell if this is "needs work" for the @todo asked for in #73/75, or if it's "needs review" pending swentel's feedback, or both. I reviewed the patch and found some things (mostly nit-picks, lots of missing docs, a couple of things that probably could use investigation/explaining), so settling on "needs work."

+++ b/core/modules/language/language.module
@@ -223,6 +222,207 @@ function language_process_language_select($element) {
+    $language_options[$langcode] = $language->locked ? t('- @name -', array('@name' => $language->name)) : $language->name;

s it just me or are those flipped?
if it's locked, the put it in t() so it can be translated?
else print it directly?

+++ b/core/modules/language/language.module
@@ -223,6 +222,207 @@ function language_process_language_select($element) {
+function language_configuration_element_submit(&$form, &$form_state) {
...
+      language_save_default_configuration($values['entity_type'], $values['bundle'],  $form_state['values'][$element_name]);

Yay for API functions and calling them in form submit handlers, rather than making form submit handlers "API functions"! ;)

+++ b/core/modules/language/language.module
@@ -223,6 +222,207 @@ function language_process_language_select($element) {
+function language_save_default_configuration($entity_type, $bundle,  $values = array()) {

(tiny nitpick) There's an extra space before $values. Can be fixed on commit unless someone gets to it in the next re-roll.

+++ b/core/modules/language/language.module
@@ -223,6 +222,207 @@ function language_process_language_select($element) {
+  return 'language.default_configuration.' . $entity_type . '.' . $bundle;

Since these values are more or less coming in directly from $form_state['values'], should there be some sanitization here to ensure people aren't putting nonsense like "../../../../" in there?

+++ b/core/modules/language/language.module
@@ -223,6 +222,207 @@ function language_process_language_select($element) {
+ * @return string
+ *   The language code.
+ *
+ */
+function language_get_default_langcode($entity_type, $bundle) {

(minor nitpick) Extra newline after "The language code."

+++ b/core/modules/language/language.module
@@ -223,6 +222,207 @@ function language_process_language_select($element) {
+  // If we still not have a default value, just return the value stored in the
+  // configuration, it has to be an actual language code.

if we still "do" not have a default value, just return the value stored in the configuration";" it has to be an actual langcode.

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageConfigurationElementTest.php
@@ -0,0 +1,105 @@
+  public function testLanguageConfigurationElement() {

+++ b/core/modules/language/tests/language_elements_test/language_elements_test.module
@@ -0,0 +1,55 @@
+function language_elements_configuration_element() {
...
+function language_elements_configuration_element_test() {

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TermLanguageTest.php
@@ -75,4 +77,43 @@ class TermLanguageTest extends TaxonomyTestBase {
+  function testDefaultTermLanguage() {

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/VocabularyLanguageTest.php
@@ -45,7 +43,9 @@ class VocabularyLanguageTest extends TaxonomyTestBase {
+  function testVocabularyLanguage() {

@@ -70,4 +70,62 @@ class VocabularyLanguageTest extends TaxonomyTestBase {
+  function testVocabularyDefaultLanguageForTerms() {

Needs PHPDoc.

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageConfigurationElementTest.php
@@ -0,0 +1,105 @@
+    // Check that the settings have been saved.
...
+    // Check that the settings have been saved.
...
+    // Fixed language.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/VocabularyLanguageTest.php
@@ -70,4 +70,62 @@ class VocabularyLanguageTest extends TaxonomyTestBase {
+    // Check that the vocabulary was actually created.
...
+    // Check that the the old settings are empty.
...
+    // Check that we have the new settings.

(minor nit-pick) Comments should be prefaced by a newline for consistency.

+++ b/core/modules/node/content_types.inc
@@ -205,18 +192,16 @@ function node_type_form($form, &$form_state, $type = NULL) {
+    $language_configuration = language_get_default_configuration('node', $type->type);
+    $form['language']['language_configuration'] = array(
+      '#type' => 'language_configuration',
+      '#entity_information' => array(
+        'entity_type' => 'node',
+        'bundle' => $type->type,
+      ),
+      '#default_value' => $language_configuration,
     );
+    $form['#submit'][] = 'language_configuration_element_submit';

In taxonomy module, this was wrapped in an if (module_exists('language')) check. Why is it not here?

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/VocabularyLanguageTest.php
@@ -70,4 +70,62 @@ class VocabularyLanguageTest extends TaxonomyTestBase {
+    $this->drupalPost('admin/structure/taxonomy/' . $machine_name . '/edit', $edit, t('Save'));

(the line below this) Two "the"s.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/VocabularyFormController.php
@@ -46,10 +46,24 @@ class VocabularyFormController extends EntityFormController {
     $form['langcode'] = array(
       '#type' => 'language_select',
-      '#title' => t('Language'),
+      '#title' => t('Vocabulary language'),
       '#languages' => LANGUAGE_ALL,
       '#default_value' => $vocabulary->langcode,
     );
+    if (module_exists('language')) {

Why is $form['langcode'] not also wrapped in an if (module_exists('language')) check? Does it apply if that module is not enabled?

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/VocabularyFormController.php
@@ -72,6 +86,12 @@ class VocabularyFormController extends EntityFormController {
+      // the submit buttons has custom submit handlers.

either "buttons have" or "button has".. I think it's the latter.

13 days to next Drupal core point release.

vasi1186’s picture

Why is $form['langcode'] not also wrapped in an if (module_exists('language')) check? Does it apply if that module is not enabled?

The language_select form element works also without the language module being installed, just that in that case it acts like a simple value field, it does not appear in the form. Some doc for this new element: http://drupal.org/node/1749954

YesCT’s picture

to reroll #70 because of directory location changes
addressing the review is yet to come.

LoMo’s picture

Re #78

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/VocabularyFormController.php
@@ -72,6 +86,12 @@ class VocabularyFormController extends EntityFormController {
+      // the submit buttons has custom submit handlers.

either "buttons have" or "button has".. I think it's the latter.

While we are on nitpicks, shouldn't the "the" at the beginning of the sentence also start with a capital 'T'?
i.e. // The submit button has custom submit handlers.

YesCT’s picture

This patch fixes some formatting. It puts @todo's into the code to track some items from webchicks comments that need to be addressed. Also, the PHPDoc comments that were added, might need to be reworded to be more specific.

I'm not sure about always having a blank line before // comments. Is that really a consistant style?

YesCT’s picture

There are still things left to be fixed. But between my confusion with re-rolling it and getting it to reapply and just being tired, I'm posting this as what was done so far, and noting things still left to be done. Feel free to pick this up.

+++ b/core/modules/language/language.moduleundefined
@@ -206,7 +206,6 @@ function language_element_info_alter(&$type) {
- *

I think the blank should be there between the param and return.

+++ b/core/modules/language/language.moduleundefined
@@ -223,6 +224,211 @@ function language_process_language_select($element) {
+ * @param string $bundle
+ *   A string representing the bundle.
+ * @return array

Similar, I think the blank line should be there between the param and return.

+++ b/core/modules/language/language.moduleundefined
@@ -223,6 +224,211 @@ function language_process_language_select($element) {
+  // @todo: http://drupal.org/node/1783346 for unresolved CMI issue.

Noting the followup issue with regards to catch, heyrocker, Berdir and swentel.

+++ b/core/modules/language/language.moduleundefined
@@ -223,6 +224,211 @@ function language_process_language_select($element) {
+ * @param string $bundle
+ *   A string representing the bundle.
+ * @return string

Need a blank line?
( http://drupal.org/node/1354 )

+++ b/core/modules/language/language.moduleundefined
@@ -223,6 +224,211 @@ function language_process_language_select($element) {
+ * @param string $bundle
+ *   The bundle name.
+ * @return string

blank needed?

+++ b/core/modules/node/content_types.incundefined
@@ -205,18 +192,20 @@ function node_type_form($form, &$form_state, $type = NULL) {
+    // @todo: http://drupal.org/node/1739928#comment-6471438 ¶
+    // In taxonomy module, this was wrapped in an if (module_exists('language'))
+    // check. Why is it not here?

trailing white space.

And I'm not sure if the reason for not wrapping here is the same as in the other location.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/VocabularyLanguageTest.phpundefined
@@ -45,8 +43,14 @@ class VocabularyLanguageTest extends TaxonomyTestBase {
+   * Tests language settings for vocabularies.

check description for accuracy.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/VocabularyLanguageTest.phpundefined
@@ -70,4 +74,67 @@ class VocabularyLanguageTest extends TaxonomyTestBase {
+   * Tests term language settings for vocabulary terms are saved and updated.

check description for accuracy.

YesCT’s picture

Status: Needs work » Needs review

Let's see about the tests too.

Status: Needs review » Needs work

The last submitted patch, language_configuration-1739928-81.patch, failed testing.

YesCT’s picture

Crud. I was afraid of that.

YesCT’s picture

Status: Needs work » Needs review
FileSize
9.34 KB
2.45 KB
52.94 KB

This is in the most recent pull:

   if ($module_language_enabled) {
      if (!variable_get('node_type_language_hidden_' . $bundle->type, TRUE)) {
        $extra['node'][$bundle->type]['form']['language'] = array(
          'label' => t('Language'),
          'description' => $description,
          'weight' => 0,
        );
      } 
      $extra['node'][$bundle->type]['display']['language'] = array(
        'label' => t('Language'),
        'description' => $description,
        'weight' => 0,
        'visible' => FALSE,
      );
    }

I had to change that by hand to the following to get the patch to apply. I need to think more about if the change by hand was the right one, or have someone look at that for me.

    // Visibility of the ordering of the language selector is the same as on the
    // node/add form.
    if ($module_language_enabled) {
      $configuration = language_get_default_configuration('node', $bundle->type);
      if (!$configuration['language_hidden']) {
        $extra['node'][$bundle->type]['form']['language'] = array(
          'label' => t('Language'),
          'description' => $description,
          'weight' => 0,
        );
      }

Regarding the capital T in comment #81: it's ok when the surrounding lines can be seen:

   if (empty($form_state['confirm_delete'])) {
      $actions = parent::actions($form, $form_state);
      array_unshift($actions['delete']['#submit'], array($this, 'submit'));
      // Add the language configuration submit handler. This is needed because
      // the submit button has custom submit handlers.
      if (module_exists('language')) {
        array_unshift($actions['submit']['#submit'],'language_configuration_element_submit');
  

I fixed the blank lines, and the trailing whitespace.

I did an interdiff on the recent patches and also one to the last one that applied and passed the tests: 70.

YesCT’s picture

+++ b/core/modules/field/modules/text/lib/Drupal/text/Tests/TextTranslationTest.phpundefined

+++ b/core/modules/language/language.moduleundefined
+++ b/core/modules/language/language.moduleundefined


@@ -216,6 +216,8 @@ function language_process_language_select($element) {
   if (!isset($element['#options'])) {
     $element['#options'] = array();
     foreach (language_list($element['#languages']) as $langcode => $language) {
+      // @todo: http://drupal.org/node/1739928#comment-6471438
+      // Is this flipped? If it's locked, put it in t() so it can be translated?
       $element['#options'][$langcode] = $language->locked ? t('- @name -', array('@name' => $language->name)) : $language->name;
     }
   }

question needs looking at still.

+++ b/core/modules/language/language.moduleundefined
@@ -223,6 +225,214 @@ function language_process_language_select($element) {
+function language_get_default_configuration_settings_path($entity_type, $bundle) {
+  // @todo: http://drupal.org/node/1739928#comment-6471438
+  // Since these values are more or less coming in directly from
+  // $form_state['values'], should there be some sanitization here to ensure
+  // people aren't putting nonsense like "../../../../" in there?
+  return 'language.default_configuration.' . $entity_type . '.' . $bundle;
+}

question needs looking at still.

+++ b/core/modules/node/content_types.incundefined
@@ -205,18 +192,20 @@ function node_type_form($form, &$form_state, $type = NULL) {
+    // @todo: http://drupal.org/node/1739928#comment-6471438
+    // In taxonomy module, this was wrapped in an if (module_exists('language'))
+    // check. Why is it not here?
+    $language_configuration = language_get_default_configuration('node', $type->type);
+    $form['language']['language_configuration'] = array(
+      '#type' => 'language_configuration',
+      '#entity_information' => array(
+        'entity_type' => 'node',
+        'bundle' => $type->type,
+      ),
+      '#default_value' => $language_configuration,
     );

question needs looking at still.

YesCT’s picture

Here is the follow up #1783346: Determine how to identify and deploy related sets of configuration for heyrocker, catch, Berdir and swentel.

Status: Needs review » Needs work

The last submitted patch, language_configuration-1739928-87.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
FileSize
503 bytes
52.68 KB

I think I sorted out the merge better.

Status: Needs review » Needs work
Issue tags: -D8MI, -sprint, -language-content

The last submitted patch, language_configuration-1739928-91.patch, failed testing.

vasi1186’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +D8MI, +sprint, +language-content

The last submitted patch, language_configuration-1739928-91.patch, failed testing.

vasi1186’s picture

@YesCT : is the patch working on your local? On my local the test comes green...

YesCT’s picture

@vasi186 I tested it briefly manually to verify the ui still looked good, and it was ok. I did not "run the tests" as I'm not sure how to do that.

swentel’s picture

I'm totally fine that language configuration is saved into its separate file. It should not ever be inserted into the bundle file once we get there as I don't think that yml file should be dumping ground for anyone.

However, regarding discoverability for export purposes, maybe the naming of the configuration file could/should be changed.

The naming scheme in the fields CMI patch at this point is field.field.field_name and field.instance.entity_type.bundle.field_name. This way, you at least have a hint which instances belong to which entity type and bundle. Fields don't need that hint as it's not tied to a bundle or entity type at all.

If we'd apply this to this patch, you'd get language.default_configuration.entity_type.bundle
Yes, you will have more config files, but you have (already a visual one if you look into the config folder) a clue that this configuration is tied to an entity_type and bundle. Also, think of a the features use case, I then can create, say a feature news with the following configuration files:

- node.news.yml
- field.field.body.yml
- field.instance.node.news.body.yml
- language.default_configuration.node.news.yml

We could even change any cmi naming to start with entity.bundle in case *any* kind of config belongs to that namespace, so we'd get something like

entity_type.bundle.language.default_configuration
entity_type.bundle.field.instance.field_name
entity_type.bundle.node_type (which is kind of weird as the node_type is also in the bundle name of course, but it's just to get an idea).

The problem here is, at least as far as I can see, that we don't have mechanism to force this kind of naming schemes, it will probably be a convention, but who knows what contrib might do. So in the end, if I would change something here, then it is more config files.

Maybe heyrocker, alexpott or sun have more/better ideas here ?

LoMo’s picture

Re: #96.

I did not "run the tests" as I'm not sure how to do that.

Hi YesCT,

This is pretty easy:

  1. Go to "/admin/modules" and enable "Testing"
  2. /admin/config/development/testing
  3. Check the appropriate areas to test*
  4. Click run tests and wait...

* This patch affects so many modules it could take a while to be sure you've selected all the right tests; so it might be better to let ALL the core tests run (that would take a while, but you could take a nap or something ;-) )

vasi1186’s picture

Or you can just run the test that fails.

gdd’s picture

I agree with swentel in #97 that we should provide a separate file and not have modules inserting configuration into files owned by other modules.

As far as naming, I really prefer the convention that starts with entity_type.bundle. I think it is going to end up being more useful in the end and I believe it properly represents the information hierarchy that is in play with the data. It allows us to get everything associated with a bundle by querying for entity_type.bundle.* which is really convenient. It will be easier to clean up everything when a bundle gets deleted (although if a field type gets deleted then instances are still a pain in the ass to find, but I am guessing that is the rare use case, and in the end something is always going to be harder to find.)

So from the example above we would have

node.news.yml
field.field.body.yml
node.news.field.instance.body.yml
node.news.language.default_configuration.yml

Actually we might be able to do node.news.instance.*.yml? Not sure if there is a use case for instances other than fields.

Discussion at #1776076-100: Convert comment module configuration to CMI is covering similar ground.

YesCT’s picture

RE #95, 98, 99
@LoMo Thanks, that was way easy!
@vasi It's green on my local.

swentel’s picture

Also, this patch doesn't take into account when a bundle is renamed. e.g. rename page content type to page_2. When that happens the bundle name needs to be updated (or the configuration file renamed in case this patch switch to individual configuration files - which it should imo)

hook_node_type_update() should be implemented then.

plach’s picture

@swentel:

Does this mean the every module has to handle a rename? Isn't there a way to provide some magic anywhere?

swentel’s picture

There's no magic so far, also because CMI has no clue that where configuration for a bundle is stored. I'm not even sure whether that's a task for CMI and if so can be easily fixed anyway.

See http://api.drupal.org/api/drupal/modules%21field%21field.attach.inc/func... which is called by node_type_update to notify fields.

LoMo’s picture

It's possible that if you guys (YesCT / Vasi) are both getting green, while there are fails on the testing system, that that could mean there is something which isn't properly configured on the testing server. That's been the case in other tests I've seen (for client projects with Jenkins continuous integration) that were failing on the testing server but running "green" locally.

YesCT’s picture

@LoMo and @vasi1186 xjm and swentel and jthorson pointed out to me in irc that its a core random bug #1779638: Unexplained test failure in LocaleFileImportStatus->testBulkImportUpdateExisting() And that we should retest it till it doesn't randomly fail. :)

YesCT’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Status: Needs review » Needs work

Came back green, but still needs to take care of the bundle rename problem.

LoMo’s picture

Status: Needs work » Needs review

... we should retest it till it doesn't randomly fail. :)

Looks like it might be fixed now, but still putting back in for a re-test to help ensure that the "random failure" issues are resolved.

#91: language_configuration-1739928-91.patch queued for re-testing.

LoMo’s picture

Issue summary: View changes

deleted the mention about the $user testing for user preferred language, #59 addresses that test.

Gábor Hojtsy’s picture

Assigned: gdd » Unassigned

@vasi1186: can you work on the bundle rename problem? Or anybody else? Certainly not assigned to heyrocker anymore as far as I can see.

vasi1186’s picture

@Gabor: yes, I will check this tonight.

vasi1186’s picture

Assigned: Unassigned » vasi1186
vasi1186’s picture

Status: Needs review » Needs work
Gábor Hojtsy’s picture

Anybody can drag this through the finish line? Its close :/

vasi1186’s picture

Status: Needs work » Needs review
FileSize
4.29 KB
54.94 KB

Hi,

really sorry for my late, had a lot to do lately...

I attached the patch with the hook_node_type_update() implemented and some tests. Let's see if the tests come green.

Status: Needs review » Needs work
Issue tags: -D8MI, -sprint, -language-content

The last submitted patch, language_configuration-1739928-115.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +D8MI, +sprint, +language-content

The last submitted patch, language_configuration-1739928-115.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
FileSize
1.09 KB
54.94 KB

trying to reroll #115. I wasn't sure how to get an interdiff on a reroll. so I just did a diff on the two patches.

Status: Needs review » Needs work

The last submitted patch, language_configuration-1739928-119.patch, failed testing.

Gábor Hojtsy’s picture

The failure is on LanguageUpgradePathTest.php line 80, looking for a language selector.

vasi1186’s picture

yes, seems that somehow the upgrade from drupal 7 to 8 has some issues regarding if the language selector should be hidden or not. I think the issue may be somewhere in the node.install file, in the last update_N() hook, but not 100% sure yet...

Gábor Hojtsy’s picture

@vasi1186: are you working on this?

vasi1186’s picture

I checked it a few times, didn't find yet how to solve the issue, but I'm pretty sure it is because the old language settings are not updated correctly. I had a really busy period now, but I will try to check this out again today in the evening, if nobody else will do.

vasi1186’s picture

Status: Needs work » Needs review
FileSize
4.26 KB
54.55 KB

Attached a patch that should now pass the tests, and the interdiff between #91 and #125

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me, resolves all concerns raised so far! Thanks!

YesCT’s picture

Here are screen shots using the latest patch from #125 Everything still looking great.

Steps to test were:
514 cd d8-office
515 git checkout 8.x
516 git reset --hard
517 git pull --rebase
518 git reset --hard
519 git pull --rebase
520 curl -O http://drupal.org/files/language_configuration-1739928-125.patch
521 git checkout -b language_fallback_1776166-125
522 git apply language_configuration-1739928-125.patch
523 drush -y si --account-name=admin --account-pass=admin --site-name=d8-office-language_configuration-1739928-125 --db-url="mysql://root:root@localhost/d8-office"

then enabled translation locale and language
edited the article content type
added a language in the configuration
edited the content type again, also edited the vocabulary

gen-s01-contenttype-2012-10-12_2238.png

gen-s02-contenttype-select-2012-10-12_2240.png

gen-s03-contenttype-nondef-nonhidden-2012-10-12_2242.png

gen-s04-vocab-2012-10-12_2244.png

gen-s05-vocab-dropdown-2012-10-12_2249.png

YesCT’s picture

Some before shots to update the issue summary.

YesCT’s picture

Issue summary: View changes

adding to the summary the next step to move the issue forward

YesCT’s picture

Issue summary: View changes

Updated issue summary. with most recent screen shots and uses the issue summary template to try and make it easier to be committed

YesCT’s picture

Status: Reviewed & tested by the community » Needs work

I'll number these for ease of referring to later.

  1. +++ b/core/modules/language/language.moduleundefined
    @@ -226,6 +226,8 @@ function language_process_language_select($element) {
       if (!isset($element['#options'])) {
         $element['#options'] = array();
         foreach (language_list($element['#languages']) as $langcode => $language) {
    +      // @todo: http://drupal.org/node/1739928#comment-6471438
    +      // Is this flipped? If it's locked, put it in t() so it can be translated?
           $element['#options'][$langcode] = $language->locked ? t('- @name -', array('@name' => $language->name)) : $language->name;
         }
    

    This question from webchick in #78, and noted in #88 was not addressed yet. So I'm marking it needs work since I figure webchick will still have the question looking at this for commit again.

    +++ b/core/modules/language/language.moduleundefined
    @@ -233,6 +235,224 @@ function language_process_language_select($element) {
    +  $languages = language_list(LANGUAGE_ALL);
    +  foreach ($languages as $langcode => $language) {
    +    $language_options[$langcode] = $language->locked ? t('- @name -', array('@name' => $language->name)) : $language->name;
    +  }
    

    This might relate to the locked/flipped question above.

    +++ b/core/modules/node/content_types.incundefined
    @@ -206,19 +206,6 @@ function node_type_form($form, &$form_state, $type = NULL) {
    -      $lang_options[$langcode] = $language->locked ? t('- @name -', array('@name' => $language->name)) : $language->name;
    

    Or the locked/flipped question might be a problem with the previous code that we dont have to solve here. And can be a follow-up.

  2. +++ b/core/modules/language/language.moduleundefined
    @@ -233,6 +235,224 @@ function language_process_language_select($element) {
    +function language_get_default_configuration($entity_type, $bundle) {
    +  // @todo: http://drupal.org/node/1783346 for unresolved CMI issue.
    +  $configuration = config('language.settings')->get(language_get_default_configuration_settings_path($entity_type, $bundle));
    +  if (is_null($configuration)) {
    +    $configuration = array();
    +  }
    +  $configuration += array('langcode' => 'site_default', 'language_hidden' => TRUE);
    +  return $configuration;
    

    This todo should be fine. The follow-up issue #1783326: Clicking "reset" does nothing is opened, is being worked on, and is linked back to this issue.

  3. +++ b/core/modules/language/language.moduleundefined
    @@ -233,6 +235,224 @@ function language_process_language_select($element) {
    +function language_get_default_configuration_settings_path($entity_type, $bundle) {
    +  // @todo: http://drupal.org/node/1739928#comment-6471438
    +  // Since these values are more or less coming in directly from
    +  // $form_state['values'], should there be some sanitization here to ensure
    +  // people aren't putting nonsense like "../../../../" in there?
    +  return $entity_type . '.' . $bundle . '.language.default_configuration';
    +}
    

    Similar to the first todo, this sanitization question from webchick in #78, and noted in #88 was not addressed yet.

  4. +++ b/core/modules/node/content_types.incundefined
    @@ -226,18 +213,20 @@ function node_type_form($form, &$form_state, $type = NULL) {
    +    // @todo: http://drupal.org/node/1739928#comment-6471438
    +    // In taxonomy module, this was wrapped in an if (module_exists('language'))
    +    // check. Why is it not here?
    +    $language_configuration = language_get_default_configuration('node', $type->type);
    +    $form['language']['language_configuration'] = array(
    +      '#type' => 'language_configuration',
    +      '#entity_information' => array(
    +        'entity_type' => 'node',
    +        'bundle' => $type->type,
    +      ),
    +      '#default_value' => $language_configuration,
    

    Similar to the first todo, this module_exists wrapping question from webchick in #78, and noted in #88 was not addressed yet.

YesCT’s picture

Issue summary: View changes

Updated issue summary. added the before screen shot of the vocab edit

YesCT’s picture

#1810386: Create workflow to setup multilingual for entity types, bundles and fields is postponed on this issue. Adding that to this issue summary.

YesCT’s picture

Issue summary: View changes

Updated issue summary. with remaining task information about webchicks questions that still need to be addressed

Gábor Hojtsy’s picture

Assigned: vasi1186 » webchick

#129/1: the locked language thing is not at all a problem the code is correct, it is not flipped. Locked languages get a "- NAME -" wrapper where NAME is the language name, the wrapper itself is sent for translations so people can use language-specific wrappers as appropriate. Other languages don't get that wrapper. None of the language names are translated. All of the quoted code is correct and should not have a @todo.

#129/2: we agreed above resolving related sets of CMI config is an existing problem with no existing solution, and we have that issue for it. Should not hold this patch up.

#129/3: for this one, I looked through many call paths, and it indeed looks like in some cases, it might end up with direct form information (machine name to be specific); We should assume that is validated by the machine name input though; I don't see a path where this is called without the form layer? Any specific case you see that should be covered?

#129/4: it is *already* wrapped in a module_exists('language') wrapper, it just happens to be already there, upper in the code and not touched in the patch; it is visible in the patch though.

I think /1 and /4 @todo's are clearly bugos and should be removed. I'm looking for feedback for /3 from webchick (assigning to her for that) and /2 should not hold this patch up.

Gábor Hojtsy’s picture

Assigned: webchick » Unassigned

One more comment on #129/3: although it is called a CMI path, it is actually a nested CMI key that is being handled there, so ../../../ type of tricks will only make it retrieve/save silly data points, it will not go out of the CMI file that is hardwired in the code. Even if the machine name validation fails for some reason that is. @vasi1186 is working on an update.

vasi1186’s picture

Assigned: Unassigned » webchick
FileSize
5.03 KB
53.73 KB

Attached a new patch that addresses the issues specified in 129. So:
1. I think Gábor already explained pretty clear why this is like that, so I just removed the @todo from the code.

2. Just removed the @todo from the code.

3. I replaced everything that is not a number, a letter or an underscore, with an underscore. The side effect there is that, for example if you have this combination of variables: ("some type", "some bundle") and ("some_type", "some_bundle") they will return the same key... I think this should actually never happen in core, but if this feature is used by any contrib code, theoretically it can happen.

4. The code was already wrapped in a module_exists(), but that module_exists() call is issued before and does not appear in the patch. This is the entire code:

<?php
if (module_exists('language')) {
    $form['language'] = array(
      '#type' => 'fieldset',
      '#title' => t('Language settings'),
      '#collapsible' => TRUE,
      '#collapsed' => TRUE,
      '#group' => 'additional_settings',
    );

    $language_configuration = language_get_default_configuration('node', $type->type);
    $form['language']['language_configuration'] = array(
      '#type' => 'language_configuration',
      '#entity_information' => array(
        'entity_type' => 'node',
        'bundle' => $type->type,
      ),
      '#default_value' => $language_configuration,
    );
    $form['#submit'][] = 'language_configuration_element_submit';
  }
?>

So it contains also the Language fieldset.

Except 3), where I could still receive some feedback from other people, the other points should be ok.

vasi1186’s picture

Status: Needs work » Needs review
vasi1186’s picture

Had a typo in the previous patch...

vasi1186’s picture

Fixed another small code style issue.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Resolved the remaining concern that was deemed valid :) Should be back to RTBC.

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary. in irc xjm recommended issues postponed be marked in the summary of the dependency issue.

YesCT’s picture

The issue summary is up-to-date and looks like all concerns that have been brought up have been addressed.

Gábor Hojtsy’s picture

Dries’s picture

I haven't tried this patch yet, but I was wondering how we'd deal with language independent tags. For example, imagine I had a tag with name of my cat. It wouldn't necessarily be 'English'. It would be the same name in all languages. I expect that problem to be previously addressed but just making sure.

Otherwise this patch looks good. I will commit it shortly, unless catch or webchick beat me to it.

Gábor Hojtsy’s picture

@Dries: Language independence is one of the language options that you can assign to terms (as well as nodes, users, etc.). With this patch you can even make a vocabulary that only has such terms and set all of those terms to inherit the language independence from the vocabulary (and hide the UI so it automatically works). Magic :)

YesCT’s picture

@Dries does this look at the values in the drop down help clarify? From #127 http://drupal.org/files/gen-s05-vocab-dropdown-2012-10-12_2249.png

webchick’s picture

Title: Generalize language configuration on content types to apply to terms and other entities » Change notice: Generalize language configuration on content types to apply to terms and other entities
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

So all in all, I like this patch a lot.

One thing that worries me a bit though is that in order to "Language selection-ify" an entity, there's quite a bit of copy/paste code. And worse, this copy/paste code seems to be inconsistent between entities.

In the test form:

  $conf = language_get_default_configuration('some_custom_type', 'some_bundle');

  $form['lang_configuration'] = array(
    '#type' => 'language_configuration',
    '#entity_information' => array(
      'entity_type' => 'some_custom_type',
      'bundle' => 'some_bundle',
    ),
    '#default_value' => $conf,
  );
  $form['#submit'][] = 'language_configuration_element_submit';

In content types:

   if (module_exists('language')) {
     $form['language'] = array(
       '#type' => 'fieldset',
       '#title' => t('Language settings'),
       '#collapsed' => TRUE,
       '#group' => 'additional_settings',
     );

    $language_configuration = language_get_default_configuration('node', $type->type);
    $form['language']['language_configuration'] = array(
      '#type' => 'language_configuration',
      '#entity_information' => array(
        'entity_type' => 'node',
        'bundle' => $type->type,
      ),
      '#default_value' => $language_configuration,
     );
    $form['#submit'][] = 'language_configuration_element_submit';

In vocabularies:

    // $form['langcode'] is not wrapped in an if (module_exists('language'))
    // check because the language_select form element works also without the
    // language module being installed.
    // http://drupal.org/node/1749954 documents the new element.
     $form['langcode'] = array(
       '#type' => 'language_select',
       '#title' => t('Vocabulary language'),
       '#languages' => LANGUAGE_ALL,
       '#default_value' => $vocabulary->langcode,
     );
    if (module_exists('language')) {
      $form['default_terms_language'] = array(
        '#type' => 'fieldset',
        '#title' => t('Terms language'),
      );
      $form['default_terms_language']['default_language'] = array(
        '#type' => 'language_configuration',
        '#entity_information' => array(
          'entity_type' => 'vocabulary',
          'bundle' => $vocabulary->machine_name,
        ),
        '#default_value' => language_get_default_configuration('vocabulary', $vocabulary->machine_name),
      );
    }
...
      // Add the language configuration submit handler. This is needed because
      // the submit button has custom submit handlers.
      if (module_exists('language')) {
        array_unshift($actions['submit']['#submit'],'language_configuration_element_submit');
        array_unshift($actions['submit']['#submit'], array($this, 'languageConfigurationSubmit'));
      }

As you can see, there are slight variations between all of them. The one in the test module (presumably the simplest implementation) doesn't have a wrapping langcode fieldset, doesn't do a module_exists() check for language. Content types do add a fieldset, and then wrap both it and the field in a module_exists('language') check. However, vocabularies don't. They only do the module_exists() check on the language selector, then does shenanigans with the submit handler that none of the other places do (the comment here doesn't really make sense; node types have a custom submit handler too?).

Now, normally I wouldn't care about consistency pedantry as much (oh, who am I kidding? ;) Yes I would. ;)), but we have at least two more things in D8 we want to convert to entities, which are custom blocks (#1535868: Convert all blocks into plugins) and menu links (#916388: Convert menu links into entities), and possibly user profiles (#1668292: Move simplified Profile module into core). And I literally have no idea which of these 3 patterns is correct to use in those places (or even if they need them, since Comments for whatever reason seem like they don't). Furthermore, this isn't part of any Entity/EntityFormController interface, so nothing is going to bark at any module developer if they forget to add the selector to their config forms. Seems like a recipe for inconsistency.

So it really feels like we need... something... A helper function? An extension to EntityFormController? to make this easier for entity module developers to do the right thing here.

Now. All of that said, this is still an important patch for multilingual that encapsulates a lot of complexity and helps move the ball forward in other areas, so given that Dries's concerns were addressed and given that mine can be tackled in a follow-up, I've committed and pushed this to 8.x, with the addition of some missing PHPDoc on functions in core/modules/language/tests/language_elements_test/language_elements_test.module (please make sure this is done on all RTBC patches in the future; it's part of the documentation gate).

But let's please make sure that these questions are answered in the change notice and, if possible, a follow-up to make it easier for developers to incorporate this element into their modules.

plach’s picture

Totally agreed with #143. We need more consistency and (even) more ease to set things up. Ideally it should be possible to trigger all this stuff automatically but just providing some entity info key or stuff like that.

That said, awesome work, guys! This is really a cornerstone for entiyy language support!

webchick’s picture

Assigned: webchick » Unassigned

Taking this off my plate.

webchick’s picture

We're once again over the critical task threshold. Please try and find time to write up documentation for your API change ASAP so your fellow contributors' features aren't blocked. Thanks.

vasi1186’s picture

Status: Active » Needs review

The change notification can be checked here: http://drupal.org/node/1816456

webchick’s picture

Title: Change notice: Generalize language configuration on content types to apply to terms and other entities » Refined change notice: Generalize language configuration on content types to apply to terms and other entities
Priority: Critical » Normal
Status: Needs review » Needs work

Awesome work, this is definitely great as a start, and walks through the API well.

As discussed in IRC, one element that's missing though is a full working example for people upgrading their modules. vasi said he can work on that a bit later today.

Since the change notice exists, and this is just further refinement, downgrading to a "normal" task.

vasi1186’s picture

Priority: Normal » Critical
Status: Needs work » Needs review

Added the working example: http://drupal.org/node/1816456

Gábor Hojtsy’s picture

Category: task » feature
Priority: Critical » Normal
Status: Needs review » Fixed

Ok @vasi1186 just updated with a usage example which looks good to me.

Gábor Hojtsy’s picture

Issue tags: -sprint
Tor Arne Thune’s picture

Title: Refined change notice: Generalize language configuration on content types to apply to terms and other entities » Generalize language configuration on content types to apply to terms and other entities
Issue tags: -Needs change record
plach’s picture

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/VocabularyFormController.php
@@ -44,12 +44,31 @@ public function form(array $form, array &$form_state, EntityInterface $vocabular
+          'entity_type' => 'vocabulary',
+          'bundle' => $vocabulary->machine_name,
+        ),
+        '#default_value' => language_get_default_configuration('vocabulary', $vocabulary->machine_name),

Please clarify if this is by design, but this is the language configuration of taxonomy terms and it's using 'vocabulary' as entity type. This is pretty weird and is breaking things in the translation UI, see #1188388-154: Entity translation UI in core

vasi1186’s picture

The thing is that the language_configuration form element has to use an entity type and a bundle name. In case of taxonomy terms, they are entities, but they do not have entity types and bundles, as for example nodes have. So, the idea was to just use 'vocabulary' as the entity type, and the name of the taxonomy as the bundle. Basically, the goal of the (entity_type, bundle) combination is to uniquely identify the object on which the configuration applies. So, in the end, it can be actually anything, but probably in most of the cases will be the entity type and the bundle name, that's why they were also chosen as keys (there were some discussions in the beginning regarding these names). If using 'vocabulary' as the entity_type breaks something (can you also tell what is actually breaking?), then we can change them, the only thing is to find something that uniquely identifies the vocabulary (which in this case is its machine name).

andypost’s picture

So what's the initial language for vocabulary here #1809816: Drop remains of landcode in favour of language element ?

vasi1186’s picture

Actually, these settings apply on the taxonomy terms, not on vocabularies. So, you can specify the default language settings for the terms of a vocabulary (on the vocabulary edit page), not for the vocabulary itself. For the case you pointed out, there is no change that has to be made (unless we implement some feature to set the default language configuration for vocabularies).

plach’s picture

In case of taxonomy terms, they are entities, but they do not have entity types and bundles, as for example nodes have.

This is not correct: every entity type has to to explicitly declare itself just to be defined. Moreover taxonomy terms have vocuabulary machine names as bundles, see: taxonomy_entity_info().

Given that, I think we should be simply be using the 'taxonomy_term' entity type instead of the 'vocabulary' one here to make things work properly. Incidentally this is what the latest patches in the entity translation UI issue are doing and updated tests pass whitout problems.

vasi1186’s picture

@plach: yes, you're right, sorry, the vocabulary machine name is the bundle name (they are actually used for the bundle of the #entity_information). But that's actually not the most important thing. The important thing is to have the combination (entity_type, bundle_name) unique for every object on which this element is used. If using 'taxonomy_term' instead of 'vocabulary' it is really fine from my point of view.

Gábor Hojtsy’s picture

Should it be patched then? :)

vasi1186’s picture

Yes, I think we should create another issue for that?

plach’s picture

Assigned: Unassigned » plach

I can post an excerpt from the UI patch that already fixes this.

plach’s picture

Status: Fixed » Needs review
FileSize
5.28 KB

Here it is.

andypost’s picture

Vocabulary is an entity, so should have own language. After #1552396: Convert vocabularies into configuration vocabularies are should be configentity

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/VocabularyLanguageTest.phpundefined
@@ -95,7 +95,7 @@ function testVocabularyDefaultLanguageForTerms() {
-    $language_settings = language_get_default_configuration('vocabulary', $edit['machine_name']);
+    $language_settings = language_get_default_configuration('taxonomy_term', $edit['machine_name']);

@@ -111,7 +111,7 @@ function testVocabularyDefaultLanguageForTerms() {
-    $language_settings = language_get_default_configuration('vocabulary', $machine_name);
+    $language_settings = language_get_default_configuration('taxonomy_term', $machine_name);

@@ -133,7 +133,7 @@ function testVocabularyDefaultLanguageForTerms() {
-    $new_settings = language_get_default_configuration('vocabulary', $new_machine_name);
+    $new_settings = language_get_default_configuration('taxonomy_term', $new_machine_name);

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/VocabularyFormController.phpundefined
@@ -63,10 +63,10 @@ public function form(array $form, array &$form_state, EntityInterface $vocabular
-          'entity_type' => 'vocabulary',
+          'entity_type' => 'taxonomy_term',
...
-        '#default_value' => language_get_default_configuration('vocabulary', $vocabulary->machine_name),
+        '#default_value' => language_get_default_configuration('taxonomy_term', $vocabulary->machine_name),

@@ -132,7 +132,7 @@ public function languageConfigurationSubmit(array &$form, array &$form_state) {
-      language_clear_default_configuration('vocabulary', $vocabulary->machine_name);
+      language_clear_default_configuration('taxonomy_term', $vocabulary->machine_name);

This is a form of vocabulary entity itself. So in case of the form that adds new vocabulary (a bundle) for term it looks ugly to try to find a language for bundle withing none-existent terms.

plach’s picture

@andypost:

This is a form of vocabulary entity itself. So in case of the form that adds new vocabulary (a bundle) for term it looks ugly to try to find a language for bundle withing none-existent terms.

Not sure I get your point. Vocabularies are indeed entities and currently have their own language selector in the Vocabulary form. As you were correctly pointing out in #155, we need a default also for it. However we cannot obviously use the one in the Vocabulary form, since that one govern the taxonomy term language selector and being able to configure a default only after needing it sounds a bit silly.

I think we need a configuration at upper level. IMO a possibily easy way to fix this could be a global language configuration default in the language settings that could be inherited by every entity type not needing something specific, for instance all Configurable entities having a language selector could use it.

vasi1186’s picture

Assigned: plach » Unassigned
Status: Needs review » Fixed

Agree with plach, that element has to be used on an upper layer. For example: in the case of nodes, it is used on the node type edit form to manage the default settings of the node add form. For taxonomy term, the form is used on the vocabulary edit form to manage the default settings of the term add form. So, for the language of the vocabularies, there has to be some upper level where we could set this.

vasi1186’s picture

Status: Fixed » Needs review

accidentally changed the status before.

andypost’s picture

As I get it right, we need a some default language for/per bundle
- nodes: language defaults for each type managed at admin/structure/types/manage/[node_type]
- terms: language defaults for each vocabulary managed at admin/structure/taxonomy/[voc-name]/edit

Suppose the defaults should be language interface as top-level setting

plach’s picture

I think a global default is something that needs some discussion and deserves a dedicated issue. Can we RTBC this meanwhile?

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Sorry, I don't get that this defines a term language. Makes a lot of sense!!!

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/VocabularyFormController.phpundefined
@@ -63,10 +63,10 @@ public function form(array $form, array &$form_state, EntityInterface $vocabular
       $form['default_terms_language']['default_language'] = array(
         '#type' => 'language_configuration',
         '#entity_information' => array(
-          'entity_type' => 'vocabulary',
+          'entity_type' => 'taxonomy_term',
           'bundle' => $vocabulary->machine_name,
         ),
-        '#default_value' => language_get_default_configuration('vocabulary', $vocabulary->machine_name),
+        '#default_value' => language_get_default_configuration('taxonomy_term', $vocabulary->machine_name),

This context I missed...

After this commited we should re-open #1809816: Drop remains of landcode in favour of language element to discuss default language for vocabulary because it could be used in forum title for example #148145: "Forums" title is not localized

Gábor Hojtsy’s picture

Category: feature » bug
Issue tags: +sprint

Moving on sprint since this bug blocks #1188388: Entity translation UI in core.

vasi1186’s picture

The patch in #162 looks good to me.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed the follow-up.

Gábor Hojtsy’s picture

Category: bug » feature
Issue tags: -sprint

Thanks a lot!

Status: Fixed » Closed (fixed)

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

YesCT’s picture

Related to comment #9 and #100
Noticed #2014955: Deleted bundles do not have their language configuration deleted while working on #1945226: Add language selector on menus
Did we have tests for deleting config?

YesCT’s picture

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/VocabularyFormController.phpundefined
@@ -132,7 +132,7 @@ public function languageConfigurationSubmit(array &$form, array &$form_state) {
     // Delete the old language settings for the vocabulary, if the machine name
     // is changed.
     if ($vocabulary && isset($vocabulary->machine_name) && $vocabulary->machine_name != $form_state['values']['machine_name']) {
-      language_clear_default_configuration('vocabulary', $vocabulary->machine_name);
+      language_clear_default_configuration('taxonomy_term', $vocabulary->machine_name);
     }

Since I'm trying to add a test for #2014955: Deleted bundles do not have their language configuration deleted

I was looking for a test to see if the config was deleted when the bundle was. I found this test, which is checking if "old" config is deleted when something (machine name) changes. Not quite the same thing I was looking for.

But, while investigating, I changed the machine name of the tags vocab. And this is what is in the language settings file:

taxonomy_term:
  tags:
    language: {  }
  tagsnewname:
    language:
      default_configuration:
        langcode: site_default
        language_show: '0'
Gábor Hojtsy’s picture

Looks like we need an issue for machine name changes too? :)

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary to reflect that remaining webchick questions were answered and are no longer todo.