This issue is a spin-off of #539110: TF #4: Translatable fields UI. If the translatable fields UI does not get committed it will live in contrib, but we absolutely need to complete the multilingual support for fields in core.

We need:

  • integration between node multilingual support provided by locale and the multilingual capabilities of fields
  • body translatable
  • language negotiation support
  • language fallback support

All this requirements are deeply interconnected and are difficult to treat in separate patches without breaking the tests so the following patch will provide a unique, coherent, solution. If don't solve the issues above translatable fields will be almost unusable.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

Status: Active » Needs review
FileSize
10.67 KB

This is should be almost ok, but tests are not fixed.
Let's see what the test bot has to say.

Status: Needs review » Needs work

The last submitted patch failed testing.

peximo’s picture

Status: Needs work » Needs review
FileSize
13.81 KB

The attached patch should fix the tests. It also introduces a new test case to cover the multilingual fields new code.

Status: Needs review » Needs work

The last submitted patch failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
17.12 KB

There was a missing hunk in the previous patch.

sun’s picture

plach’s picture

The current patch suppress field_multilingual_get_items as its behavior was deemed prone to failures in #557292-44: TF #3: Convert node title to fields.

Status: Needs review » Needs work

The last submitted patch failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
14.67 KB

mmh, rerolled with eclipse

Status: Needs review » Needs work

The last submitted patch failed testing.

peximo’s picture

Status: Needs work » Needs review
FileSize
15.54 KB

Re-rolled

Status: Needs review » Needs work

The last submitted patch failed testing.

peximo’s picture

Rerolled with eclipse

peximo’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
16.37 KB

Rerolled. Now this depends on #590590: field_create_field ignores the 'translatable' property to work correctly.

Status: Needs review » Needs work

The last submitted patch failed testing.

plach’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
12.89 KB

rerolled

plach’s picture

wrong patch

bjaspan’s picture

This patch contains a fairly small change to modules/field/field.multilingual.inc and a much larger set of changes outside of module/field. Might you get more/better reviews if you re-categorize the issue?

plach’s picture

Issue tags: +i18n, +i18n sprint

@bjaspan: well TF is a pretty mixed matter, and most of the changes on locale are field related.
Let's see if we have better luck this way.

Status: Needs review » Needs work

The last submitted patch failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
15.64 KB

Rerolled. Actually it's CNW.

Status: Needs review » Needs work

The last submitted patch failed testing.

webchick’s picture

Just subscribing for now so I don't lose track of this.

plach’s picture

Status: Needs work » Needs review
FileSize
23.32 KB

If the bot does not complaints we sholud be ready for review.

Status: Needs review » Needs work

The last submitted patch failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
23.9 KB

mmh, I have no problem to apply the patch, let's retry...

Status: Needs review » Needs work

The last submitted patch failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
25.27 KB

wow, let's see if tests are fixed

Status: Needs review » Needs work

The last submitted patch failed testing.

plach’s picture

This one should fix the tests.

"Help with rolling this patch into RTBC as quickly as possible would be highly appreciated."

plach’s picture

Status: Needs work » Needs review
FileSize
27.47 KB

this one...

sun’s picture

This looks very good already.

+++ includes/bootstrap.inc	14 Oct 2009 08:46:38 -0000
@@ -1657,6 +1657,13 @@ function drupal_language_types() {
+function drupal_multilingual() {
+  return variable_get('language_count', 1) > 1;
+}

Should we statically cache this?

+++ includes/language.inc	14 Oct 2009 11:33:04 -0000
@@ -396,3 +396,37 @@ function language_url_split_prefix($path
+/**
+ * Return the possible fallback languages ordered by language negotiation
+ * settings and language weight.

Summary should not wrap at 80 chars.

+++ includes/locale.inc	14 Oct 2009 12:59:55 -0000
@@ -159,8 +159,10 @@ function locale_languages_overview_form_
-  // Changing the language settings impacts the interface.
+  // Changing the language settings impacts the interface and the fieldable
+  // entities info.
   cache_clear_all('*', 'cache_page', TRUE);
+  cache_clear_all('field_info_types', 'cache_field');
@@ -362,6 +364,9 @@ function locale_languages_predefined_for
+  // Changing the enabled languages impacts the fieldable entities info.
+  cache_clear_all('field_info_types', 'cache_field');
@@ -459,6 +464,8 @@ function locale_languages_delete_form_su
+    // Changing the enabled languages impacts the fieldable entities info.
+    cache_clear_all('field_info_types', 'cache_field');

We should invoke a hook here instead and move the cache flushing into field module instead.

We can use a single/unique hook name, like hook_language_settings_alter(), for example.

+++ modules/field/field.info.inc	14 Oct 2009 08:46:38 -0000
@@ -147,7 +147,7 @@ function _field_info_collate_types($rese
             $entity_info += array(
               'cacheable' => TRUE,
-              'translation_handlers' => array(),
+              'translation' => array(),
               'bundles' => array(),
             );
+++ modules/simpletest/tests/field_test.module	14 Oct 2009 14:12:46 -0000
@@ -93,8 +93,13 @@ function field_test_entity_info() {
 function field_test_entity_info_alter(&$entity_info) {
+  // Enable/disable field_test as a translation handler.
   foreach (field_test_entity_info_translatable() as $obj_type => $translatable) {
-    $entity_info[$obj_type]['translation_handlers']['field_test'] = TRUE;
+    $entity_info[$obj_type]['translation']['field_test'] = $translatable;
+  }
+  // Disable locale as a translation handler.
+  foreach (field_info_fieldable_types() as $obj_type => $info) {
+    $entity_info[$obj_type]['translation']['locale'] = FALSE;
   }
 }

I'm not sure whether we shouldn't keep the old key name... "translation" on its own is not kinda descriptive..

We can go with "translation" though, in case we don't have a better idea now.

+++ modules/field/field.multilingual.inc	14 Oct 2009 13:27:46 -0000
@@ -71,10 +73,12 @@ function field_multilingual_content_lang
+ * If no handler is passed, simply check if there is any translation handler
+ * enabled for the give entity type.

s/give/given/

+++ modules/field/field.multilingual.inc	14 Oct 2009 13:27:46 -0000
@@ -82,9 +86,22 @@ function field_multilingual_content_lang
+  else if (isset($obj_info['translation'])) {

s/else if/elseif/

+++ modules/locale/locale.module	14 Oct 2009 12:51:08 -0000
@@ -386,13 +386,24 @@ function locale_form_node_type_form_alte
 /**
+ * Returns whether the given content type has multilingual support.
+ *
+ * @return
+ *   Boolean value.
+ */
+function locale_multilingual_node_type($type) {
+  return (bool) variable_get('language_content_type_' . $type, 0);
+}
+
+

1) The description of @return is a bit lacking.

2) Normally, we pass a fully loaded $type object instead of $type->type. So we should do this here as well.

