Move field language fallback code from Locale to Field and field language handling for nodes to Node

A first patch to be reviewed.

The main changes I made:
1. I moved the locale_entity_info_alter() to the node module (basically put the translation handler in the hook_entity_info()), and also move the submit handler that was attached to the node form in the locale_form_node_form_alter() also in the node module because all that logic should reside in the node module and not the field module, IMO. Also, the field module is a Drupal required field, so it will be always enabled.

2. The locale_field_language_alter() and locale_field_language_fallback() were moved into the field module.

For the 4th point, I implemented the change in the attached patch.
For the 3rd point, I wrapped now that translation handler into a module_exists() check, so, if the language module is not enabled, the field_language translation handler should not be assigned to the node.
By doing this, basically the 2nd point is also solved, because the field_language_fallback() function is called only if the entity type has the field_language translation handler attached (except the case when a custom module would alter the entity definition and always add the translation handler...)

For the 1st point, I think the root of the issue is in the field_language() function, in field_multilingual.inc file. There, the $display_langcode is an array and not just a "simple" value. So, either we change the name of that variable to something like $field_display_langcodes (because $display_langcodes is already used), or change the field_language_fallback() function and use $display_langcode instead of $field_langcodes. I would choose the first option, what do you think?

#4: drupal-language_negotiation-1630264-4.patch queued for re-testing.

