Problem/Motivation

The current language settings for node types has some limitations. The site builder wants to choose what happens when a content type has multilingual or not.

The language options when adding a node are:

  1. None
  2. Site default language
  3. Page language
  4. Default value
  5. User language (#40)

When configuring a content type it should be possible to set a default for and allow to pin the language option.

Proposed resolution

Image taken from #88

How to test

  1. Enable Content translation module
  2. Visit content type config page, e.g. admin/structure/types/manage/article
  3. Select 'Language settings' tab and set some settings.
  4. Next create an Article and see whether those settings apply.
  5. Check the translation tab and add a language.
  6. Try to translate the Article into the new language.
  7. Repeat the process to try all of the possible settings.
  8. ? more steps ?

Remaining tasks

Review these test cases (#116):
- tests the default initial language
- the default lock language option
- the site's default initial language option
- changing the initial language option

Still needed tests tests:
- update test
- user default initial language

Are there more tests needed?

Use entity_create according to #42.
This issue is not only about node types but should work in the end for all entity types. So we need a list of all related issues here.

User interface changes

tbd

API changes

tbd

Original report by dvinegla

GaborHojtsy clarified: #31 http://drupal.org/node/258785#comment-5602950

In you creates content type without language support, default language is assigned to it.

I think it must be '' (neutral) or FALSE

CommentFileSizeAuthor
#216 node-type-changelog.patch574 bytesGábor Hojtsy
#211 258785-211.patch30.02 KBSchnitzel
#211 interdiff-204-211.txt4.47 KBSchnitzel
#204 interdiff-202-204.txt2.13 KBSchnitzel
#204 258785-204.patch30.31 KBSchnitzel
#202 258785-202.patch30.27 KBSchnitzel
#202 interdiff-199-202.txt4.07 KBSchnitzel
#199 258785-199.patch28.31 KBSchnitzel
#199 interdiff-195-199_2.txt15.33 KBSchnitzel
#195 258785-195.patch27.23 KBSchnitzel
#191 258785-191.patch28.22 KBSchnitzel
#190 258785-190.patch28.09 KBSchnitzel
#187 258785-187.patch28.12 KBSchnitzel
#184 258785-184.patch81.97 KBSchnitzel
#181 258785-181.patch82.21 KBSchnitzel
#181 interdiff-179-181.txt1.89 KBSchnitzel
#179 258785-179.patch61.53 KBSchnitzel
#179 interdiff-174-179.txt21.72 KBSchnitzel
#177 258785-176.patch27.34 KBSchnitzel
#175 258785-175.patch30.12 KBSchnitzel
#174 258785-174.patch26.81 KBkbasarab
#174 interdiff.txt2.36 KBkbasarab
#167 258785-167-2.patch26.1 KBkbasarab
#167 interdiff.txt8.32 KBkbasarab
#163 258785-162.patch25.81 KBkbasarab
#161 258785-160.patch28.03 KBkbasarab
#158 258785-158.patch28.03 KBkbasarab
#156 258785-156-5.patch27.69 KBkbasarab
#156 interdiff.txt27.69 KBkbasarab
#153 interdiff.txt2.62 KBkbasarab
#152 258785-152.patch26.41 KBkbasarab
#152 interdiff.txt26.41 KBkbasarab
#151 25875-151.patch27.64 KBGábor Hojtsy
#151 interdiff.txt884 bytesGábor Hojtsy
#147 25875-147.patch27.59 KBkbasarab
#146 25875-reroll-only-146.patch27.08 KBkbasarab
#139 interdiff-137-139.txt1.06 KBDésiré
#139 258785-more_flexible_initial_language_on_content_types-139.patch27.84 KBDésiré
#139 258785-more_flexible_initial_language_on_content_types-drupalCreateNode-LANGUAGE_NOT_SPECIFIED-should_fail-139.patch27.52 KBDésiré
#137 258785-more_flexible_initial_language_on_content_types-137.patch27.78 KBDésiré
#137 interdiff-135-137.txt988 bytesDésiré
#135 258785-more_flexible_initial_language_on_content_types-135.patch27.61 KBDésiré
#131 258785-language_selection-131.patch27.68 KBArtusamak
#121 258785-more_flexible_initial_language_on_content_types-121.patch28.22 KBDésiré
#121 interdiff-119.txt2.6 KBDésiré
#119 258785-more_flexible_initial_language_on_content_types-119.patch25.97 KBDésiré
#119 interdiff-116.txt922 bytesDésiré
#116 258785-more_flexible_initial_language_on_content_types-116.patch25.64 KBDésiré
#116 interdiff-110-116.txt3.56 KBDésiré
#110 258785-more_flexible_initial_language_on_content_types-110.patch22.08 KBclemens.tolboom
#110 258785-more_flexible_initial_language_on_content_types-interdiff-87-110.txt4.82 KBclemens.tolboom
#88 nolanguage | drupal8.local_.jpg50.44 KBSchnitzel
#87 interdiff-85-87.patch14.54 KBSchnitzel
#87 258785-more_flexible_initial_language_on_content_types-87.patch22.18 KBSchnitzel
#85 258785-more_flexible_initial_language_on_content_types-85.patch21 KBSchnitzel
#84 258785-more_flexible_initial_language_on_content_types-84.patch20.97 KBSchnitzel
#72 258785-more_flexible_initial_language_on_content_types-72.patch19.98 KBDésiré
#72 interdiff-71-72.txt17.7 KBDésiré
#72 notranslate.png46.87 KBDésiré
#72 withtranslate.png48.12 KBDésiré
#71 interdiff-69-71.txt1.6 KBDésiré
#71 258785-more_flexible_initial_language_on_content_types-71.patch20.6 KBDésiré
#69 interdiff-67-69.txt448 bytesDésiré
#69 258785-more_flexible_initial_language_on_content_types-69.patch19.19 KBDésiré
#67 interdiff-65-67.txt4.29 KBDésiré
#67 258785-more_flexible_initial_language_on_content_types-67.patch19.18 KBDésiré
#65 258785-more_flexible_settings_for_language_on_content_types-65.patch17.6 KBDésiré
#64 258785-more_flexible_settings_for_language_on_content_types-64.patch18.51 KBDésiré
#61 interdiff-58-61.txt7.48 KBDésiré
#61 258785-more_flexible_settings_for_language_on_content_types-61.patch18.41 KBDésiré
#58 258785-more_flexible_settings_for_language_on_content_types-58.patch12.83 KBDésiré
#58 interdiff-56-58.txt7.5 KBDésiré
#58 Fullscreen-1.png160.72 KBDésiré
#58 Article | sprint.png52.88 KBDésiré
#56 258785-more_flexible_settings_for_language_on_content_types-56.patch11.29 KBDésiré
#54 entity-translation-settings.jpg41.44 KBandypost
#54 node-translation-i18n-content-type.jpg123.44 KBandypost
#47 258785-more_flexible_settings_for_language_on_content_types-46.patch11.29 KBDésiré
#42 Képernyőfotó 2012-02-18 - 4.35.09 PM.png81.69 KBpp
#42 Képernyőfotó 2012-02-18 - 4.31.01 PM.png81.23 KBpp
#38 258785-more_flexible_settings_for_language_on_content_types-38.patch7.22 KBpkiraly
#37 258785-more_flexible_settings_for_language_on_content_types-37.patch7.07 KBpkiraly
#36 258785-more_flexible_settings_for_language_on_content_types-36.patch8.72 KBDésiré
#34 258785-more_flexible_settings_for_language_on_content_types-34.patch3.96 KBDésiré
#18 content_language_neutral.zip825 bytesdonquixote
#13 neutral_language_content_default_issue_258785.patch2.52 KBvoxpelli
#1 neutral.patch859 bytesdesbeers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

desbeers’s picture

Version: 6.2 » 7.x-dev
Status: Active » Needs review
FileSize
859 bytes

I completely agree. For example, the i18n module for D6 has the option to filter content and one of the filter-options is 'including neutral'.

Let's say you create a custom node-type that just contains a photo; it's language-neutral and it's silly to assign the default language to it.

I found this problem while upgrading my site form D5 to D6 and added the 'translate' function only to stories and not for pages yet. New pages didn't show-up when visiting the 'non-default' language version of my site.

Attached patch just removes the 'offending' code in function locale_form_alter() in locale.module. and the SimpleTest didn't complain after running the tests.

bensemmel’s picture

Had the same issue. Thanks for the Patch. It is working for me as well

lilou’s picture

Status: Needs review » Reviewed & tested by the community

Look good and all tests pass (thx Testbed ;-)

Dries’s picture

I'd like to have Gabor review this.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

I think that there are more levels here. While for photos, this looks like a valid use case, you might have other areas where this does not apply. For example, you have a single foreign language blog (let's say French), there you would not enable language selection for blog posts, since you only have one language. But your blog posts are definitely not language neutral, they are French. So there might be two options to not add language selection to a content type: to assign default language or to assign language neutral. This could be another possibility in the locale module provided settings additionally to what we have on content types already.

If you language-enable a content type, it means you can select from at least Language neutral and a specific language, so it is not a one-op thing as you assert.

thePanz’s picture

I'm with Gàbor! A setting for default-language is needed: none (or language neutral) or Default (default site language)

I know that this feature won't be added into D6, but will be useful to have some module that implenent this (and maybe also http://drupal.org/project/preserve_language features).

Regards

Alice Heaton’s picture

Following a request from thePanz, the Preserve Language module now implements this fix for Drupal 6. The code was contributed to the module by thePanz.

I've only just committed it to CVS - so package might take up to 12 hours to be ready.

drewish’s picture

i think goba's got the right idea here. but i think that even the photo example is a little too simplistic. photos usually have a caption or title and that's going to have a language.

i've usually had to form alter the language selector to remove language neutral out of certain node types because they always have a language. but i'm using the active translation module to handle the selection of content for languages so my needs may be different than most.

David Lesieur’s picture

I'd love an option as proposed by Gábor in #5. Furthermore, I think that for language-enabled content types, admins should be able to remove the "language neutral" option.

On a particular project, I wish to avoid language neutral content, and show the original nodes to visitors when a translation is not available in their language. To avoid a "duplicate content penalty", I want all nodes to have a language in order to be able to instruct robots not to index a node when the current language is not the actual language of that node—something we can't do if nodes are "language neutral".

So far my plan to achieve this involves:

  1. Using Active Translation to select the proper nodes in general queries.
  2. Using Select Translation to select the proper nodes in views.
  3. Using hook_form_alter to remove the "language neutral" option from forms and force writers to select a language.
  4. When viewing a node, add the noindex meta tag if the node's language does not match the current language.

Hopefully, this discussion might help solve step 3 in D7.

dvinegla’s picture

Pasqualle’s picture

same alias problem here: #311158: default node language

I almost agree with Gábor, but the language of non-multilingual nodes should be configurable per node type. I may want a node type which is not multilingual, not language neutral and not written in default language.

But there is still a question what should be the alias language? Let's say node/5 is not a multilingual node and it has an alias. If I use url('node/5') then the alias should be used regardless of the active language and the node language.. So I guess alias language for non-multilingual nodes should be language neutral..

And as far as I know if the locale module is not enabled then all nodes are language neutral. So it is problematic when creating some nodes and enabling the locale module later.. Maybe a bulk update checkbox when changing the language of non-multilingual node types..

marcushenningsen’s picture

Maybe a bulk update checkbox when changing the language of non-multilingual node types..

This would be a great feature. It's very tedious to go through each alias to change it into "neutral language". If you can bulk promote to frontpage it should be possible to bulk change language.

voxpelli’s picture

Status: Needs work » Needs review
FileSize
2.52 KB

This needs to be fixed since the current behavior is very inconsistent.

Language neutral is default when locale.module isn't activated and also when it is activated and the multilingual support is enabled. The default all of the sudden is the site's default language though when locale.module is active and the multilingual support is disabled - which is currently the only time it's the default language for a node.

I think that the default behavior should always be neutral unless you actively choose otherwise - it would result in the most consistent behavior. I don't think the expected behavior for a new user of locale.module is to have all of their new content to become the default language - it's just confusing.

I'm attaching a patch that makes it configurable for each content type whether to default to neutral or to the site's default language. I've made neutral the default in the patch for the reasons I mentioned above.

voxpelli’s picture

Category: bug » feature

Also - I think this should be a feature request and not a bug report, no matter how annoying the current behavior might be...

Status: Needs review » Needs work

The last submitted patch failed testing.

plach’s picture

subscribe

donquixote’s picture

Agree with voxpelli.
I would love to see a module to fix this for D6.. or actually, I would love this to get fixed in the D6 locale module itself.
I'm not sure if the "Preserve Language" module is mature enough? I think for now I will code my own.

donquixote’s picture

FileSize
825 bytes

Anyone who needs a fix now can use the attached module.

klonos’s picture

Does the latest patch work in D6?

Also, I believe this would be better if it was an option. Some people want it to be set to neutral, others need it to remain set to the default site language, others might want to set this to a specific language. They all seem to have legit reasons and use cases. So, perhaps a setting in the language options would satisfy all these scenarios.

There could be a 'Language for content types without language support:' set of radio-boxes that would offer the following options:

- Set it to language Neutral.
- Set it to the site's default language.
[... list of all enabled languages ...]
- Language 1
- Language 2
- ...

What do you people think?

Now, I haven't reached the level of Drupal knowledge (yet) where I could re-roll patches with my suggestions, but as far as I can tell all there needs to be added to the previous patch in #18 is a variable that will hold the setting and the set of radio-boxes in the language settings page that would allow this variable's value to be set through the UI. Anyone up to it?

plach’s picture

Version: 7.x-dev » 8.x-dev

I am afraid this will have to wait for D8 :(

klonos’s picture

that's too bad :(

D8 means year(s) till this is finally fixed!

plach’s picture

Unfortunately we are way past over D7 feature freeze, we'll have to cope with this in contrib for another release.

Gabriel R.’s picture

In real life and with no holier-than-thou logic attached, not setting a language to a language neutral node is the only sane thing to do. Apply the change in patch #1 safely.

You will want then to go into the DB and update the `node` table to set `language` to an empty string for the previously created elements. Not fun.

donquixote’s picture

I think that there are more levels here. While for photos, this looks like a valid use case, you might have other areas where this does not apply. For example, you have a single foreign language blog (let's say French), there you would not enable language selection for blog posts, since you only have one language. But your blog posts are definitely not language neutral, they are French. So there might be two options to not add language selection to a content type: to assign default language or to assign language neutral. This could be another possibility in the locale module provided settings additionally to what we have on content types already.

Yes, there are more levels:
The French blog post is French, but it is not intended to be a part of the "french version of the website". Instead, all visitors are supposed to read all blog posts, no matter if French or Italian.

On the other hand, the French start page is only intended for visitors who switch the site language to French, while the Italian visitors are supposed to see the Italian front page.

Thirdly, there might be some "French FAQ" and "Italian FAQ" nodes, that only apply to the language of the reader, and where FAQ nodes in French have no direct counterpart in Italian.

And then we could have an international forum, where people write in any language they like (or mixed), and can't be expected to set a post or comment language.

Other sites could follow a different philosophy.

The real problem is not the post language, but the content selection logic.
The Active Translation module addresses this problem, and can solve it to some extent:
The French blog post does not have a translation associated with it, so it will be visible to Italian users. The same for the front page, as long as it is not translated. Once a translation of the front page exists, the French version will be hidden from Italian visitors in node listings.
The French FAQ nodes will never have a translation, but still they should be hidden from Italian visitors, who have their own FAQ section (that might be structured in a different way, because it is maintained by the Italian FAQ team).

Another point:
For some sites I made, I started with English (because I want to see an English admin interface), but the content was all in German. Of course, Drupal would tag all the content as English, because that was the default language.

AdrianB’s picture

Subscribing.

HnLn’s picture

sub

clashar’s picture

+1

rwt’s picture

How to force "Language Neutral" as default in Drupal 7, hack :

  1. Install & activate Internalization with content translation (i18n)
  2. In Multilingual settings -> Node Options (config/regional/i18n/node), check "Language neutral" as default language for content
  3. Enable Multilingual support for your content type (Publishing options tab)
  4. Optional : In multilingual settings of your content type, check "lock language"
  5. Optional : Install "Node form columns" module to hide the language selector drop down list in the node edit form or alter the edit form with a preprocess function.
marcushenningsen’s picture

I can confirm that this "hack" works perfectly. It's shame, though, that this isn't default behaviour in Core.

clemens.tolboom’s picture

Gábor Hojtsy’s picture

Title: Language for content types without language support must be Neutral » Provide more flexible settings for language on content types
Issue tags: +D8MI, +language-content

1. So what we have in Drupal 7 is that nodes default to "Language neutral" until you enable Locale module.
2. Then nodes will still default to language neutral but you can enable language support on node types.
3. Those node types will let you choose a language from a dropdown (which includes all your configured languages and language neutral). But it will still default to language neutral.

So Drupal 7 already fulfilled the goals of the original poster:

In you creates content type without language support, default language is assigned to it.

I think it must be '' (neutral) or FALSE

Now however many people complain that this is not the right way to do stuff. Ehm. @clemens is complaining about that at http://drupal.org/node/158803#comment-5602838

So people are not happy either way, so looks like we need to figure out what is the better sensible default and apply that + looks like we cannot forgo providing a setting. All this is of course only possible in D8. Retitled for that.

Looking at what i18n module provides I think we can get some ideas as to what people wanted so far:

A. For no-mutlilingual support nodes (1) default to site default language (2) default to none
B. For multilingual-supported nodes (1) default to current page language (2) default to none
C. For multilingual-supported nodes (1) remove language none from the options (2) keep it
D. For multilingual-supported nodes (1) lock the language (2) keep it selectable

I think B1 makes sense for A cases too and D1 is basically more like driving back towards A (we tell what should it have and not the users). So looks like we have three questions to answer:

I. What should be the default language for this content type? (page language, site default, none)
II. Should we lock the language for this type? (formerly multilingual support enabled/disabled)
III. Should "none" be an option for this content type? (yes/no?)

Then keep in mind we want to generalize language to all entity types, and these sound like reasonable settings for any entity type (eg. taxonomy terms, users, etc).

(i18n also supports listing disabled languages for assignment, not sure we should lump it up here if we want results sooner than months).

I don't agree with @clemens that this would block #158803: Hide "Language" column in content overview table if only one language is enabled though, so I think we can keep working on both in parallel.

Gábor Hojtsy’s picture

Status: Needs work » Postponed

BTW definitely postponed for now on #540294: Move node language settings from Locale to Node module because that moves around the code that this patch would largely affect.

Gábor Hojtsy’s picture

Status: Postponed » Active
Issue tags: +sprint

#540294: Move node language settings from Locale to Node module landed, so this is now unblocked! Let's validate the plan I've outlined then!

Désiré’s picture

Assigned: Unassigned » Désiré
Status: Active » Needs review
FileSize
3.96 KB

Just a first try, for testing.

Status: Needs review » Needs work
Désiré’s picture

Status: Needs work » Needs review
FileSize
8.72 KB

Fixed variable_get calls.

Default language is selected on node add form.

pkiraly’s picture

Assigned: Désiré » Unassigned
Status: Needs review » Active
FileSize
7.07 KB

Creating default language settings on node type form, and using the default language in node add form.

pkiraly’s picture

The previous changes and moving the node_type_language_enable_translation form element from node type form to translation module.

Désiré’s picture

Status: Active » Needs review
fubhy’s picture

Status: Needs review » Needs work
+++ b/core/modules/node/content_types.incundefined
@@ -188,11 +188,52 @@ function node_type_form($form, &$form_state, $type = NULL) {
+    $defaults = array(
+      LANGUAGE_NONE => t('- None -'),
+      'default' => t('Site\'s default'),
+      'current' => t('Current language'),
+      'user' => t('User\'s default'),

Personally, I prefer having double quotes around the string when there are single quotes required IN the string.

So instead of t('Site\'s default') better use t("Site's default")

+++ b/core/modules/node/node.pages.incundefined
@@ -87,7 +87,12 @@ function node_add($type) {
+    'uid' => $user->uid,
+    'name' => (isset($user->name) ? $user->name : ''),

The brackets are not required in 'name'.

Just use isset($user->name) ? $user->name : '' instead of (isset($user->name) ? $user->name : '')

fubhy’s picture

Status: Needs work » Needs review

Yes, looking good now.

pp’s picture

Status: Needs review » Needs work
FileSize
81.23 KB
81.69 KB

Please show settings at description of fieldset like "Publishing options": Published, Promoted to front page.

Some screenshot:

Képernyőfotó 2012-02-18 - 4.31.01 PM.png

Képernyőfotó 2012-02-18 - 4.35.09 PM.png

Gábor Hojtsy’s picture

+++ b/core/modules/node/content_types.incundefined
@@ -188,11 +188,52 @@ function node_type_form($form, &$form_state, $type = NULL) {
       '#title' => t('Multilingual support'),

"Language settings" or "Language configuration". It is not necessarily multilingual if there is a single language only.

+++ b/core/modules/node/content_types.incundefined
@@ -188,11 +188,52 @@ function node_type_form($form, &$form_state, $type = NULL) {
+      'default' => t('Site\'s default'),
+      'current' => t('Current language'),
+      'user' => t('User\'s default'),

We usually use "" to wrap text with single quotes in them.

+++ b/core/modules/node/content_types.incundefined
@@ -188,11 +188,52 @@ function node_type_form($form, &$form_state, $type = NULL) {
+    $lang_options = array('Specials' => $defaults);

Let's brainstorm a better name for these.

+++ b/core/modules/node/content_types.incundefined
@@ -188,11 +188,52 @@ function node_type_form($form, &$form_state, $type = NULL) {
+      '#description' => t('Select which language should be the default language when new node is created.', array('!languages' => url('admin/config/regional/language'))),

Putting on my @yoroy hat here :) 1. This is a select box, so we don't need to document that the user needs to do that. 2. Language is repeated twice (and we already know from the options and label). 3. Node is the internal technical term for content.

So what about: "Used for newly created content." or something similarly simple.

+++ b/core/modules/node/content_types.incundefined
@@ -188,11 +188,52 @@ function node_type_form($form, &$form_state, $type = NULL) {
+      '#title' => t('The default language is locked'),

"Lock language" IMHO

+++ b/core/modules/node/content_types.incundefined
@@ -188,11 +188,52 @@ function node_type_form($form, &$form_state, $type = NULL) {
+      '#description' => t('The user can not change the language of the node.'),

"Users will not be able to change the default language on content of this type." maybe? Not sure.

+++ b/core/modules/node/node.pages.incundefined
@@ -181,7 +186,11 @@ function node_form($form, &$form_state, $node) {
+  require_once 'content_types.inc';

Maybe document why we need this? Do we need it at all? I don't get why?

+++ b/core/modules/translation/translation.moduleundefined
@@ -109,12 +109,16 @@ function translation_permission() {
+    '#description' => t('If enabled, the content can be translated.'),

I'm not sure this adds value to the checkbox? It reiterates the same thing?

clemens.tolboom’s picture

+++ b/core/modules/node/node.pages.inc
@@ -87,7 +87,12 @@ function node_add($type) {
+  $node = (object) array(
+    'uid' => $user->uid,
+    'name' => (isset($user->name) ? $user->name : ''),
+    'type' => $type,
+    'language' => node_type_get_default_language($type)
+  );

This should use entity_create?

See http://api.drupal.org/api/drupal/core!modules!entity!entity.module/funct...
See devel : #1375800: Fatal error: Call to undefined method stdClass::save() when generating comments

Gábor Hojtsy’s picture

Talked with @Bojhan about the intro text on the fieldset. It does not add much value, so what about removing it?

Gábor Hojtsy’s picture

Also we have been thinking about how to hide the language as well, since the current patch degrades functionality for those where "Disabled" was selected for language before. Earlier the language select was not shown, this one shows the item. I think we should make some improvements there. Ie. make locked a three way option:

- Selection dispalyed
- Locked and hidden
- Locked and shown

And the last option would hide the value. Mid-option would show the language but not as a #disabled select, since that sounds like freaky for a user. I'd use an #item field for that for display. Not sure about the name of the options, but I think we'll need the three options separately. I thought about a "visibility" setting as well (to separate from the locked feature), but that would not have much meaning in case the value is not locked but not visible either (that would effectively lock it). So sounds like there is an overlap which means we'd want to have it in one option set.

Désiré’s picture

Status: Needs work » Needs review
FileSize
11.29 KB

@pkiray and I have merged our patches.

- Update and uninstall hook.

Some documentation needed....

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

Can you post updated screenshots? Thanks!

cosmicdreams’s picture

Yea, in general the documentation isn't where it needs to be.
Every function that has a return needs a @return in it's documentation. I'm trying to cut my teeth on this issue by making a best effort with that one thing.

andypost’s picture

Patch needs re-roll

# patch -p1 --dry-run < 258785-more_flexible_settings_for_language_on_content_types-46.patch
patching file core/modules/comment/comment.module
Hunk #1 FAILED at 1841.
1 out of 1 hunk FAILED -- saving rejects to file core/modules/comment/comment.module.rej
patching file core/modules/node/content_types.inc
patching file core/modules/node/node.install
Hunk #2 succeeded at 559 (offset 14 lines).
patching file core/modules/node/node.pages.inc
Hunk #1 FAILED at 87.
Hunk #3 succeeded at 193 with fuzz 1.
1 out of 3 hunks FAILED -- saving rejects to file core/modules/node/node.pages.inc.rej
patching file core/modules/path/path.test
patching file core/modules/translation/translation.module
patching file core/modules/translation/translation.test
Hunk #2 succeeded at 230 (offset -1 lines).
andypost’s picture

Assigned: Unassigned » andypost

Very interesting issue so I take it to make some progress... by my own.

I think we should minimize variable and option usage on settings.
Translation module already works by the same as it now.

if node is not translatable why it's langcode could be different from LANGUAGE_NONE?
When node translatable we just need to limit user's ability to change language and provide sensible defaults when user disallowed to set node language.

Last patch done before conversion from language to langcode so needs rework

+++ b/core/modules/comment/comment.module
@@ -1841,7 +1841,7 @@ function comment_form($form, &$form_state, $comment) {
-  if (($comment_langcode == LANGUAGE_NONE) && variable_get('node_type_language_' . $node->type, 0)) {
+  if (($comment_langcode == LANGUAGE_NONE) && !variable_get('node_type_language_locked_' . $node->type, 0)) {

Locked does not mean none-multilingual

+++ b/core/modules/node/content_types.inc
@@ -188,11 +188,49 @@ function node_type_form($form, &$form_state, $type = NULL) {
+    $form['language'] = array(
+      '#type' => 'fieldset',
+      '#title' => t('Language settings'),

We should add some .js magic to make it work as all other node's VTs

+++ b/core/modules/node/content_types.inc
@@ -188,11 +188,49 @@ function node_type_form($form, &$form_state, $type = NULL) {
+    $languages = language_list();
+    $grouped_languages = array();
+    foreach ($languages as $langcode => $language) {
+      $grouped_languages[(int) $language->enabled][$langcode] = $language;
+    }
+
+    $groups = array(t('Disabled'), t('Enabled'));
+    $defaults = array(
+      LANGUAGE_NONE => t('- None -'),
+      'site_default' => t("Site's default"),
+      'current' => t('Current language'),
+      'user_default' => t("User's default"),
+    );
+    $lang_options = array('Special options' => $defaults);
+    foreach (array(1, 0) as $status) {
+      if (isset($grouped_languages[$status])) {
+        $group = $groups[$status];
+        foreach ($grouped_languages[$status] as $langcode => $language) {
+          $lang_options[$group][$langcode] = $language->name;
+        }
+      }
+    }

Copied from translations module and needs to be more clear.
I'm going to fix this in translation module too.

+++ b/core/modules/node/content_types.inc
@@ -188,11 +188,49 @@ function node_type_form($form, &$form_state, $type = NULL) {
+    $form['language']['node_type_language_default'] = array(
+      '#type' => 'select',
+      '#title' => t('Default language'),
+      '#options' => $lang_options,
+      '#default_value' => variable_get('node_type_language_default_' . $type->type, LANGUAGE_NONE),
+    );
+

translation alters this piece so needs more attention here

+++ b/core/modules/node/content_types.inc
@@ -188,11 +188,49 @@ function node_type_form($form, &$form_state, $type = NULL) {
+    $form['language']['node_type_language_locked'] = array(
       '#type' => 'checkbox',
-      '#title' => t('Multilingual support'),

there's no more variable indicating multilinguage type for node-type?

+++ b/core/modules/node/content_types.inc
@@ -254,6 +292,42 @@ function node_type_form($form, &$form_state, $type = NULL) {
+function node_type_get_default_language($node_type) {
+  global $user;
+
+  $default_value = variable_get('node_type_language_default_' . $node_type, LANGUAGE_NONE);

Should live in node.module to be accessinble for other modules

+++ b/core/modules/node/node.install
@@ -456,9 +456,12 @@ function node_uninstall() {
+     // TODO: node_type_language_translation_enabled_ in the translation module
+     // ->condition('name', 'node_type_language_translation_enabled_' . $type)

not sure we need 3 vars per node-type

+++ b/core/modules/node/node.install
@@ -542,6 +545,31 @@ function node_update_8001() {
+      variable_set('node_type_language_default_' . $type, LANGUAGE_NONE);
+      if ($language) {
+        variable_set('node_type_language_locked_' . $type, TRUE);
+      }
+      else {
+        variable_set('node_type_language_locked_' . $type, FALSE);
+      }
+      if ($language == 2) {
+        variable_set('node_type_language_translation_enabled_' . $type, TRUE);
+      }
+    }
+    variable_del('node_type_language_' . $type);
+  }

deprecating *language_type* we need change notice and a new way to allow other modules to know a default language for node

+++ b/core/modules/node/node.pages.inc
@@ -87,7 +87,13 @@ function node_add($type) {
-  $node = (object) array('uid' => $user->uid, 'name' => (isset($user->name) ? $user->name : ''), 'type' => $type, 'language' => LANGUAGE_NONE);
+  module_load_include('inc', 'node', 'content_types');
+  $node = (object) array(
+    'uid' => $user->uid,
+    'name' => (isset($user->name) ? $user->name : ''),
+    'type' => $type,
+    'language' => node_type_get_default_language($type)

langcode for now!
Do we really need to change defaults for node?

BTW node_type_get_default_language() should be alterable or should use a variables to allow i18n change it's result

+++ b/core/modules/node/node.pages.inc
@@ -181,7 +187,7 @@ function node_form($form, &$form_state, $node) {
-  if (variable_get('node_type_language_' . $node->type, 0) && module_exists('language')) {
+  if (module_exists('language')) {
     $languages = language_list(TRUE);
     $language_options = array();
     foreach ($languages as $langcode => $language) {
@@ -193,6 +199,7 @@ function node_form($form, &$form_state, $node) {

@@ -193,6 +199,7 @@ function node_form($form, &$form_state, $node) {
       '#default_value' => (isset($node->language) ? $node->language : ''),
       '#options' => $language_options,
       '#empty_value' => LANGUAGE_NONE,
+      '#disabled' => variable_get('node_type_language_locked_' . $node->type, TRUE),

no variable to detect "multi-language-ability" of node

#disable is bad way, suppose better to #value if locked

+++ b/core/modules/translation/translation.module
@@ -111,10 +111,12 @@ function translation_permission() {
+  $form['language']['node_type_language_translation_enabled'] = array(
+    '#type' => 'checkbox',
+    '#title' => t('Enable translation'),
+    '#return_value' => TRANSLATION_ENABLED,
+    '#default_value' => variable_get('node_type_language_translation_enabled_' . $form['#node_type']->type, FALSE),
+  );

3rd variable? seems overkill

Gábor Hojtsy’s picture

if node is not translatable why it's langcode could be different from LANGUAGE_NONE?
When node translatable we just need to limit user's ability to change language and provide sensible defaults when user disallowed to set node language.

Not sure you are looking for feedback on the first question, but the sentence you posted right after that would be the answer :) That a node is not translatable does not mean it should always be LANGUAGE_NONE. It is more likely that it would be a fixed language to something (typically site default). A major aim of this patch is to make that configurable, so the binary on/off language support on node becomes more granular where you can set defaults and whether users should modify them or not.

there's no more variable indicating multilinguage type for node-type?

What's a multilingual node type? :) The proposed patch does away with that concept and instead provides flexible options to default nodes to certain languages. So if you'd default your content type to language none and fix it to that, that would be quasi equivalent to D7 behavior (minus it not being hidden, see below).

#disable is bad way, suppose better to #value if locked

Well, #disabled and type value are two different answers because they provide different levels of visbility. If you make it a value type, the language will be hidden from the user (not just locked). If you make it disabled, it will be locked but not hidden. I was lamenting on the missing "hidden" functionality in #46 above. Maybe we should have a "hidden" feature instead of a locked feature and leave locked but visible to contrib?

andypost’s picture

Setting up i18n and entity translation we have a lot of settings (screens bellow)

Currently we have only one variable in core (I {t} as suffix for node-type)
Suppose we should rename it to
node_type_translation_type_{t} - determines a way to translate a node
For D7: language_content_type_{t}
Now in D8 node_type_language_{t} changed in #540294: Move node language settings from Locale to Node module

  • 0 - Disabled
  • 1 - Enabled
  • 2 - Enabled with translation (core's translation.module - TRANSLATION_ENABLED)
  • 4 - Enabled, with field translation (entity_translation.module - ENTITY_TRANSLATION_ENABLED)

Then we can add another options as i18n does.

i18n uses it's own set of variables
i18n_node_default_language_none - Default language for content types with Multilingual support disabled.
i18n_node_options_[node_type] - Extended language options

  • 'current' =>Set current language as default for new content.
  • 'required' => Require language (Do not allow Language Neutral).
  • 'lock' => Lock language (Cannot be changed).

i18n_node_extended_[node_type] - Extended language support

  return array(
    I18N_LANGUAGE_ENABLED => t('Normal - All enabled languages will be allowed.', array(), $options),
    I18N_LANGUAGE_EXTENDED => t('Extended - All defined languages will be allowed.', array(), $options),
    I18N_LANGUAGE_EXTENDED | I18N_LANGUAGE_HIDDEN => t('Extended, but not displayed - All defined languages will be allowed for input, but not displayed in links.', array(), $options),
  );

entity-translation-settings.jpg

node-translation-i18n-content-type.jpg

Gábor Hojtsy’s picture

Title: Provide more flexible settings for language on content types » Provide more flexible settings for inital language on content types

@andypost: yes, this issue is about the initial language for content types. We cannot fix everything at once IMHO without stacking confusing settings on top of each other like i18n does. So this issue is about the initial language, therefore i18n's 1st and 3rd checkboxes apply and the radio buttons do not apply. For the list of languages that apply to certain things (the middle checkbox and the radio buttons), see #1314250: Allow filtering/configuration of which languages apply to what (UI, nodes, files, etc).

So we talked about introducing (a) flexible defaults for language (b) the possibility to lock language. These two allow to keep the core functionality by mapping to the new settings AND also provide a rich set of new settings (and conveniently sized so we can actually solve it without debating it to death endlessly and keeping the current mess in indecision). The mapping of existing functionality (Drupal 7) to the new settings proposed in the patch above would be:

Drupal 7 setting Drupal 8 default langugae Drupal 8 locked Drupal 8 translation enabled
Multilingual support disabled LANGUAGE_NONE YES NO
Multilingual support enabled LANGUAGE_NONE NO NO
Multilingual support enabled with translation LANGUAGE_NONE NO YES

(The node translation method is planned to be integrated with the field translation method, so we'll only have one way).

Note that these settings do allow users to set different flexible defaults, the ones provided in the above table are the D7 defaults for the corresponding settings, this will not necessarily be the D8 defaults.

Once again, this issue is about expanding the initial value configuration for nodes. In D7 you could only lock a node type to LANGUAGE_NONE and if you allowed selection of language, it always defaulted to LANGUAGE_NONE. By refactoring the settings to be more flexible I think we make the feature more understandable (I got lots of questions on what multilingual support enabled means, now the options spell it out :).

For the list of languages available, #1314250: Allow filtering/configuration of which languages apply to what (UI, nodes, files, etc) is the relavant issue, not this one. Scope and intention clearer now?

Désiré’s picture

Status: Needs work » Needs review
FileSize
11.29 KB

Rerolled patch for testbot.

Status: Needs review » Needs work
Désiré’s picture

- node_type_get_default_language moved to node.module, renamed to node_type_get_default_langcode
- fixed array key 'langcode' in node_add function
- Language settings vertical tab moved to locale.module
- JS magic for language setting vertical tab summary

Screenshot of language selector.
Screenshot of language settings vertical tab.

Désiré’s picture

Status: Needs work » Needs review

test it please....

Status: Needs review » Needs work
Désiré’s picture

Status: Needs work » Needs review
FileSize
18.41 KB
7.48 KB

Some updated tests.

Désiré’s picture

Assigned: andypost » Désiré

Status: Needs review » Needs work
Désiré’s picture

Status: Needs work » Needs review
FileSize
18.51 KB
Désiré’s picture

Corrected patch (no gitignore...)

Status: Needs review » Needs work
Désiré’s picture

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

- Site's language by default on content type setting page
- probably no test fails...

Status: Needs review » Needs work

The last submitted patch, 258785-more_flexible_initial_language_on_content_types-67.patch, failed testing.

Désiré’s picture

Status: Needs work » Needs review
FileSize
19.19 KB
448 bytes

language_from_default() function depends on locale module, so use language_default()->langcode instead.

Status: Needs review » Needs work

The last submitted patch, 258785-more_flexible_initial_language_on_content_types-69.patch, failed testing.

Désiré’s picture

Status: Needs work » Needs review
FileSize
20.6 KB
1.6 KB

A new node has now the sites default language by default.

Désiré’s picture

- language options vertical tab in node module (content_types.inc)
- uninstall hook for translation module: sets translatable content types to selectable
- use 'node_type_language_selectable' variable instead of 'node_type_locked' and 'node_type_translation_enabled' (both language locked and translation enabled can't be TRUE #258785-55: Provide more flexible settings for initial language on content types)
- use radio buttons instead of 2 checkboxes

Language settings without translation module:
Language settings without translation module.

Language settings with translation module:
Language settings with translation module.

Status: Needs review » Needs work

The last submitted patch, 258785-more_flexible_initial_language_on_content_types-72.patch, failed testing.

andypost’s picture

Locked and no hidden? should we introduce new core's permission - change/set content language?

Gábor Hojtsy’s picture

I dont think this is a permission question, it is about configuruing which types you want to tie to which languages. Such as if you have a multilingual site but a monolingual forum, you could tie those nodes to your default language. Or if you have a product pictures file set, you could tie those to language not applicable.

andypost’s picture

I suppose if site has different editors for different language parts so changing a node's language should have moderation (permission to change language of node's type)
I'm talking about nodes Only!

The assumption I made is that disabled language elemnt would confuse a bit more then hidden. OTOH l18n does not provides Hidden option (default + reqired + locked)

Gábor Hojtsy’s picture

@andypost: yes, language based permissions make sense (eg. These users can only create/edit X language nodes), but that is not at all the scope of this issue. This is simply about giving more versatile options for setting how the language is initialized on nodes.

Kristen Pol’s picture

Title: Provide more flexible settings for inital language on content types » Provide more flexible settings for initial language on content types
pixelite’s picture

The UI for #72 looks great! I would suggest the following changes to the labels:

  • Language selection on node edit form > Language options per node
  • Locked > Locked to Default Language
  • Selectable > User Selects Language
  • Selectable with Translation > User Selects Language, Translation Enabled

Since the language options include whether the nodes can be translated as well as whether you can select a language, I think changing the label from 'Language selection' to 'Language options' makes sense.

Kristen Pol’s picture

1) I like the naming that @pixelite suggests better than what is currently on the UI mockup.

2) I installed the patch and went to edit a content type and got this error:

Notice: Use of undefined constant LANGUAGE_NONE - assumed 'LANGUAGE_NONE' in locale_form_node_type_form_alter() (line 309 of /var/www/drupal8/core/modules/locale/locale.module).
Kristen Pol’s picture

Regarding Default language:

1) Wondering if the label should be more descriptive though maybe it's obvious enough, e.g.

Default language for content
or
Default language for nodes

2) For some reason, I find the "Special options" label confusing though I'm not thinking of a better alternative at the moment.

klonos’s picture

The wording as is helps the tab's quick info text make sense in a glance (even when the selected tab is a different one from the "Language settings"). Consider the current variants:

Hungarian, Locked
Hungarian, Selectable
Hungarian, Selectable with translation support

To the ones suggested in #79:

Hungarian, Locked to Default Language
Hungarian, User Selects Language
Hungarian, User Selects Language, Translation Enabled

Personally, I like it better the way it is now. It's shorter and it makes sense.

Berdir’s picture

+++ b/core/modules/node/node.install
@@ -558,6 +559,23 @@ function node_update_8002() {
 /**
+ * Rename node type language variable names.
+ *
+ * @see http://drupal.org/node/258785
+ */

@see's to issues are usually not added to core, unless there is a very good reason to that is relevant in the future. E.g. you having to do a workaround due to a an issue in another module that you can't fix, or reference to a issue that's still open.

Additionally, this would actually make it show up in the UI I think.

+++ b/core/modules/node/node.pages.inc
@@ -187,14 +192,16 @@ function node_form($form, &$form_state, $node) {
+      // New nodes without multilingual support get the default language of the
+      // content type (for new nodes already set in node_add), old nodes keep
+      // their language if language.module is not available.
+      '#value' => $node->langcode,

No idea if the comment here is still necessary as the new code has no checks or special cases anymore. I guess it's now documented in the place where $node->langcode is set.

+++ b/core/modules/translation/translation.install
@@ -0,0 +1,21 @@
+    if ($translatable == TRANSLATION_ENABLED) {
+      variable_set('node_type_language_selectable_' . $type, 1);
+    }

The constant can't be used in the .install file, that's causing the notices in the test.

There are also various rejects in the patch due to the language_none rename patch. I guess the new function that returns the language needs to be updated as well.

Schnitzel’s picture

Reroll of the patch, because LANGUAGE_NONE has changed to LANGUAGE_NOT_SPECIFIED

Schnitzel’s picture

Status: Needs work » Needs review
FileSize
21 KB

There are also various rejects in the patch due to the language_none rename patch. I guess the new function that returns the language needs to be updated as well.

this is fixxed in the attached patch.

The other comments from Berdir are not yet included!

Status: Needs review » Needs work

The last submitted patch, 258785-more_flexible_initial_language_on_content_types-85.patch, failed testing.

Schnitzel’s picture

Status: Needs work » Needs review
FileSize
22.18 KB
14.54 KB

had a talk with Gabor about the the new settings.
The Problem is, that with the RadioButtons it was not possible to set the Default Language "Locked" but still be able to translate it. So we need there Checkboxes.

I changed the code back that it uses Checkboxes. Also changed this:
- Added JS which disables "Enable Translation" Checkbox when DefaultLanguage is "None" and "Locked"
- Wrote also an Field Validator for the same case
- fixxed @see problem in update hook (it really showed in UI)
- fixxed using of constant in translation.install

Schnitzel’s picture

here a short picture how it looks like now
http://drupal.org/files/nolanguage | drupal8.local_.jpg

Kristen Pol’s picture

I applied the patch and it said it was going to create a file that already existed but it seemed to be successful:

[kristen:drupal8]$ patch -p1 < 258785-more_flexible_initial_language_on_content_types-87.patch 
patching file core/modules/comment/comment.module
patching file core/modules/field/modules/text/text.test
patching file core/modules/locale/locale.test
patching file core/modules/node/content_types.inc
patching file core/modules/node/content_types.js
patching file core/modules/node/node.install
patching file core/modules/node/node.module
patching file core/modules/node/node.pages.inc
patching file core/modules/path/path.test
patching file core/modules/poll/poll.test
patching file core/modules/simpletest/tests/upgrade/upgrade.language.test
The next patch would create the file core/modules/translation/translation.install,
which already exists!  Assume -R? [n] y
patching file core/modules/translation/translation.install
patching file core/modules/translation/translation.module
patching file core/modules/translation/translation.test
pixelite’s picture

The UI looks good. I've tested the patch in #87 with various configurations and it works well.

Kristen Pol’s picture

Feedback on patch so far:

  1. The "Lock language" checkbox is enabled by default.
  2. There might be a space at the end of the "Lock language" text because it is showing up in the left side (under the Language settings fieldset lable) with a space before the comma when enabling translation as well, e.g. "Burmese, Lock language , Enable translation"
Désiré’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Great work!

There might be a space at the end of the "Lock language" text because it is showing up in the left side (under the Language settings fieldset lable) with a space before the comma when enabling translation as well, e.g. "Burmese, Lock language , Enable translation"

'Lock language' and 'Translation enabled' options are mutually exclusive. (You can't translate a node to hungarian if the type is locked to english.) (This was the reason why I originally used radio buttons for this...)

Now we need validation for this, and probably a javascript (or states) solution to uncheck 'Lock language' when 'Translation enabled' is checked, and vice versa. And probably a description line.

The patch still needs automated tests:
- for update hook
- for changing the initial language of a node
- for changing the sites default language
- for 'Lock language' option

Kristen Pol’s picture

@Désiré Thanks for the clarification. Makes sense!

Should the language be locked by default? I wasn't sure if that is the desired behavior though it seems reasonable (e.g. locked and site default language).

donquixote’s picture

Hi,
I have not exactly followed this issue in all detail, but I have a more general comment about language selection for content.

If a node has entity translation (translation per field) enabled, that means there is one nid for multiple languages. In theory, the node should be language neutral, whereas each of the localized versions does have a language. In practice, when you create a node with entity/field translation enabled, this node needs to be given a language - usually the site default.

I think this is not totally wrong, but it does give the language setting a different meaning, and should rather be named "source language" than "node language". The node itself has multiple languages, it is just that one of them is used as a source for the translation process, and as a fallback.

This distinction does become relevant

  • when filtering content visibility by language
  • when determining a language for things that are associated with the node (such as, menu items)
  • for all the labels and flavor text in the admin and authoring forms.

I hope this does help anyone..

klonos’s picture

If a node has entity translation (translation per field) enabled, that means there is one nid for multiple languages. In theory, the node should be language neutral, ... The node itself has multiple languages, it is just that one of them is used as a source for the translation process, and as a fallback.

Nope, if a nod id has multiple languages, then it should be marked as such - not as language neutral. It seems that we are going to have LANGUAGE_MULTIPLE: #1471432: Rework language_list(), let people use more special languages

Schnitzel’s picture

'Lock language' and 'Translation enabled' options are mutually exclusive. (You can't translate a node to hungarian if the type is locked to english.) (This was the reason why I originally used radio buttons for this...)

Why not? Maybe there is a missunderstaning. We actually don't copy the functionality of Language Locking in D7 with i18n. In there the locking means that you cannot change the language after a node is created.
With the locking in D8 you force the node from the beginning to have a specific language setting.
So right now I'm not really sure if we need Radios or Checkboxes.

I could maybe imagine this usecase:
You have multiple languages enabled but want only 1 language as source language and the other languages should only be translated via a contrib Module such as TMGMT. Then Locking is required and Translation should also be enabled.

Should the language be locked by default? I wasn't sure if that is the desired behavior though it seems reasonable (e.g. locked and site default language).

Yes, this is the same as in D7 when you had Multilanguage Support disabled (which is also default there)

Schnitzel’s picture

If a node has entity translation (translation per field) enabled, that means there is one nid for multiple languages. In theory, the node should be language neutral, ... The node itself has multiple languages, it is just that one of them is used as a source for the translation process, and as a fallback.

Agree with klonos #95, as far as I understood, that in D8 you don't select Node or Field Translations anymore, this depends on the Lanuage Setting of the Node. If it has "Multiple", then Field Translation will happen.
I am right with this?

donquixote’s picture

#95, #97
sounds good :)
My comment #94 was totally uninformed then.
Maybe it's time to update the issue summary.

That said.. even if a node/entity is set as multi-language, there is still one language that is the "source", isn't it? Or are they all equal?

klonos’s picture

...there is still one language that is the "source", isn't it?

Good question Andreas! Anyone know how we'll be tracking this now? Will the original language fields be marked as source somehow?

Gábor Hojtsy’s picture

@klonos: we documented the field language support stuff yesterday at http://drupal.org/node/1500308 :) hopefully that answers your question proper. If not, we need to improve there, so we don't need to explain this 1-1 to people all the time :)

klonos’s picture

Thanx Gábor, I wasn't aware of that handbook page. It seems to be explained perfectly there and if something needs to be improved it should be done there as you say.

If I got it right we'll be using $entity->langcode (in entity/fields based translation) to figure out the "source" language of a filed that has translation enabled instead of using tnid like we did with content translation. Right?

Gábor Hojtsy’s picture

@klonos: yes.

@Schnitzel, @Desire: can either of you continue working on this? It would be great to make this land sooner than later :) Also I still would like to underline that the "hidden language selector" feature that is present before the patch is not present with this patch added. So for content types where language is locked, the selector will always be visible, right? I think we need to make sure to figure out something good for that. Maybe rename that to "locked and hidden" and make it hidden as well by default. Then contrib will be able to show it if they want it. I think it would be important from a usability PoV now to show disabled fields, when we don't need to.

andypost’s picture

IMO Locking is confusing and produce more questions from editors: why it could not be changed? and also eats a screen for useless option and makes content edit form harder to understand for newbies

Gábor Hojtsy’s picture

Ok, so what about if we make it hidden then and say it is "Hidden"? It would make it locked too in essence, since you'd have no UI to change it. That would keep the core functionality while making it more flexible.

Désiré’s picture

And what if we use 'item' instead of 'select' for the locked language on the node edit form? (And probably a short description)

Gábor Hojtsy’s picture

@Désiré: that is still a functionality change and it would be hard to get in a patch that removes functionality without any good reason. Let's keep the setting here for setting initial language and not exposing it if the user chooses that. That is the existing functionality (except you cannot pick the default dynamically, which is what this patch is about). We are past 100 comments, which is a clear warning sign we should not blow up the scope :)

Gábor Hojtsy’s picture

@Désiré: are you working on this?

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary.

clemens.tolboom’s picture

Issue summary: View changes

Inserted a (short) issue summary.

clemens.tolboom’s picture

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

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

The last submitted patch, 258785-more_flexible_initial_language_on_content_types-87.patch, failed testing.

clemens.tolboom’s picture

Attached is a reroll from patch #87 ... there was one problem with content_types.js so please review that again please.

(I ran patch -F100 -p1 < ../258785-more_flexible_initial_language_on_content_types-87.patch which succeeded to apply)

clemens.tolboom’s picture

clemens.tolboom’s picture

Issue summary: View changes

Fixed typo and broken image.

clemens.tolboom’s picture

I've updated the Issue Summary twice: Added an Issue Summary template and added step 'How to test.' which I hope helps others to quicken their review.

@Gábor Hojtsy : Are there issues for entity language settings yet? Maybe we could fill-in the remaining tasks with those.

(sorry for the many updates)

Désiré’s picture

Status: Needs review » Needs work

Still needs automated tests.

Désiré’s picture

Issue summary: View changes

Added 'steps to test'

clemens.tolboom’s picture

I updated the Issue summary a little for 'Remaining Tasks'

@Désiré: could you update the issue summary 'Remaining Tasks' to make a complete list of tests. Tnx :)

Gábor Hojtsy’s picture

No we don't yet know how to abstract this to a generic config UI for entities yet. I believe we don't have a common entity settings form concept ATM, do we?

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary.

Kristen Pol’s picture

Issue summary: View changes

Updated issue summary.

Désiré’s picture

Status: Needs work » Needs review
FileSize
3.56 KB
25.64 KB

Here is a rerolled patch, with some test cases:

- tests the default initial language
- the default lock language option
- the site's default initial language option
- changing the initial language option

missing tests:
- update test
- user default initial language

Désiré’s picture

Issue summary: View changes

Updated issue summary.

Désiré’s picture

Issue summary: View changes

Updated issue summary.

Désiré’s picture

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

The last submitted patch, 258785-more_flexible_initial_language_on_content_types-116.patch, failed testing.

Désiré’s picture

Status: Needs work » Needs review
FileSize
922 bytes
25.97 KB

Fixes in drupalCreateNode().

Status: Needs review » Needs work

The last submitted patch, 258785-more_flexible_initial_language_on_content_types-119.patch, failed testing.

Désiré’s picture

All tests fixed:
The default initial langcode of a node is the site default,
BUT the array key of the content fields (like node body) is still LANGUAGE_NOT_SPECIFIED ('und')

Désiré’s picture

Status: Needs work » Needs review
Ujin’s picture

seems patch need reroll

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Suppose langcode for node fields is out of scope for the patch

# patch -p1 < 258785-more_flexible_initial_language_on_content_types-121.patch
patching file core/modules/comment/comment.module
Hunk #1 succeeded at 1838 (offset -5 lines).
andypost’s picture

+++ b/core/modules/node/content_types.inc
@@ -186,11 +186,45 @@ function node_type_form($form, &$form_state, $type = NULL) {
   if (module_exists('language')) {
-    $form['workflow']['node_type_language'] = array(
+    $languages = language_list();
+    $grouped_languages = array();
+    foreach ($languages as $langcode => $language) {
+      $grouped_languages[(int) $language->enabled][$langcode] = $language;
+    }
+    $groups = array(t('Disabled'), t('Enabled'));
+    $defaults = array(

This hunk should be rewritten in #1539072: Support for disabled languages broken, drop it

Gábor Hojtsy’s picture

@andypost: thanks for the review. Indeed #1539072: Support for disabled languages broken, drop it would not apply if this will land first.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

#1539072: Support for disabled languages broken, drop it landed first, so this will need all disabled language handling removed. Thanks for your great work so far!

Artusamak’s picture

Assigned: Désiré » Artusamak
perusio’s picture

Assigned: Artusamak » perusio

I'm wetting my beak with this.

Artusamak’s picture

Assigned: perusio » Artusamak

I'm still on this issue, i'm reassigning it to myself. ;)

Artusamak’s picture

Status: Needs work » Needs review
FileSize
27.68 KB

First attempt on this reroll, going home and need to some how is the testbot behaving.

Status: Needs review » Needs work

The last submitted patch, 258785-language_selection-131.patch, failed testing.

Désiré’s picture

+++ b/core/modules/locale/locale.testundefined
diff --git a/core/modules/node/content_types.inc b/core/modules/node/content_types.inc
old mode 100644
new mode 100755

+++ b/core/modules/node/node.moduleundefined
diff --git a/core/modules/node/node.pages.inc b/core/modules/node/node.pages.inc
old mode 100644
new mode 100755

+++ b/core/modules/system/tests/upgrade/upgrade.language.testundefined
diff --git a/core/modules/translation/translation.module b/core/modules/translation/translation.module
old mode 100644
new mode 100755

Why changed the modes?

Désiré’s picture

+++ b/core/modules/node/node.pages.incundefined
@@ -181,26 +181,54 @@ function node_form($form, &$form_state, Node $node) {
+    // Get the default langcode to use.
+    // If we are creating a new translation,
+    // the default language is the one passed from the URL
+    if (isset($_GET['target'])) {
+      $default_langcode = $_GET['target'];
+    }
+    // If the user is editing a translation,
+    // keep the default language from the node.
+    else if (isset($node->nid) && isset($node->vid)) {
+      $default_langcode = $node->langcode;

On a node edit page, or a translation page the $node object has already the correct langcode variable, so this code are unnecessary.

+++ b/core/modules/node/node.pages.incundefined
@@ -181,26 +181,54 @@ function node_form($form, &$form_state, Node $node) {
+    // Otherwise get the default langcode coming from the content type
+    // configuration.
+    else {
+      $default_langcode = node_type_get_default_langcode($node->type);

This can be set up in the node_add function, just like in the previous patches.

Désiré’s picture

Status: Needs work » Needs review
FileSize
27.61 KB

The patch in #131 was wrongly rerolled, so I rerolled the patch from #121 again.

andypost’s picture

Status: Needs review » Needs work
+++ b/core/modules/comment/comment.module
@@ -1843,7 +1843,7 @@ function comment_form($form, &$form_state, Comment $comment) {
-  if (($comment_langcode == LANGUAGE_NOT_SPECIFIED) && variable_get('node_type_language_' . $node->type, 0)) {
+  if (($comment_langcode == LANGUAGE_NOT_SPECIFIED) && !variable_get('node_type_language_locked_' . $node->type, 0)) {
     $comment_langcode = $language_content->langcode;
  }

What does it mean? Any reason to depend a comment language on locked state?

+++ b/core/modules/node/content_types.inc
@@ -186,11 +186,45 @@ function node_type_form($form, &$form_state, $type = NULL) {
+    $groups = array(t('Disabled'), t('Enabled'));

We have no support for disabled languages after #1539072: Support for disabled languages broken, drop it

Désiré’s picture

Status: Needs work » Needs review
FileSize
988 bytes
27.78 KB

1.
Node types was LANGUAGE_NOT_SPECIFIED language by default and, when a node type was multilingual support, a comment gets it's language from the node. But now all content types has language support, so you have right, this modification is wrong, instead, now the comment can always get the langcode from the node.

-  // If a content type has multilingual support we set the comment to inherit the
-  // content language. Otherwise mark the comment as language neutral.
-  $comment_langcode = $comment->langcode;
-  if (($comment_langcode == LANGUAGE_NOT_SPECIFIED) && variable_get('node_type_language_' . $node->type, 0)) {
-    $comment_langcode = $language_content->langcode;
-  }
   $form['langcode'] = array(
     '#type' => 'value',
-    '#value' => $comment_langcode,
+    '#value' => $language_content->langcode,
   );

2.
In the latest patch (in #135) don't contains support for disabled languages.

andypost’s picture

Status: Needs review » Needs work
+++ b/core/modules/comment/comment.module
@@ -1836,15 +1836,9 @@ function comment_form($form, &$form_state, Comment $comment) {
   $form['langcode'] = array(
     '#type' => 'value',
-    '#value' => $comment_langcode,
+    '#value' => $language_content->langcode,
   );

This require a shord comment why comment language uses this kind of language

+++ b/core/modules/node/content_types.inc
@@ -186,11 +186,35 @@ function node_type_form($form, &$form_state, $type = NULL) {
+    foreach ($languages as $langcode => $language) {
+      $lang_options[$langcode] = t($language->name);
+    }

Don't use t($var). Suppose $language->name is enough to choose

+++ b/core/modules/simpletest/drupal_web_test_case.php
@@ -935,7 +935,7 @@ class DrupalWebTestCase extends DrupalTestCase {
-      'langcode'  => LANGUAGE_NOT_SPECIFIED,
+      'langcode'  => language_default()->langcode,

Do this related to the patch? You are going to change to much!

Désiré’s picture

- Comment added to comment language (as mentioned above)
- Misused t() removed from content_types.inc (as mentioned above)
- patch rerolled

+++ b/core/modules/simpletest/drupal_web_test_case.php
@@ -935,7 +935,7 @@ class DrupalWebTestCase extends DrupalTestCase {
-      'langcode'  => LANGUAGE_NOT_SPECIFIED,
+      'langcode'  => language_default()->langcode,

I think this is related to the patch, because from now a node type gets the sites default language by default for initial language, so drupalCreateNode() should use this for default settings. (btw I think, it couse test fails, if we leave it LANGUAGE_NOT_SPECIFIED, so I attached a patch for test this)

Désiré’s picture

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

Status: Reviewed & tested by the community » Needs work

Looks like the "should fail" patch did not fail. Hm. Here is a deeper code review with things that people did not seem to catch yet. Surprising this was RTBC for some time given these findings. :/

+++ b/core/modules/comment/comment.moduleundefined
@@ -1834,15 +1834,10 @@ function comment_form($form, &$form_state, Comment $comment) {
-  // If a content type has multilingual support we set the comment to inherit the
-  // content language. Otherwise mark the comment as language neutral.
-  $comment_langcode = $comment->langcode;
-  if (($comment_langcode == LANGUAGE_NOT_SPECIFIED) && variable_get('node_type_language_' . $node->type, 0)) {
-    $comment_langcode = $language_content->langcode;
-  }
+  // Uses the language of the content as comment language.
   $form['langcode'] = array(
     '#type' => 'value',
-    '#value' => $comment_langcode,
+    '#value' => $language_content->langcode,

The previous code allowed to preset the comment language (and only defaulted to node language if it was not preset to a concrete language). This is not happening anymore.

+++ b/core/modules/locale/locale.testundefined
@@ -2271,9 +2271,9 @@ class LocaleContentFunctionalTest extends DrupalWebTestCase {
@@ -2298,7 +2298,7 @@ class LocaleContentFunctionalTest extends DrupalWebTestCase {

@@ -2298,7 +2298,7 @@ class LocaleContentFunctionalTest extends DrupalWebTestCase {
     $edit = array(
       'type' => 'page',
       'title' => $node_title,
-      'body' => array($langcode => array(array('value' => $node_body))),
+      'body' => array(LANGUAGE_NOT_SPECIFIED => array(array('value' => $node_body))),
       'langcode' => $langcode,
     );
     $node = $this->drupalCreateNode($edit);

@@ -2341,7 +2341,7 @@ class LocaleContentFunctionalTest extends DrupalWebTestCase {
@@ -2390,7 +2390,7 @@ class LocaleContentFunctionalTest extends DrupalWebTestCase {

@@ -2390,7 +2390,7 @@ class LocaleContentFunctionalTest extends DrupalWebTestCase {
     $edit = array(
       'type' => 'article',
       'title' => $node_title,
-      'body' => array($langcode => array(array('value' => $node_body))),
+      'body' => array(LANGUAGE_NOT_SPECIFIED => array(array('value' => $node_body))),
       'langcode' => $langcode,
       'promote' => 1,
     );

Hm, why would you change these?

+++ b/core/modules/node/content_types.incundefined
@@ -186,11 +186,35 @@ function node_type_form($form, &$form_state, $type = NULL) {
+    $lang_options = array(
+      LANGUAGE_NOT_SPECIFIED => t('- None -'),
+      'site_default' => t("Site's default"),
+      'current' => t('Current language'),
+      'user_default' => t("User's default"),
+    );

I'd explain what "current language" means here. Is it interface language, content language? (Unless we replace this with a dynamic list of all language types defined on the system, I'd suggest we hardcode "current content language").

+++ b/core/modules/node/node.installundefined
@@ -458,9 +458,12 @@ function node_uninstall() {
+     // TODO: node_type_language_translation_enabled_ in the translation module
+     // ->condition('name', 'node_type_language_translation_enabled_' . $type)

Why is this TODO not implemented in translation module?

+++ b/core/modules/node/node.installundefined
@@ -558,6 +561,24 @@ function node_update_8002() {
+    $language = variable_get('node_type_language_' . $type, 0);
+    if ($language == 0) {
+      variable_set('node_type_language_locked_' . $type, TRUE);
+    }
+    if ($language == 2) {
+      variable_set('node_type_language_translation_enabled_' . $type, 2);

I don't think we want to name this variable $language, since its not a language object. We use $language exclusively for language objects in Drupal 8. Name it $setting or $multilingual_setting or $node_type_language_setting IMHO :)

+++ b/core/modules/node/node.moduleundefined
@@ -639,6 +639,47 @@ function node_field_extra_fields() {
+ * Get the default language for a node type
+ *
+ * @param string $node_type
+ *   The type of node
+ *
+ * @return (string)
+ *   The language code of the node type's default langcode
+ */

Make sure to include dots at end of descriptions. 3 dots missing here.

+++ b/core/modules/node/node.moduleundefined
@@ -639,6 +639,47 @@ function node_field_extra_fields() {
+  if ($default_value == LANGUAGE_NOT_SPECIFIED) {
+    return LANGUAGE_NOT_SPECIFIED;

I don't understand why you need this here. It would be returned later anyway(?). Also baking this in looks bad for when more special languages come to fruition.

+++ b/core/modules/node/node.moduleundefined
@@ -639,6 +639,47 @@ function node_field_extra_fields() {
+    case 'current':
+      global $language_interface;
+      $default_value = $language_interface->langcode;
...
+        global $language_interface;
+        $default_value = $language_interface->langcode;

In light of my above comment, these would be $language_content, right?

Also you can avoid repeating the same code by moving the 'user_default' case above the 'current' case and falling over if the condition did not pass.

+++ b/core/modules/node/node.pages.incundefined
@@ -181,7 +181,7 @@ function node_form($form, &$form_state, Node $node) {
@@ -193,14 +193,16 @@ function node_form($form, &$form_state, Node $node) {

@@ -193,14 +193,16 @@ function node_form($form, &$form_state, Node $node) {
       '#default_value' => (isset($node->langcode) ? $node->langcode : ''),
       '#options' => $language_options,
       '#empty_value' => LANGUAGE_NOT_SPECIFIED,
+      '#disabled' => variable_get('node_type_language_locked_' . $node->type, TRUE),

I think we agreed above in the discussion that this would hide the widget too in core (basically make #type => value). Contrib could show it selectively if they want to, no? As it is this is a regression since it would show language fields on single language sites for example.

+++ b/core/modules/node/node.pages.incundefined
@@ -193,14 +193,16 @@ function node_form($form, &$form_state, Node $node) {
-      // New nodes without multilingual support get the default language, old
-      // nodes keep their language if language.module is not available.
-      '#value' => !isset($form['#node']->nid) ? language_default()->langcode : $node->langcode,
+      // New nodes without multilingual support get the default language of the
+      // content type (for new nodes already set in node_add), old nodes keep
+      // their language if language.module is not available.
+      '#value' => $node->langcode,

I don't understand the comment now that the condition was removed from below?!

+++ b/core/modules/node/node.testundefined
@@ -2289,8 +2348,8 @@ class NodeTokenReplaceTestCase extends NodeWebTestCase {
-    $tests['[node:body]'] = _text_sanitize($instance, $node->langcode, $node->body[$node->langcode][0], 'value');
-    $tests['[node:summary]'] = _text_sanitize($instance, $node->langcode, $node->body[$node->langcode][0], 'summary');
+    $tests['[node:body]'] = _text_sanitize($instance, $node->langcode, $node->body[LANGUAGE_NOT_SPECIFIED][0], 'value');
+    $tests['[node:summary]'] = _text_sanitize($instance, $node->langcode, $node->body[LANGUAGE_NOT_SPECIFIED][0], 'summary');

@@ -2310,8 +2369,8 @@ class NodeTokenReplaceTestCase extends NodeWebTestCase {
-    $tests['[node:body]'] = $node->body[$node->langcode][0]['value'];
-    $tests['[node:summary]'] = $node->body[$node->langcode][0]['summary'];
+    $tests['[node:body]'] = $node->body[LANGUAGE_NOT_SPECIFIED][0]['value'];
+    $tests['[node:summary]'] = $node->body[LANGUAGE_NOT_SPECIFIED][0]['summary'];

+++ b/core/modules/simpletest/drupal_web_test_case.phpundefined
+++ b/core/modules/simpletest/drupal_web_test_case.phpundefined
@@ -935,7 +935,7 @@ class DrupalWebTestCase extends DrupalTestCase {

@@ -935,7 +935,7 @@ class DrupalWebTestCase extends DrupalTestCase {
       'sticky'    => 0,
       'type'      => 'page',
       'revisions' => NULL,
-      'langcode'  => LANGUAGE_NOT_SPECIFIED,
+      'langcode'  => language_default()->langcode,
...
@@ -960,7 +960,7 @@ class DrupalWebTestCase extends DrupalTestCase {

@@ -960,7 +960,7 @@ class DrupalWebTestCase extends DrupalTestCase {
       'value' => $this->randomName(32),
       'format' => filter_default_format(),
     );
-    $settings['body'][$settings['langcode']][0] += $body;
+    $settings['body'][LANGUAGE_NOT_SPECIFIED][0] += $body;

Why do you need to change these?

+++ b/core/modules/system/tests/upgrade/upgrade.language.testundefined
@@ -77,7 +77,7 @@ class LanguageUpgradePathTestCase extends UpgradePathTestCase {
-    $this->assertNoFieldByName('langcode');
+    $this->assertFieldByName('langcode');

Note: this will fail once you actually remove the field for locked languages again. Needs to be set back as it was before.

+++ b/core/modules/translation/translation.installundefined
@@ -0,0 +1,21 @@
+  // Update node type language selectable variables, set to 'Selectable' from
+  // 'Selectable with translation support'.
+  $types = db_query('SELECT type FROM {node_type}')->fetchCol();
+  foreach ($types as $type) {
+    $translatable = variable_get('node_type_language_locked_' . $type, FALSE);
+    if ($translatable) {
+      variable_set('node_type_language_locked_' . $type, 0);
+    }
+  }

The comment is not accurate anymore? There does not seem to be such options in the patch. Also why would you do this? If the node type was language locked, you unlock it??!? When the translation module in uninstalled?

+++ b/core/modules/translation/translation.moduleundefined
@@ -120,10 +120,24 @@ function translation_permission() {
+/**
+ * Checks that translation is disabled when default language is none and
+ * default language is locked.
+ */
+function translation_node_type_language_translation_enabled_validate($element, &$form_state, $form){
+  if ($form_state['values']['node_type_language_default'] == 'und' && $form_state['values']['node_type_language_locked'] && $form_state['values']['node_type_language_translation_enabled'] == TRANSLATION_ENABLED) {
+    form_set_error('node_type_language_translation_enabled', t('Translation cannot be enabled if no default language is None and locked.'));
+  }

PHPdoc function comment summaries should be on one line only. Further explanation should go after an empty line and can be of any length.

Also I don't get the logic. If the language is locked, translation would not be possible in any case right? Regardless of what the type is locked to.

Also I don't get the error message. "no default language is None"?!

andypost’s picture

I'd suggest we hardcode "current content language")

What's about SLS?

+ global $language_interface;
Probably this should be drupal_container()->get(LANGUAGE_TYPE_INTERFACE);

Artusamak’s picture

Assigned: Artusamak » Unassigned

Unassigning myself from this task for now, i don't have time to work on it. :(

kbasarab’s picture

Assigned: Unassigned » kbasarab

Gonna look into this rerolling this. Seems like right now we just need to take #141 comments into consideratoin and reoll/incorporate that into #139.

Gábor Hojtsy’s picture

@kbasarab: Yes, looking for your updates, thanks.

kbasarab’s picture

Status: Needs work » Needs review
FileSize
27.08 KB

Posting strictly the reroll to make sure tests pass before jumping into Gábor's changes.

kbasarab’s picture

Assigned: kbasarab » Unassigned
FileSize
27.59 KB

Didn't change everything that you mentioned Gábor just a few of the main issues/styles. Let most of the questioning ones for some discussion.

Up to date in the reroll now though with the clean patch above and then patch here with some of the changes. Unassigning myself now until we have more definite ideas of changes.

Status: Needs review » Needs work

The last submitted patch, 25875-147.patch, failed testing.

Gábor Hojtsy’s picture

Priority: Normal » Major

@kbasarab: can you make sure it passes tests? Given that nobody had secod opinions in over two weeks and that this would be a cornerstone of moving forward with language support on entities, it would be good to move ahead instead of pretending some discussion is happening here.

kbasarab’s picture

@Gábor Yeah. Not sure why that didn't pass testing. But I"ll look into it and get it passing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
884 bytes
27.64 KB

Ok, so this was not a really big change. The comment language assignment was checking a variable not available anymore, so it never inherited the node language which made the tests obviously fail. Can you help with fixing the rest of the issues? This has been close to being committed several times, it should not need to endure this much pain if we stretch it a bit and get it in sooner than later. The more we tinker with it the more Drupal core changes and the less useful our work becomes.

kbasarab’s picture

FileSize
26.41 KB
26.41 KB

I haven't been able to get tests to run locally the past few days locally so I may need to do another reinstall of DB. Keep getting lots of AJAX errors whenever trying to run tests locally. Here is some changes though that we'll play roulette with to see if they pass. Interdiff is from #151 to #152.

kbasarab’s picture

FileSize
2.62 KB

Oops, uploaded wrong interdiff.

Status: Needs review » Needs work

The last submitted patch, 258785-152.patch, failed testing.

kbasarab’s picture

So my diffs are screwed up here. I think I submitted the wrong piece. What I get for rushing before an event. I'll circle back to this later on tonight or tomorrow.

kbasarab’s picture

Status: Needs work » Needs review
FileSize
27.69 KB
27.69 KB

Lets try this one.

Status: Needs review » Needs work

The last submitted patch, 258785-156-5.patch, failed testing.

kbasarab’s picture

Status: Needs work » Needs review
FileSize
28.03 KB

Just another test to see where I'm at. I had made an edit to WebTestBase.php last time and am not sure I should have done that. Just interested to see what failures this one gets if I revert just that one piece:

Status: Needs review » Needs work

The last submitted patch, 258785-158.patch, failed testing.

kbasarab’s picture

Status: Needs work » Needs review

Main change here is changing operator in webtestbase.php and remove change I had made in an earlier commit:

kbasarab’s picture

FileSize
28.03 KB

Helps to attach the patch.

Status: Needs review » Needs work

The last submitted patch, 258785-160.patch, failed testing.

kbasarab’s picture

Status: Needs work » Needs review
FileSize
25.81 KB

And again. Rerolled and its passed all the tests localled except for a page caching one which was failing on core 8.x for me as well.

kbasarab’s picture

Assigned: Unassigned » kbasarab

A pass. Could someone review again to ensure all the style changes and everything looks right on this? I've been looking through the code so much I'm not sure which end is up anymore.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Thanks again for working on this patch. Here is my review of the current patch:

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleMultilingualFieldsTest.phpundefined
@@ -44,10 +44,10 @@ class LocaleMultilingualFieldsTest extends WebTestBase {
-    $this->assertRaw(t('The content type %type has been updated.', array('%type' => 'Basic page')), t('Basic page content type has been updated.'));
+    $this->assertRaw(t('The content type %type has been updated.', array('%type' => 'Basic page')), t('The content type Basic page has been updated.'));

We don't need to change the last t()-ed string, it is just a confirmation message in the test results, does not need to be identical to what was looked for (1st string).

+++ b/core/modules/node/content_types.incundefined
@@ -185,11 +185,35 @@ function node_type_form($form, &$form_state, $type = NULL) {
+    $lang_options = array(
+      LANGUAGE_NOT_SPECIFIED => t('- None -'),

There are various other LANGUAGE_* constants now that denote special languages. Those should be supported here too.

+++ b/core/modules/node/content_types.incundefined
@@ -185,11 +185,35 @@ function node_type_form($form, &$form_state, $type = NULL) {
+      'site_default' => t("Site's default"),
+      'current' => t('Current content language'),
+      'user_default' => t("User's default"),

"User's preferred language" would be a better name. When a user is edited it is not presented as their "default" language, but rather as their "preferred".

+++ b/core/modules/node/content_types.incundefined
@@ -185,11 +185,35 @@ function node_type_form($form, &$form_state, $type = NULL) {
+      '#title' => t('Lock language'),
+      '#default_value' => variable_get('node_type_language_locked_' . $type->type, TRUE),
+      '#description' => t('Users will not be able to change the default language on content of this type.'),

I don't know how often I said this above, but it feels like quite a few times :) Let's not make this "*locked* and appear" but make it *hidden* (locked and not appear).

At least make and explain the behavior like that. I'm not 100% sure "hidden_" is a good replacement for "locked_" in the variable names, but it likely is.

I looked but could not find in this patch now the place where this locked feature is enforced.

+++ b/core/modules/node/node.moduleundefined
@@ -627,6 +627,47 @@ function node_field_extra_fields() {
+  if ($default_value == LANGUAGE_NOT_SPECIFIED) {
+    return LANGUAGE_NOT_SPECIFIED;
+  }
+
+  drupal_container()->get(LANGUAGE_TYPE_INTERFACE);

You wanted to assign the drupal_container() return value to $language_interface locally?

Also, the UI says it would be the **content** language, not the UI language. So use LANGUAGE_TYPE_CONTENT!

+++ b/core/modules/translation/translation.moduleundefined
@@ -120,10 +120,23 @@ function translation_permission() {
+  $form['language']['node_type_language_translation_enabled'] = array(
+    '#type' => 'checkbox',
+    '#title' => t('Enable translation'),
+    '#return_value' => TRANSLATION_ENABLED,
+    '#default_value' => variable_get('node_type_language_translation_enabled_' . $form['#node_type']->type, FALSE),
+    '#element_validate' => array('translation_node_type_language_translation_enabled_validate'),
+    '#prefix' => "<label>" . t('Translation') . "</label>",

Why not hide this with #states when not valid (additionally to the error message on validation)?

+++ b/core/modules/translation/translation.moduleundefined
@@ -120,10 +120,23 @@ function translation_permission() {
+  if ($form_state['values']['node_type_language_default'] == 'und' && $form_state['values']['node_type_language_locked'] && $form_state['values']['node_type_language_translation_enabled'] == TRANSLATION_ENABLED) {
+    form_set_error('node_type_language_translation_enabled', t('Translation cannot be enabled if no default language is set and locked.'));

If the based language is of any other special languages (see above), that would also not allow it to be translatable.

kbasarab’s picture

Haven't had a change to work on this yet but could you elaborate some on this part. Without dealing with this entire issue though I've still kinda missed what the objective of this part is:

+++ b/core/modules/node/content_types.incundefined
@@ -185,11 +185,35 @@ function node_type_form($form, &$form_state, $type = NULL) {
+      '#title' => t('Lock language'),
+      '#default_value' => variable_get('node_type_language_locked_' . $type->type, TRUE),
+      '#description' => t('Users will not be able to change the default language on content of this type.'),

I don't know how often I said this above, but it feels like quite a few times :) Let's not make this "*locked* and appear" but make it *hidden* (locked and not appear).

At least make and explain the behavior like that. I'm not 100% sure "hidden_" is a good replacement for "locked_" in the variable names, but it likely is.

kbasarab’s picture

Status: Needs work » Needs review
FileSize
8.32 KB
26.1 KB
+++ b/core/modules/node/node.moduleundefined
@@ -627,6 +627,47 @@ function node_field_extra_fields() {
+  if ($default_value == LANGUAGE_NOT_SPECIFIED) {
+    return LANGUAGE_NOT_SPECIFIED;
+  }
+
+  drupal_container()->get(LANGUAGE_TYPE_INTERFACE);

You wanted to assign the drupal_container() return value to $language_interface locally?

I'm not sure what the intention was here. I did change it to LANGUAGE_TYPE_CONTENT though.

RobLoach’s picture

Status: Needs review » Needs work
+++ b/core/modules/node/node.moduleundefined
@@ -644,7 +644,7 @@ function node_type_get_default_langcode($node_type) {
   }
 
-  drupal_container()->get(LANGUAGE_TYPE_INTERFACE);
+  drupal_container()->get(LANGUAGE_TYPE_CONTENT);

You actually probably don't even need to call this as the object will be initialized the first time the ->get() is called. If you just remove that line of code, the tests should still pass.

kbasarab’s picture

Ok. Thanks Rob. One other thing I meant to ask about was the form #state in:

'#states' => array(
+      'invisible' => array(
+        ':input[name="node_type_language_default"]' => array('value' => 'und'),
+      ),
+    ),

Since we added support for the other constants and wanting those not to trigger this field can we do a form state invisibile on an or value? Seemed like from what I researching there is a patch for that but not in core yet.

Gábor Hojtsy’s picture

@kbasarab: I think we'd "only" need to have three values, so you'd only need to repeat the :input condition array 3 times, which might be fine here. If you are interested in working on the *or* condition for states, that also sounds cool, but we've been working on this for *long* and we'd really need to get this in to generalize to entities sooner than later. I hoped we can make this quick and move on to the more general case after (so we don't also need to migrate all settings and figure out form integration and settings integration for other entities, but the longer this lasts, the more it makes sense not to limit its scope, and the harder it will get to any change in :/)

kbasarab’s picture

Yeah, Definitely am not wanting to expand scope. Just want to get this done and ready. The issue I saw though is when I duplicated the input condition array it did not work for states. So the only one that actually becomes invisible is the option for none.

If I tried duplicating the input condition three times it would overwrite the array key each time:

'#states' => array(
+      'invisible' => array(
+        ':input[name="node_type_language_default"]' => array('value' => 'und'),
+        ':input[name="node_type_language_default"]' => array('value' => 'zxx'),
+      ),
+    ),

So in the example above only the N/A (zxx) would become invisibile via #states. If I make 'value' an array it doesn't work at all as #states doesn't support this.

I can just leave it as is though and have "None" become invisible and the Multiple or N/A continue to show.

I'll reroll today with Rob's change included and tested and get it back ready for another review.

Gábor Hojtsy’s picture

Right, right, it overwrites it. I think it is not good if we don't hide it when it does not make sense to show. So I don't think just hiding it for 'und' is a good intermediate step. Let's try to figure out some clever solution. We can put in a little custom JS I think to the same effect. There are tricks like the regex states trick from http://evolvingweb.ca/story/extending-form-api-states-regular-expressions, but they are not core-worthy as-is. :/

Gábor Hojtsy’s picture

In other words, I think we should have a few lines of custom JS and note there that it should be refactored once #states is more versatile.

kbasarab’s picture

Status: Needs work » Needs review
FileSize
2.36 KB
26.81 KB

This adds some jQuery to the form for removing items and rolls in Rob Loach's change.

Schnitzel’s picture

FileSize
30.12 KB

new patch, letting the testbot doing the hard work and run all test, because some of them might fail.

Status: Needs review » Needs work

The last submitted patch, 258785-175.patch, failed testing.

Schnitzel’s picture

Status: Needs work » Needs review
FileSize
27.34 KB

patch was wrong way around, reroll

Status: Needs review » Needs work

The last submitted patch, 258785-176.patch, failed testing.

Schnitzel’s picture

Status: Needs work » Needs review
Issue tags: +JavaScript
FileSize
21.72 KB
61.53 KB

so forgot to adapt some more tests.

Changelog for this new patch, thanks for @LoMo to help me on this one.

  • Completely renamed Locked to Hidden, also in the code, in tests, etc
  • We had a discussion about the language selector, which could confuse People. Because we add an explanation about the special languages in the language list we link to this with an link in the description, see #1471432: Rework language_list(), let people use more special languages
  • fixxed some naming Problems, like showin "Not applicable" instead of "- N/A -". And using better keys like current_interface instead of current
  • Fixxed the Javascript on the ContentType Form, there where two JS content_types.js and language_types.js which tried to do the same, now using a new File translation.js which hides the whole "translation" part in the Form if special language is selected and the "hidden" checkbox selected
  • Fixxed and adapted Test: NodeTypeInitialLanguageTest (had wrong Name: NodeTypeInitalLanguageTestCase), because we actually hide the language selector on node form and not only disable it, if "hidden" checkbox is selected

Adding JavaScript Tag for @nod_ because we definitely need some review of the translation.js file.

Status: Needs review » Needs work

The last submitted patch, 258785-179.patch, failed testing.

Schnitzel’s picture

FileSize
1.89 KB
82.21 KB

oh thanks testbot :)

Found a bug in the node_update_8003 upgrade script, it does not unhide the language selector if translation was enabled before, which needs to be done if translation should be possible

Schnitzel’s picture

Assigned: kbasarab » Schnitzel
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 258785-181.patch, failed testing.

Schnitzel’s picture

FileSize
81.97 KB

ohh PSR-0 patches broke it, reroll

Schnitzel’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 258785-184.patch, failed testing.

Schnitzel’s picture

Status: Needs work » Needs review
FileSize
28.12 KB

rerolled, because another PSR-0 came into my way and it had some strange crap which shouldnt be there.

RobLoach’s picture

Looking very nice! It would be great to take a closer look to see if we could accomplish what the JavaScript does with States. Might need some closer looking into. Leaving needs review so other people could have a quick look.

+++ b/core/modules/translation/translation.jsundefined
@@ -0,0 +1,21 @@
+
+"use strict";

Use strict?

+++ b/core/modules/translation/translation.jsundefined
@@ -0,0 +1,21 @@
+  }
+};
+
+})(jQuery);
\ No newline at end of file

Would be good to have a newline here to avoid the git warnings.

nod_’s picture

Use strict. https://drupal.org/node/1570578

That's true that #states might be the way to go for translate.js

Schnitzel’s picture

FileSize
28.09 KB

So here is a new patch which fixxes the new line, thx @Rob Loach.

@nod_
will be okay for you if we get this patch in and we fix the translate.js JS review in a follow up?
It's a pretty important patch to have in and in my opinion the JS can also be done later :)) and of course will not put preasure on you

Schnitzel’s picture

FileSize
28.22 KB

reroll because of PSR-0 commits

nod_’s picture

I'd rather not but I don't want to hold back a big major patch just for that. I will hunt you down and make you review and RTBC the follow-up patch. Otherwise the clean up will never happen.

Be prepared :)

Schnitzel’s picture

#191: 258785-191.patch queued for re-testing.

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

The last submitted patch, 258785-191.patch, failed testing.

Schnitzel’s picture

Schnitzel’s picture

Status: Needs work » Needs review
nod_’s picture

JS is fine for me like that, it should be changed overall as a follow-up of #1574470: Selectors clean-up.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ b/core/modules/node/content_types.incundefined
@@ -185,11 +185,42 @@ function node_type_form($form, &$form_state, $type = NULL) {
+    $lang_descriptions = array(
+      LANGUAGE_NOT_SPECIFIED => t('Use this setting if the language is not (yet) specified'),
+      LANGUAGE_NOT_APPLICABLE => t('Use this setting if an language assignement is not applicable'),
+      LANGUAGE_MULTIPLE => t('Use this setting if a single content contains multiple languages'),
+    );

You are not using this code anymore. It was in an earlier version of the patch that you used it, but not anymore.

+++ b/core/modules/node/content_types.incundefined
@@ -185,11 +185,42 @@ function node_type_form($form, &$form_state, $type = NULL) {
+      '#description' => t('Explanation of the language options is found on the <a href="@languages_list_page">languages list page.</a>', array('@languages_list_page' => url('admin/config/regional/language'))),

I think we do links "European style" (do not include the sentence ending dot) usually. Just the text :)

+++ b/core/modules/node/content_types.jsundefined
@@ -21,6 +21,15 @@ Drupal.behaviors.contentTypes = {
+    $('fieldset#edit-language', context).drupalSetSummary(function(context) {
+      var vals = [];
+
+      $('input:checked', context).next('label').each(function() {
+        vals.push(Drupal.checkPlain($(this).text()));
+      });
+
+      return vals.join(', ');
+    });

Sounds like this would not include the text from the select box, just the checkbox.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeTypeInitialLanguageTest.phpundefined
@@ -0,0 +1,70 @@
+/**
+ * @file
+ * Definition of Drupal\node\Tests\NodeTypeInitalLanguageTest.
+ */

Inital => Initial

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeTypeInitialLanguageTest.phpundefined
@@ -0,0 +1,70 @@
+    return array(
+        'name' => 'Node type initial language',
+        'description' => 'Tests node type initial language settings.',
+        'group' => 'Node',
+        );
...
+    // Adds a new language and set it as default.
+    $edit = array(
+        'predefined_langcode' => 'hu',
+        );
+    $this->drupalPost('admin/config/regional/language/add', $edit, t('Add language'));
+    $edit = array(
+        'site_default' => 'hu',
+        );
+    $this->drupalPost('admin/config/regional/language', $edit, t('Save configuration'));
+
+    // Tests the initial language after changing the site default language.
+    // First unhide the language selector
+    $edit = array(
+        'node_type_language_hidden' => FALSE,
+        );
...
+    // Changes the inital language settings.
+    $edit = array(
+        'node_type_language_default' => 'en',
+        );

Whitespace issues. Too much space.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeTypeInitialLanguageTest.phpundefined
@@ -0,0 +1,70 @@
+    $this->assertField('langcode', "Language is selectable on node add/edit page when language not hidden.");

Use single quotes.

+++ b/core/modules/node/node.installundefined
@@ -458,9 +458,12 @@ function node_uninstall() {
+     // TODO: node_type_language_translation_enabled_ in the translation module
+     // ->condition('name', 'node_type_language_translation_enabled_' . $type)

Is this not done yet?!?

+++ b/core/modules/node/node.installundefined
@@ -543,6 +546,29 @@ function node_update_8002() {
+      // Translation was enabled, so enable it again and
+      // unhide the language selector. Because if language is
+      // LANGUAGE_NOT_SPECIFIED and the selector hidden, translation
+      // cannot be enabled.
+      variable_set('node_type_language_hidden_' . $type, FALSE);
+      variable_set('node_type_language_translation_enabled_' . $type, 2);

Why is translation enabled still 2 and not TRUE/FALSE in the new code? For brand new sites it sounds like it would be a checkbox, so only updated sites get 2??!

+++ b/core/modules/node/node.moduleundefined
@@ -629,6 +629,47 @@ function node_field_extra_fields() {
+ * @return (string)

No brackets for return type I think(?)

+++ b/core/modules/node/node.pages.incundefined
@@ -199,13 +199,15 @@ function node_form($form, &$form_state, Node $node) {
-      // New nodes without multilingual support get the default language, old
-      // nodes keep their language if language.module is not available.
-      '#value' => !isset($form['#node']->nid) ? language_default()->langcode : $node->langcode,
+      // Use default language when language module is inactive.
+      '#value' => $node->langcode,

How is that we just magically get the $node now, and did not do before?!

+++ b/core/modules/translation/translation.moduleundefined
@@ -119,11 +119,30 @@ function translation_permission() {
+/**
+ * Checks if default language=none && default language=locked
+ */
+function translation_node_type_language_translation_enabled_validate($element, &$form_state, $form){
+  $language_default = (($form_state['values']['node_type_language_default'] == 'und' || $form_state['values']['node_type_language_default'] == 'zxx' || $form_state['values']['node_type_language_default'] == 'mul') ? TRUE : FALSE);
+  if ($language_default && $form_state['values']['node_type_language_hidden'] && $form_state['values']['node_type_language_translation_enabled'] == TRANSLATION_ENABLED) {
+    form_set_error('node_type_language_translation_enabled', t('Translation cannot be enabled if no default language is set and locked.'));

Let's make the comment human readable :) Include dot at the end.

The first line is pretty convoluted, and definitely does not need to be a ternary operators there, the value itself will be TRUE or FALSE as needed without the ternary :) Could be dramatically shortened with an in_array() equivalent.

Also, $language_default is a misleading name, let's have a better one. We have an API function for this in the language_list() extension patch at #1471432: Rework language_list(), let people use more special languages, so we may want to add a TODO here to use that if this one is committed earlier.

Schnitzel’s picture

FileSize
15.33 KB
28.31 KB

Implemented Feedback of @Gabor

Changelog

  • removed TRANSLATION_ENABLED completely and using now TRUE instead of "2"
  • removed $lang_descriptions
  • added some todos to be done when #1471432: Rework language_list(), let people use more special languages is commited
  • Fxxing Whitespace Issues
  • Enabled deleting of node_type_language_translation_enabled_ variables on node_uninstall()
  • #default_value of lancode field on node edit/add form is now $node->langcode, because it is set earlier
  • Much better documentation and way how to check that translation is only enabled when allowed
Schnitzel’s picture

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

Status: Needs review » Needs work

Only found two minor issues now.

+++ b/core/modules/translation/translation.moduleundefined
@@ -119,11 +114,31 @@ function translation_permission() {
+/**
+ * Checks if language is set to one of the special languages and
+ * language selector is not hidden, translation cannot be enabled.
+ */

phpdoc for functions should be on one line.

+++ b/core/modules/translation/translation.moduleundefined
@@ -119,11 +114,31 @@ function translation_permission() {
+  // @todo Change this to language_is_locked() when #1471432 is commited

dot missing at end of line.

Schnitzel’s picture

Status: Needs work » Needs review
FileSize
4.07 KB
30.27 KB

fixxed issues found by @Gabor
also found some other Codestyle issues and better explanation of "Translation is not supported"

Gábor Hojtsy’s picture

Typos / code style problems in the interdiff still.

+++ b/core/modules/node/content_types.incundefined
@@ -186,7 +186,7 @@ function node_type_form($form, &$form_state, $type = NULL) {
-    // @Todo use language_special_languages() if #1471432 is commited
+    // @Todo use language_special_languages() if #1471432 is commited.

@todo (not uppercase) and comitted (two t).

+++ b/core/modules/translation/translation.moduleundefined
@@ -130,14 +130,14 @@ function translation_form_node_type_form_alter(&$form, &$form_state) {
 /**
- * Checks if language is set to one of the special languages and
- * language selector is not hidden, translation cannot be enabled.
+ * Checks if language is set to one of the special languages and language selector is not hidden, translation cannot be enabled.
  */

The solution is not to make it longer unfortunately but to make a shorter initial sentence and write the longer explanation in a second sentence, separating the two by a newline.

+++ b/core/modules/translation/translation.moduleundefined
@@ -130,14 +130,14 @@ function translation_form_node_type_form_alter(&$form, &$form_state) {
+    // @todo use language_special_languages() if #1471432 is commited.

comitted

Schnitzel’s picture

FileSize
30.31 KB
2.13 KB

fixed!

svendecabooter’s picture

I was testing this earlier, and most of the feedback I listed seems fixed in #199 and #202 now.
Below still some remarks:

* Under the select list the description says "Explanation of the language options is found on the languages list page."
When clicking the link I see no documentation at all referring to these options. Is this part of another patch that hasn't landed yet?
It is quite misleading to expect more information, when you just get redirected to a page with a list of languages...

* I think the two checkboxes could use a small description text to make more clear what the effect of (un)checking them will be.

Gábor Hojtsy’s picture

@sven: yes, the language list patch from #1471432: Rework language_list(), let people use more special languages adds info on those special languages. We can remove the link here, but we expect both this one and that one to hopefully be committed this week, so it seems logical to keep that in mind, instead of messing with followups for them. For the checkboxes, the question is if we can give you real value with more description. We had @Bojhan review this earlier, maybe need to have 1-2 more people to figure out if the options were clear or not. Can we do this sooner than later (as in *now*?).

Schnitzel’s picture

* Under the select list the description says "Explanation of the language options is found on the languages list page."
When clicking the link I see no documentation at all referring to these options. Is this part of another patch that hasn't landed yet?

Yes, this will be way better if #1471432: Rework language_list(), let people use more special languages has landed

* I think the two checkboxes could use a small description text to make more clear what the effect of (un)checking them will be.

Any ideas what you would write? We tried to make it as short as possible and as logical as possible

Schnitzel’s picture

Okay, made some usability reviews with 3 people here at the #d8mi sprint.

Overall all of them could tell me that:

"Hide language selector" is for not allowing the author of a node to change the language

"Enable translation" enables the possibility to translate the node

So basically we don't need more explanation. But what caused the most problems are the new three special languages in D8, but which is not the topic of this patch.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Which means, this now looks like good to go. :)

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Now that #1471432: Rework language_list(), let people use more special languages is in, it sounds like we can make this patch simpler. marking needs work for that.

Schnitzel’s picture

Status: Needs work » Needs review
FileSize
4.47 KB
30.02 KB

okay, rerolled the patch and resolved all the @todo's

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed it again, looks good to me now.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, huge thanks to Schnitzel and Gábor for patiently walking me through this issue's functionality.

The patch looks bigger than you would think based on the issue description, but a lot of that is due to a brand new set of tests, as well as the addition of new contextual default values like "User's preferred language" to the options, which should help a lot for various use cases.

We reviewed the usability of this together, and while the list of "not actual languages" in the language list is a bit sad, the only one we could really figure out that could be removed is "Multiple languages," since the semantic difference between that and "Not specified" I don't think is valuable enough to clutter the interface further. But that can completely be a follow-up patch.

So.... committed and pushed to 8.x. Yay! :D

Gábor Hojtsy’s picture

We should have a changelog entry for this as well as a change notice node. Anybody up for those?

Schnitzel’s picture

We should have a changelog entry for this as well as a change notice node. Anybody up for those?

will do one. but give me about 48h =)

Gábor Hojtsy’s picture

Title: Provide more flexible settings for initial language on content types » CHANGELOG: Provide more flexible settings for initial language on content types
Status: Fixed » Needs review
FileSize
574 bytes

Cleaning out issue board. Changelog patch. :) Please RTBC if looks good.

Added change notice at http://drupal.org/node/1649382, should hopefully be good.

Kristen Pol’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed the CHANGELOG.txt to 8.x. Thanks!

Gábor Hojtsy’s picture

Issue tags: -Needs tests, -sprint

Superb, thanks all.

Gábor Hojtsy’s picture

Title: CHANGELOG: Provide more flexible settings for initial language on content types » Provide more flexible settings for initial language on content types

Status: Fixed » Closed (fixed)

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

andypost’s picture

andypost’s picture

Issue summary: View changes

comment link to #116