3) Minor: Duplicate blank line after the function.

+++ modules/locale/locale.module	14 Oct 2009 12:51:08 -0000
@@ -420,6 +432,16 @@ function locale_form_alter(&$form, &$for
 /**
+ * Node form submit handler.
+ */
+function locale_field_node_form_submit($form, &$form_state) {

"Form submit handler for node_form()."

Should also have a PHPDoc description that explains what's happening here.

+++ modules/locale/locale.module	14 Oct 2009 12:51:08 -0000
@@ -440,6 +462,31 @@ function locale_theme() {
+function locale_field_attach_view_alter(&$output, $context) {
+  static $recursion;
+
+  // Do not apply fallback rules if multilingual support is not enabled.
+  if (!$recursion && variable_get('locale_field_fallback_view', TRUE) && field_multilingual_check_translation_handlers($context['obj_type'], 'locale')) {
+    $recursion = TRUE;
+    module_load_include('inc', 'locale', 'locale.field');
+    locale_field_fallback_view($output, $context);
+    $recursion = FALSE;
+  }
+}

Is this concept of $recursion already explained elsewhere or a common pattern in field API as of now? If not, then we should squeeze some inline comments in here.

+++ modules/locale/locale.field.inc	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,76 @@
+/**
+ * Node form submit handler.
+ *
...
+function locale_field_node_form_update_field_language($form, &$form_state, $reset_previous = TRUE) {

Like above, Form submit handler for node_form().

+++ modules/locale/locale.field.inc	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,76 @@
+/**
+ * Apply fallback rules to the given object. Parameters are the same of
+ * hook_field_attach_view().
+ */
+function locale_field_fallback_view(&$output, $context) {

The second sentence should be moved into the PHPDoc description.

This review is powered by Dreditor.

plach’s picture

Thanks sun! Suggestions from #36 are implemented, except:

We should invoke a hook here instead and move the cache flushing into field module instead.
We can use a single/unique hook name, like hook_language_settings_alter(), for example.

I ain't sure about this: conceptually it's right, but I ain't sure we usually use hooks in these situations.

I'm not sure whether we shouldn't keep the old key name... "translation" on its own is not kinda descriptive..
We can go with "translation" though, in case we don't have a better idea now.

I introduced this change while working at #539110: TF #4: Translatable fields UI: the main reason is I stored a complex data structure in it, which holds generic data used for the entity translation.

sun’s picture

Will do a more in-depth re-review later, and just want to assure that we need to do the hook to provide other modules a single entry point for flushing any language-relevant caches. So Field module is not really the measure here, but we still want to move the clearing of caches into a hook implementation of Field module, just like do in http://api.drupal.org/api/function/field_modules_installed/7 already. Following this pattern ensures a sane API, other modules can plug into.

catch’s picture

I wasn't subscribed to this :(

While the hook sounds like a good idea, we currently don't have that pattern in core, so I don't think it's essential for a commit here.

plach’s picture

let webchick choose :)

sun’s picture

Status: Needs review » Reviewed & tested by the community

This looks good from a code perspective now. I've to admit that I didn't test it (live), but if the code looks right, then there is little chance that things go wrong.

alexanderpas’s picture

+++ includes/bootstrap.inc	15 Oct 2009 02:04:30 -0000
@@ -1657,6 +1657,17 @@ function drupal_language_types() {
+  static $multilingual;

drual_static()?

+++ includes/bootstrap.inc	15 Oct 2009 02:04:30 -0000
@@ -1657,6 +1657,17 @@ function drupal_language_types() {
+    $multilingual = variable_get('language_count', 1) > 1;

we might make it more clear, we're getting a boolean from here.

I'm on crack. Are you, too?

plach’s picture

rerolled

@alexanderpas: thanks for reviewing!

drual_static()?

I didn't use it because we'll never need to reset the variable value during a single page request. Moreover it would invalidate the (small) benefit of avoiding a function call.

we might make it more clear, we're getting a boolean from here.

I don't get what you're suggesting: an inline comment?

webchick’s picture

Here's a code review of this patch. All of my questions were answered on IRC. This is needs work for docs; I can't really find anything else to complain about, although I confess I don't 100% follow everything that's going on here.

First question is why aren't node titles made translatable as well? Answer is because it's being handled over here: #571654: Revert node titles as fields

+++ includes/bootstrap.inc	15 Oct 2009 15:15:38 -0000
@@ -1672,7 +1683,7 @@ function language_list($field = 'languag
-    if (variable_get('language_count', 1) > 1 || module_exists('locale')) {
+    if (drupal_multilingual() || module_exists('locale')) {

Why would locale exist if we weren't a multilingual site?

A: If people are translating English into something slightly different.

+++ includes/language.inc	15 Oct 2009 15:15:38 -0000
@@ -396,3 +396,36 @@ function language_url_split_prefix($path
+    // Let other modules hook in and add/change candidates.
+    drupal_alter('language_get_fallback_candidates', $fallback_candidates);

Need to document hook_language_get_fallback_candidates_alter(). Especially since there doesn't seem to be a hook_language_get_fallback_candidates(). Would this be more appropriately called hook_language_list()?

A: No, because we're altering an ordered language list which also contains FIELD_LANGUAGE_NONE.

+++ modules/locale/locale.api.php	15 Oct 2009 15:15:38 -0000
@@ -137,5 +137,12 @@ function hook_language_negotiation_info_
 /**
+ * Allow modules to react to language settings changes.
+ */
+function hook_multilingual_settings_changed() {
+  cache_clear_all('field_info_types', 'cache_field');
+}
+

These docs need vast improvement. There needs to be answers to these questions:

1. How do I know when I should implement this hook?
2. How do I know when I should /invoke/ this hook? (since there are at least 3 places in this one patch that this happens)
3. What sort of stuff should I be doing in here?

+++ modules/locale/locale.module	15 Oct 2009 15:15:38 -0000
@@ -440,6 +464,34 @@ function locale_theme() {
+function locale_field_attach_view_alter(&$output, $context) {
+  // In locale_field_fallback_view() we might call field_attach_view(). The
+  // static variable avoids unnecessary recursion. 
+  static $recursion;

Q: Is there a chance of this function being called multiple times on the same page? Should we add a $reset param/drupal_static?

A: No, because it's reset down at the bottom of the function.

+++ modules/locale/locale.test	15 Oct 2009 15:18:58 -0000
@@ -1619,6 +1621,73 @@ class UILanguageNegotiationTest extends 
+class LocaleMultilingualFieldsFunctionalTest extends DrupalWebTestCase {

Please add a line of PHPDoc here.

+++ modules/locale/locale.test	15 Oct 2009 15:18:58 -0000
@@ -1619,6 +1621,73 @@ class UILanguageNegotiationTest extends 
+  function testMultilingualNodeForm() {

Please add a line of PHPDoc here.

This review is powered by Dreditor.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
webchick’s picture

Issue tags: +Needs documentation

Spoke with sun on IRC. He said that despite him not being able to give this a more thorough review:

<tha_sun> webchick: so far, plach took always all considerations of all i18n team members into account -- I fully trust him to take proper decisions, and, well, I'm sure we'll have to rule out some bugs later... so, yeah, +1

And, well, that's good enough for me. ;)

Since the only thing left-over is docs, and docs are *not* bound by code freeze, *and* it's like bazillion o'clock plach's time, I went ahead and committed this to HEAD. plach promised me there would be a follow-up here tomorrow with the missing documentation. :)

yched’s picture

This is largely over my head now, and I'm not where I can spend quality time with a patch anyway, but congratulations, folks !

Minor : could we use field_info_cache_clear() instead of the various direct cache_clear_all('field_info_types', 'cache_field'); calls ?

plach’s picture

Status: Needs work » Needs review
FileSize
3.84 KB

@webchick: here are the PHP docs I owed you ;)

The patch also makes two one-line changes: it implements yched's suggestion from #48 (thanks yched! have fun :) and changes hook_language_fallback_get_candidates_alter into hook_language_fallback_candidates_alter.

webchick’s picture

Status: Needs review » Fixed

Excellent. Thanks, plach! :)

Committed follow-up to HEAD.

Status: Fixed » Closed (fixed)
Issue tags: -i18n, -i18n sprint, -Needs documentation, -translatable fields

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