+++ b/core/modules/field/field.moduleundefined
@@ -369,6 +369,61 @@ function field_system_info_alter(&$info, $file, $type) {
+ * Core language fallback rules simply check if fields have a field translation
+ * for the requested language code. If so the requested language is returned,

There should be a comma after "If so" => "If so, the requested language ..."

+++ b/core/modules/node/node.pages.incundefined
@@ -524,6 +527,31 @@ function node_form_submit($form, &$form_state) {
 /**
+ * Checks if field_language is registered as a translation handler.
+ *
+ * If so, handle possible node language changes.

I don't like this function description.

"Handles possible node language changes if 'field_language' is registered as a translation handler."

+++ b/core/modules/node/node.pages.incundefined
@@ -524,6 +527,31 @@ function node_form_submit($form, &$form_state) {
+
+      // Handle a possible language change: new language values are inserted,
+      // previous ones are deleted.

New (capital letter after the ":").

+++ b/core/modules/field/field.moduleundefined
@@ -369,6 +369,61 @@ function field_system_info_alter(&$info, $file, $type) {
+  $field_languages = array();

Unused variable.

The last version of the patch is from me but it just changes a few comments and I already discussed it with the others at the sprint and it seems to be RTBC material now.

+++ b/core/modules/field/field.api.php
@@ -1473,8 +1473,8 @@ function hook_field_attach_prepare_translation_alter(&$entity, $context) {
+  if (variable_get('field_language_fallback', TRUE) && field_has_translation_handler($context['entity_type'], 'field_language')) {

+++ b/core/modules/field/field.module
@@ -369,6 +369,60 @@ function field_system_info_alter(&$info, $file, $type) {
+  if (variable_get('field_language_fallback', TRUE) && field_has_translation_handler($context['entity_type'], 'field_language')) {

+++ b/core/modules/field/tests/modules/field_test/field_test.entity.inc
@@ -128,9 +128,9 @@ function field_test_entity_info_alter(&$entity_info) {
+    $entity_info[$entity_type]['translation']['field_language'] = FALSE;

+++ b/core/modules/node/node.module
@@ -226,6 +226,11 @@ function node_entity_info() {
+    $return['node']['translation']['field_language'] = TRUE;

+++ b/core/modules/node/node.pages.inc
@@ -524,6 +527,30 @@ function node_form_submit($form, &$form_state) {
+  if (field_has_translation_handler('node', 'field_language')) {

The field translation handler name is supposed to be a module name. Actually Node is providing the code dealing with node field languages, hence we should probably go with 'node' instead of 'field_language'. I'd suggest to just remove the translation handler name from the field_has_translation_handler() call in field_field_language_alter(), this will provide language fallback for every entity type if there's at least a translation handler enabled for it.

BTW, when we have an entity translation UI in core I would just drop the field translation handler concept altogether, since it has been introduced with the idea that we would have no UI in core for translatable fields.

+++ b/core/modules/field/tests/field.test
@@ -3414,3 +3414,130 @@ class EntityPropertiesTestCase extends FieldTestCase {
+
+/**
+ * Functional test for multilingual fields.
+ */
+class FieldMultilingualTestCase extends WebTestBase {

field.test already has a bunch of tests for generic field language support. This one should totally go in node.test.

Attached a patch to implement the changes from #8.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeFieldMultilingualTestCase.php
@@ -0,0 +1,137 @@
\ No newline at end of file

Just a minor thing: no new line at the end of the file. Otherwise looks good, great work!

For some reason, after making the diff on the latest d8 version, the patch got much bigger, but it should work as before...

#11 actually deletes LocaleMultilingualFieldsTest, which actually #9 didn't do. RTBC if the bot is happy.

+++ b/core/modules/field/field.installundefined
@@ -6,6 +6,14 @@
 /**
+ * Implements hook_uninstall().
+ */
+function field_uninstall() {
+  // Clear some variables.
+  variable_del('field_language_fallback');

Field is a required module, can never be uninstralled, and I'm sure it has other variables that aren't cleaned up like this, so this should be removed unless we start properly putting in hook_uninstall() to all required modules.

+++ b/core/modules/field/field.moduleundefined
@@ -369,6 +369,60 @@ function field_system_info_alter(&$info, $file, $type) {
+function field_field_language_alter(&$display_langcode, $context) {
+  // Do not apply core language fallback rules if they are disabled or if
+  // the entity does not have a translation handler registered.
+  if (variable_get('field_language_fallback', TRUE) && field_has_translation_handler($context['entity_type'])) {
+    field_language_fallback($display_langcode, $context['entity'], $context['langcode']);
+  }

Is it OK to continue defaulting to TRUE here if this is baked into the field system. If I only have one language on my site won't that mean extra code to run?

Also any particular reason to have field module implementing it's own alter hook?

For the first comment, I will remove the hook.

For the 2nd observation: I think it is good to default to TRUE, even if there is some extra code to run in case there is only one language, because then if the site has more languages, then people will have to know that they must set that variable to TRUE in order to have this functionality.
Regarding field implementing it's own alter, dont't really have a particular, strong, reason for that... I can move the code into the field_language() function itself if you think this would be better...

I think you forgot to upload your patch or accidently set it to 'needs review'

yes, accidently set it to "needs review", I think it was preselected...

Attached a patch with these changes:
- removed the hook_uninstall for the field module.
- removed hook_field_language_alter() from the field module and put the logic in the field_language() itself.
- by default, "field_language_fallback" is FALSE and the value is updated in hook_language_insert() and hook_language_delete().

#19: drupal-language_negotiation-1630264-19.patch queued for re-testing.

Rerolled the patch for the latest version of d8

Cool, this looks like sensible clean-up.

I had one question about this:

 function field_language_delete() {
   field_info_cache_clear();
+  // If the number of languages is less than 2, disable the core language
+  // fallback rules.
+  // Because the language_count is updated after the hook is invoked, we check
+  // if the language_count is less or equal with 2 at the current time.
+  if (variable_get('language_count', 1) <= 2) {
+    variable_set('field_language_fallback', FALSE);
+  }
 }

I was worried that this would result in a lot of unnecessary variable_set() calls, but it was pointed out that this hook only fires when languages are deleted/inserted manually, which will be relatively rare.

Committed and pushed to 8.x. Thanks! :)

Filed related issue #1977790: Drop node_entity_info_alter() as unused
Seems translation key could be now removed in favor of controllers