Through out the core we are seeing many situations (e.g. #571654: Revert node titles as fields) in which one might need to select the language of a field as it would be displayed. At the moment this is possible only by going through a field_attach_view() call and picking the field built values, which has evident performance drawbacks and might also be just wrong.

We need a way to select the raw items in a $object->$field_name data structure using the right language. This might be also needed by #612894: field_format() is just a pile of code that doesn't work to provide coherent results.

CommentFileSizeAuthor
#47 field_fallback-657972-47.patch48.12 KBplach
#45 field_fallback-657972-45.patch48.57 KBplach
#43 field_fallback-657972-43.patch48.29 KBplach
#41 field_fallback-657972-41.patch48.44 KBplach
#39 field_fallback-657972-39.patch41.8 KBplach
#34 field_fallback-657972-34.patch41.19 KBplach
#33 field_fallback-657972-33.patch37.86 KBplach
#31 field_fallback-657972-30.patch39.72 KBBerdir
#29 field_fallback-657972-28.patch39.72 KBBerdir
#25 field_fallback-657972-25.patch40.94 KBplach
#24 field_fallback-657972-24.patch37.61 KBplach
#21 field_fallback-657972-19.patch40.91 KBplach
#18 field_fallback-657972-18.patch40.81 KBplach
#17 field_fallback-657972-17.patch40.81 KBplach
#12 field_fallback-657972-12.patch40.3 KBplach
#9 field_fallback-657972-9.patch40.34 KBplach
#4 field_fallback-657972-4.patch30.34 KBplach
#1 field_fallback-657972-1.patch27.58 KBplach
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Active » Needs review
FileSize
field_fallback-657972-1.patch27.58 KB

The attached patch introduces a new field_multilingual_fallback() function which returns an array of language codes keyed by field name. It can work in single-field mode in which case it returns a single language code.

Tests and documentation have been updated accordingly (an undocumented hook has also been added to fied.api.php).

yched’s picture

yched CreditAttribution: yched commented

We need this. Subscribe for now.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented 
+++ modules/field/field.multilingual.inc	12 Dec 2009 11:10:11 -0000
@@ -33,39 +33,48 @@ function field_multilingual_settings_cha
+function field_multilingual_available_languages($obj_type, $field, $language_suggestion = NULL) {
+  $available_languages = _field_multilingual_available_languages($obj_type, $field);
+
+  if (!empty($language_suggestion)) {
...
+    if (field_multilingual_enabled($obj_type, $field)) {
+      $languages = field_multilingual_content_languages();
+      // Let other modules alter the available languages.
+      $context = array('obj_type' => $obj_type, 'field' => $field);
+      drupal_alter('field_languages', $languages, $context);
+      $field_languages[$field_name] = $languages;
     }
     else {
...
+      $field_languages[$field_name] = array(LANGUAGE_NONE);
     }

@@ -140,3 +174,37 @@ function field_multilingual_valid_langua
+function field_multilingual_fallback($obj_type, $object, $langcode, $field_name = NULL) {
+  $fallback = array();
...
+  if (field_multilingual_enabled_handler($obj_type)) {
...
+  }
+
+  $single_field = isset($field_name) && is_array($fallback) && isset($fallback[$field_name]);
+  return $single_field ? $fallback[$field_name] : $fallback;
+}

Most of the code in this patch seems to makes sense. However, I'd really like to see more inline comments that explain (especially for people who do not know this subsystem inside out) what the code is doing and why. The pasted example snippets are just examples.

I'm on crack. Are you, too?

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
field_fallback-657972-4.patch30.34 KB

Improved and augmented inline comments and PHP docs.

yched’s picture

yched CreditAttribution: yched commented
Status: Needs review » Needs work

Spent some time with the patch, here's what I came with so far.

+++ modules/field/field.attach.inc	13 Dec 2009 19:08:59 -0000
@@ -197,10 +197,12 @@ function _field_invoke($op, $obj_type, $
+      // Unless a language suggestion is provided we iterate on all the
+      // available languages.
+      foreach ($available_languages as $langcode) {
         $field_translations[$langcode] = isset($object->{$field_name}[$langcode]) ? $object->{$field_name}[$langcode] : array();
       }

Comment is misleading. In the code, I see no logic for the 'unless'. The logic is probably embedded in f_m_available_languages(), but in this case it should not be reflected in a comment inside _field_invoke() (or the comment should make it clear that the logic is done in the helper func)
Also applies to _field_invoke_multiple()

+++ modules/field/field.attach.inc	13 Dec 2009 19:08:59 -0000
@@ -1163,7 +1167,13 @@ function field_attach_prepare_view($obj_
+  // The given object might not have translations for the requested language. To
+  // ensure a value is displayed for each field we need to apply language
+  // fallback rules.
+  $fallback = field_multilingual_fallback($obj_type, $object, $langcode);
+  $options = array('language' => empty($fallback) ? $langcode : $fallback);

Hm, if I get this patch correctly, f_m_fallback() without a specific field name will return an array of language fallbacks for each field separately, so this doesn't look correct.
Besides, it's a bit strange that we call f_m_fallback() here, since the lang fallback logic will be taken care of in the _field_invoke() calls. AFAICT, this is only needed to populate the $context['language'] element provided to hook_field_attach_view_alter()at the end of the function. But does this info make any sense now ?
Correct me if I'm wrong but it seems that, if anything, we should provide the *originally* requested language. each $output[$field_name] array has a '#language' property that contains the actual language selected for the field.

+++ modules/field/field.multilingual.inc	13 Dec 2009 19:01:31 -0000
@@ -30,42 +29,56 @@ function field_multilingual_settings_cha
  * @param $suggested_languages
  *   An array of language preferences which will be intersected with the enabled
  *   languages.
+ *
  * @return
  *   An array of valid language codes.
  */
-function field_multilingual_available_languages($obj_type, $field, $suggested_languages = NULL) {
+function field_multilingual_available_languages($obj_type, $field, $language_suggestion = NULL) {

PHPdoc still has the old name of the param.

+++ modules/field/field.multilingual.inc	13 Dec 2009 19:01:31 -0000
@@ -30,42 +29,56 @@ function field_multilingual_settings_cha
+    // We might have an array of language suggestions keyed by field name.

The PHPdoc should mention that possibility

+++ modules/field/field.multilingual.inc	13 Dec 2009 19:01:31 -0000
@@ -77,7 +90,26 @@ function field_multilingual_available_la
+/**
+ * Check if the given field has language support enabled.

"Checks whether a field has language support" ?

+++ modules/field/field.multilingual.inc	13 Dec 2009 19:01:31 -0000
@@ -140,3 +180,45 @@ function field_multilingual_valid_langua
+/**
+ * Apply fallback rules to the fields attached to the given object.
+ *

According to the new doc standards, PHPdoc descriptions should be active: "Applies fallback rules...". Please fix the PHPdocs added or updated in the patch.

+++ modules/field/field.multilingual.inc	13 Dec 2009 19:01:31 -0000
@@ -140,3 +180,45 @@ function field_multilingual_valid_langua
+ *   If a field name is specified a language code, otherwise an array of

Incorrect english.

+++ modules/field/field.multilingual.inc	13 Dec 2009 19:01:31 -0000
@@ -140,3 +180,45 @@ function field_multilingual_valid_langua
+function field_multilingual_fallback($obj_type, $object, $langcode, $field_name = NULL) {

The name is cryptic.
This is *the* outside-facing API function, right ? Other functions in field.multilingual.inc are mainly used by the rest of Field API / locale ?
If so, I'd suggest field_get_language($obj_type, $object, $langcode, $field_name)

More generally, all the function names in field.multilingual.inc are cryptic. I'm really not a fan of the field_multilingual_* namespace, really convoluted. The fact that functions are grouped in a 'multilingual.inc' file is one thing, this doesn't mean we need to use that as a namespace.
Similarly, field_multilingual_available_languages() could be field_available_languages(), for instance.

Could be fixed by a cleaner separation of which functions are internal helper functions and which functions are actual API functions ?

+++ modules/locale/locale.field.inc	13 Dec 2009 19:05:00 -0000
@@ -36,42 +34,51 @@ function locale_field_node_form_update_f
+function locale_field_fallback($obj_type, $object, $langcode, $field_name = NULL) {

It's not clear that the function is being called by locale's hook_field_multilingual_fallback_alter().
Is it really worth as a separate function ? or could it be merged into locale_field_multilingual_fallback_alter() ?
At least it should be made clearer in the PHPdoc.
Same pattern of 'it's unclear what is an API function and what is a helper function, BTW.

+++ modules/field/tests/field.test	13 Dec 2009 18:37:36 -0000
@@ -2595,6 +2603,79 @@ class FieldTranslationsTestCase extends 
+  function testFieldLanguageFallback() {
+    $field_name = drupal_strtolower($this->randomName() . '_field_name');
+    $entity_type = 'test_entity';
+
+    $field = array(

Please add a comment that we're adding a new field in addition to the one in setUp(). The tests could use a couple more comments to provide an outline of what is being done. Easier debugging when some patch triggers a fail in there - and multilinguial code and logic is hairy enough as is ;-)

+++ modules/field/tests/field.test	13 Dec 2009 18:37:36 -0000
@@ -2595,6 +2603,79 @@ class FieldTranslationsTestCase extends 
+      'label' => $this->randomName() . '_label',
+      'description' => $this->randomName() . '_description',
+      'weight' => mt_rand(0, 127),
+      'settings' => array(
+        'test_instance_setting' => $this->randomName(),
+      ),
+      'widget' => array(
+        'type' => 'test_field_widget',
+        'label' => 'Test Field',
+        'settings' => array(
+          'test_widget_setting' => $this->randomName(),
+        ),

Do we really need those ? would make the scope of the test clearer if properties that are not strictly required are omitted.
Same goes for the current FieldTranslationsTestCase::setUp(), BTW.

+++ modules/field/tests/field.test	13 Dec 2009 18:37:36 -0000
@@ -2595,6 +2603,79 @@ class FieldTranslationsTestCase extends 
+      $this->assertTrue(isset($entity->{$field_name}[$langcode]) && $langcode != $requested_language, t('The fallback language for the field %field_name is %language.', array('%field_name' => $field_name, '%language' => $langcode)));

This doesn't really test that the fallback language is the one expected, right ?

+++ modules/field/tests/field_test.field.inc	13 Dec 2009 18:37:36 -0000
@@ -324,3 +324,11 @@ function field_test_field_access($op, $f
+/**
+ * Implements hook_field_multilingual_fallback_alter().
+ */
+function field_test_field_multilingual_fallback_alter(&$fallback, &$context) {

Why is this hook implemented in field_test.field.inc while hook_field_langages_alter() is in field_test.module ?

+++ modules/field/tests/field_test.module	13 Dec 2009 18:37:36 -0000
@@ -87,14 +87,16 @@ function field_test_field_test_op_multip
-function field_test_field_languages($obj_type, $field, &$languages) {
-  if ($field['settings']['test_hook_in']) {
+function field_test_field_languages_alter(&$languages, $context) {
+  if ($context['field']['settings']['test_hook_in']) {

Already present in the code, but 'test_hook_in' is really ugly ;-).
Also, comments could be useful around the uses of this 'setting' in the .test file.

+++ modules/field/tests/field_test.module	13 Dec 2009 18:37:36 -0000
@@ -87,14 +87,16 @@ function field_test_field_test_op_multip
+    $languages = array_flip($languages);
+    unset($languages['en']);
+    $languages = array_flip($languages);

an array_search()/unset() would be cleaner

I'm on crack. Are you, too?

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

I'll implement the suggestions from #5 ASAP, meanwhile:

Hm, if I get this patch correctly, f_m_fallback() without a specific field name will return an array of language fallbacks for each field separately, so this doesn't look correct.

Probably I didn't make clear the approach I followed. To make language fallback actually reusable outside f_a_view() we need a mapping of the field items in $object to be used: as theorically every field might hold a different translation set (this is already true for translatable vs untranslatable fields), we need a per-field language suggestion. I implemented it as an array of language codes keyed by field name.
In _field_invoke() the whole array is needed to select to correct items that will be used to build the renderable array.
Perhaps $options['language'] is misleading, we might want to rename it to $options['language_suggestion'], but it can hold either a single language code or an array.

Besides, it's a bit strange that we call f_m_fallback() here, since the lang fallback logic will be taken care of in the _field_invoke() calls.

No, language fallback is a concept strictly tied to visualization: it makes no sense to apply it in contexts like 'insert' or 'update'. This is why I kept it outside _field_invoke().

AFAICT, this is only needed to populate the $context['language'] element provided to hook_field_attach_view_alter()at the end of the function. But does this info make any sense now ?
Correct me if I'm wrong but it seems that, if anything, we should provide the *originally* requested language. each $output[$field_name] array has a '#language' property that contains the actual language selected for the field.

The main advantage of this solution is it doesn't mess with renderable arrays anymore: f_a_view() just tells _field_invoke() which items to use to build the renderable array. No language alteration should be performed in hook_f_a_view_alter() implementations, I just forgot to remove $context['language'] :)

The name is cryptic.
This is *the* outside-facing API function, right ? Other functions in field.multilingual.inc are mainly used by the rest of Field API / locale ?
If so, I'd suggest field_get_language($obj_type, $object, $langcode, $field_name)

Well, perhaps we need some discussion about this. I ain't sure it f_m_fallback() should be called directly outside the field API, IMO it would be better to encapsulate it into a function, say field_get_items(), actually selecting the raw items.

More generally, all the function names in field.multilingual.inc are cryptic. I'm really not a fan of the field_multilingual_* namespace, really convoluted. The fact that functions are grouped in a 'multilingual.inc' file is one thing, this doesn't mean we need to use that as a namespace.
Similarly, field_multilingual_available_languages() could be field_available_languages(), for instance.
Could be fixed by a cleaner separation of which functions are internal helper functions and which functions are actual API functions ?

No strong objections on this, but we need to understand if we can still do it and figure out how to, before actually coding.

It's not clear that the function is being called by locale's hook_field_multilingual_fallback_alter().
Is it really worth as a separate function ? or could it be merged into locale_field_multilingual_fallback_alter() ?
At least it should be made clearer in the PHPdoc.

IMO making any logic reusable, no matter if there is an actual need, can't hurt. In this case we have a function implementing the core field language fallback rules, which is called also in field_test_field_fallback_alter(). Moreover storing it in a separate file allows to save memory. Also, if we merged it in locale_field_multilingual_fallback_alter(), we would be tied to the translation handler check and the value of the 'locale_field_fallback_view' variable.
I don't think PHPdocs should report who calls the documented function, in most cases it would be an incomplete information.

yched’s picture

yched CreditAttribution: yched commented

- f_a_view() / f_m_fallback(): OK, got confused. I get it now, but I really needed to give a close look at the code in field_invoke() / f_m_available_languages() / f_m_fallback(). I'm sorry to say that the multilingual workflow around field_invoke() and $options['language'] really needs to be documented better. If even I (sorry for the arrogance :-p) get confused about how TF work, then I strongly suspect this will only appear as inextricable black magic to 99% people trying to wrap their heads around TF.
Opened #664136: Enhance TF code documentation for this.

- The PHPdoc for f_m_fallback() needs to be more specific. "determines the actual language to be used for each field given a) the requested language b) the actual data available in the fields c) the current language fallback configuration." ?
+ Even if we cannot hardcode in the PHPdocs the exact places calling the function, saying that *for instance* f_a_view() uses this function to determine the language to display for each field of an object would make it less abstract what the function does.

-

I ain't sure it f_m_fallback() should be called directly outside the field API, IMO it would be better to encapsulate it into a function, say field_get_items(), actually selecting the raw items.

IMO both field_get_language() (== f_m_fallback()) and field_get_items() make sense as API functions. Removing 'multilingual' in a couple existing functions can be done in a separate patch, but let's make sure the functions introduced here have clear, usable names.

- locale_field_fallback() / locale_field_multilingual_fallback_alter():
Memory savings from putting code in .inc files has been deemed not critical in D7. The logical code structure and readability should prevail - esp. for this small amount of code.
If locale_field_fallback() is useful as a reusable API function, then its PHPdoc should be clearer on what it does. It should also not be hidden inside a file that needs to be manually loaded. The PHPdoc for locale_field_fallback() really needs to at least say that this code is active through locale's hook_field_multilingual_fallback_alter(), or we cannot relate it to the rest of the workflow.

- hook_field_multilingual_fallback_alter() is actually not an alter hook, we don't build anything that modules can then alter. The data *is* what is returned by the hook implemantations. Having the hook named _alter makes you think there's some basic, canonical logic coded somewhere, and thus gives a misleading view on the actual mechanism.

+++ modules/field/field.attach.inc	13 Dec 2009 19:08:59 -0000
@@ -1163,7 +1167,13 @@ function field_attach_prepare_view($obj_
+  $langcode = field_multilingual_valid_language($langcode, FALSE);

Can't this check be made within the f_m_fallback() call ?

+++ modules/field/field.attach.inc	13 Dec 2009 19:08:59 -0000
@@ -1163,7 +1167,13 @@ function field_attach_prepare_view($obj_
+  // The given object might not have translations for the requested language. To
+  // ensure a value is displayed for each field we need to apply language
+  // fallback rules.

Not too accurate. I'd suggest
"Apply language fallback rules to determine the actual language to display for each field given the available languages in the field data."
It should also be clearer that $fallback is an array. Maybe $fallbacks ?

+++ modules/field/field.attach.inc	13 Dec 2009 19:08:59 -0000
@@ -1163,7 +1167,13 @@ function field_attach_prepare_view($obj_
+  $options = array('language' => empty($fallback) ? $langcode : $fallback);

Needs a comment. When does this happen and why is this logic the right one ?
Similarly, can't this be included within f_m_fallback() ? (Maybe not, just asking).

This review is powered by Dreditor.

yched’s picture

yched CreditAttribution: yched commented

the revamped field_view_field() function in #612894: field_format() is just a pile of code that doesn't work got in, so we should now also make use of the new fallback mechanism over there.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs work » Needs review
FileSize
field_fallback-657972-9.patch40.34 KB

Ok, here is an attempt to improve readability, documentation and implement the suggestions from #5, #7, #8.

Brief summary:

  • field_multilingual_fallback() is now field_display_language()
  • fallback rules are less mentioned in comments in favor of the (hopefully) more human terminology "display languages"
  • field_display_language() actually initialize its returned values and then let hook implementations alter them
  • locale.field.inc has gone, the code has been moved in locale.module
  • f_m_available_languages() does not handle suggestions anymore, I introduced a helper function named _field_language_suggestion() to be able to reuse the code in _field_invoke() and _field_invoke_multiple(). I don't really like this solution, though.
  • $options['language'] is now documented
  • field_view_field() has been updated
  • field_get_items() has been introduced
Dries’s picture

Dries CreditAttribution: Dries commented

I started looking at this patch but got interrupted. I'm not entirely sure why this patch is 'critical' -- it isn't a regression is it? In fact, it is still unclear what exactly this patch does.

For example, I spend the first 5 minutes trying to figure out the difference between 'available languages for a field' and 'display languages for a field'. It is not clear why we need both sets.

Anyway, the patch isn't obvious, so I'll need to invest more time in trying to understand it. Have to run now -- maybe we can make it more intuitive or simpler in the mean time.

+++ modules/field/field.multilingual.inc	22 Dec 2009 10:26:19 -0000
@@ -133,10 +144,64 @@ function field_multilingual_valid_langua
+ * The actual language for each given field are determined based on the

The verb is not correct and does not match the noun.

+++ modules/field/field.attach.inc	22 Dec 2009 10:13:28 -0000
@@ -398,6 +412,38 @@ function _field_invoke_multiple_default(
+ * Returns the suggested language if it appears among the available ones.
+ *
+ * @param $available_languages
+ *   An array of valid language codes.
+ * @param $language_suggestion
+ *   A language code or an array of language codes keyed by field name.
+ * @param $field_name
+ *   The name of the field being processed.
+ *
+ * @return
+ *   An array of valid language codes.

The code doesn't explain how the suggested language is selected from the available languages.

This review is powered by Dreditor.

yched’s picture

yched CreditAttribution: yched commented

@Dries: the context of this patch is "I'm asked to display a node in spanish. field_a has values in spanish, but field_b only has values in english and french. What should I display for field_b ?".

This is determined by language fallback logic. Currently, the Field / TF API does this in two steps:
- let field_attach_view() build render arrays for values in spanish if they exist.
- in hook_field_attach_view_alter(), locale computes the fallback language according to its logic and current settings, and then computes and switches in the render array for the fallback language.

issues:
- convoluted flow
- more importantly, the language fallback logic is blackboxed as magic stuff during field_attach_view() workflow. Short of actually building the render array, there's currently no way to answer the simple question 'Given the requested language and current site settings, what language should be displayed for this field'. This ability is critical. Raised lots of foobar in #571654: Revert node titles as fields where there were attempts to build / render a fully themed 'field' just to access $node->title[$the_right_language].

This patch externalizes the language fallback logic to a public API function (field_display_language() in the last iteration), that field_attach_view() uses on the various fields.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
field_fallback-657972-12.patch40.3 KB

Implemented suggestions from #10 and moved _field_language_suggestion() in field.multilingual.inc.

@Dries:

For example, I spend the first 5 minutes trying to figure out the difference between 'available languages for a field' and 'display languages for a field'. It is not clear why we need both sets.

IMO to make the behavior implemented by this patch understandable, we just need to make the above distinction extra-clear (I'll provide a thorough documentation in #664136: Enhance TF code documentation): all the very core of translatable fields is related to it.

Almost all field_attach_X() functions act on every available language. For each field available languages is the set of languages that can be assigned to the field data.

  • For untranslatable fields this set has cardinality 1 and contains only LANGUAGE_NONE.
  • For translatable fields the set can contain any language code; by default it is the set of enabled languages with the addition of LANGUAGE_NONE. This default can be altered by contrib modules.

However there are two relevant exceptions which instead act in single-language mode:

  • field_attach_form() just needs a single language code to specify in which language the field values will be submitted.
  • field_attach_view() needs a desired/requested language which is the language one wish to display the object in; however we can't be sure that a translation in the requested language is available for each field. We have two ways to deal with this: 1) we ignore missing translations, 2) we provide an alternative value in a different language (i.e. language fallback). The core behavior implemented here is inspecting the field data available in the object to be displayed and determining for each field a language for which a translation actually exists; this way we can display a value for each field.

For each field the display language is the most fitting language code, according to the current language fallback configuration, for which field data is available. It can be equal to the desired/requested language, but it will differ every time field data for the requested language is not available.
Core language fallback rules inspect all the enabled languages ordered by their weight, but this behavior can be altered or disabled, thus letting one choose the approach #1.

Status: Needs review » Needs work
Issue tags: -translatable fields, -API clean-up, -D7 API clean-up

The last submitted patch, , failed testing.

Status: Needs work » Needs review

Re-test of from comment #2402236 was requested by @user.

Status: Needs review » Needs work
Issue tags: +translatable fields, +API clean-up, +D7 API clean-up

The last submitted patch, , failed testing.

yched’s picture

yched CreditAttribution: yched commented
Status: Needs work » Needs review

bot #fail

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
field_fallback-657972-17.patch40.81 KB

The patch is trying to remove locale.field.inc. Probably this is messing up the bot. This one simply empties it. We'll need a manual deletion, perhaps.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
field_fallback-657972-18.patch40.81 KB

Rerolled

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
field_fallback-657972-19.patch40.91 KB

Rerolled

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

#21: field_fallback-657972-19.patch queued for re-testing.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Issue tags: -Performance

Here is a rerolled patch.

I also performed some benchmarks to quantify the performance gain the new approach is supposed to introduce. While performing the benchmark I discovered a critical bug in the HEAD which prevents the language fallback to be performed.

The tests were performed with a patched version of the HEAD to fix the bug above (see #710526: Improve multilingual field test coverage for details); the patch should have absolutely no performance impact, so the results should be quite realistic.

HEAD (10 nodes on front page, 10 fields for each node, locale not installed, ab -c1 -n100):

Document Path:          /
Document Length:        40244 bytes

Concurrency Level:      1
Time taken for tests:   27.035 seconds
Complete requests:      100
Failed requests:        0
Write errors:           0
Total transferred:      4070300 bytes
HTML transferred:       4024400 bytes
Requests per second:    3.70 [#/sec] (mean)
Time per request:       270.348 [ms] (mean)
Time per request:       270.348 [ms] (mean, across all concurrent requests)
Transfer rate:          147.03 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   250  270   9.7    265     312
Waiting:      250  257   9.5    250     296
Total:        250  270   9.7    265     312

Percentage of the requests served within a certain time (ms)
  50%    265
  66%    265
  75%    281
  80%    281
  90%    281
  95%    281
  98%    312
  99%    312
 100%    312 (longest request)

Patch (10 nodes on front page, 10 fields for each node, locale not installed, ab -c1 -n100):

Document Path:          /
Document Length:        40244 bytes

Concurrency Level:      1
Time taken for tests:   27.144 seconds
Complete requests:      100
Failed requests:        0
Write errors:           0
Total transferred:      4070300 bytes
HTML transferred:       4024400 bytes
Requests per second:    3.68 [#/sec] (mean)
Time per request:       271.440 [ms] (mean)
Time per request:       271.440 [ms] (mean, across all concurrent requests)
Transfer rate:          146.44 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   250  271   8.6    265     296
Waiting:      250  261   8.4    265     281
Total:        250  271   8.6    265     296

Percentage of the requests served within a certain time (ms)
  50%    265
  66%    281
  75%    281
  80%    281
  90%    281
  95%    281
  98%    281
  99%    296
 100%    296 (longest request)

HEAD (10 nodes on front page, 10 fields for each node, locale installed - 2 languages, ab -c1 -n100):

Document Path:          /
Document Length:        40604 bytes

Concurrency Level:      1
Time taken for tests:   30.077 seconds
Complete requests:      100
Failed requests:        0
Write errors:           0
Total transferred:      4106300 bytes
HTML transferred:       4060400 bytes
Requests per second:    3.32 [#/sec] (mean)
Time per request:       300.768 [ms] (mean)
Time per request:       300.768 [ms] (mean, across all concurrent requests)
Transfer rate:          133.33 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   281  301   8.0    296     312
Waiting:      281  287   8.0    281     312
Total:        281  301   8.0    296     312

Percentage of the requests served within a certain time (ms)
  50%    296
  66%    296
  75%    312
  80%    312
  90%    312
  95%    312
  98%    312
  99%    312
 100%    312 (longest request)


 
Document Path:          /fr
Document Length:        40720 bytes

Concurrency Level:      1
Time taken for tests:   30.623 seconds
Complete requests:      100
Failed requests:        0
Write errors:           0
Total transferred:      4117900 bytes
HTML transferred:       4072000 bytes
Requests per second:    3.27 [#/sec] (mean)
Time per request:       306.228 [ms] (mean)
Time per request:       306.228 [ms] (mean, across all concurrent requests)
Transfer rate:          131.32 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   1.6      0      16
Processing:   281  306   8.8    312     328
Waiting:      281  293   8.6    296     312
Total:        281  306   8.8    312     328

Percentage of the requests served within a certain time (ms)
  50%    312
  66%    312
  75%    312
  80%    312
  90%    312
  95%    312
  98%    328
  99%    328
 100%    328 (longest request)

Patch (10 nodes on front page, 10 fields for each node, locale installed - 2 languages, ab -c1 -n100):

Document Path:          /
Document Length:        40604 bytes

Concurrency Level:      1
Time taken for tests:   29.344 seconds
Complete requests:      100
Failed requests:        0
Write errors:           0
Total transferred:      4106300 bytes
HTML transferred:       4060400 bytes
Requests per second:    3.41 [#/sec] (mean)
Time per request:       293.436 [ms] (mean)
Time per request:       293.436 [ms] (mean, across all concurrent requests)
Transfer rate:          136.66 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   281  293  11.2    296     328
Waiting:      265  279  10.4    281     312
Total:        281  293  11.2    296     328

Percentage of the requests served within a certain time (ms)
  50%    296
  66%    296
  75%    296
  80%    296
  90%    312
  95%    312
  98%    328
  99%    328
 100%    328 (longest request)
 
 

Document Path:          /fr
Document Length:        40720 bytes

Concurrency Level:      1
Time taken for tests:   29.390 seconds
Complete requests:      100
Failed requests:        0
Write errors:           0
Total transferred:      4117900 bytes
HTML transferred:       4072000 bytes
Requests per second:    3.40 [#/sec] (mean)
Time per request:       293.904 [ms] (mean)
Time per request:       293.904 [ms] (mean, across all concurrent requests)
Transfer rate:          136.83 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   281  294   9.7    296     328
Waiting:      265  280   9.8    281     312
Total:        281  294   9.7    296     328

Percentage of the requests served within a certain time (ms)
  50%    296
  66%    296
  75%    296
  80%    296
  90%    296
  95%    312
  98%    328
  99%    328
 100%    328 (longest request)

The numbers above show an average performance gain of more than 3% with the patch applied, and the performance gain should be O(n), where n is the number of fields attached to a node. Not mentioning the big gain on the API side.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
field_fallback-657972-24.patch37.61 KB

The rerolled patch...

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
field_fallback-657972-25.patch40.94 KB

Oops, wrong patch.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Issue tags: +Performance
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Issue tags: -translatable fields, -API clean-up, -D7 API clean-up

#25: field_fallback-657972-25.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Performance, +translatable fields, +API clean-up, +D7 API clean-up

The last submitted patch, field_fallback-657972-25.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
field_fallback-657972-28.patch39.72 KB

Ok, this was ugly to re-roll. All those object/entity renames caused failures all over the place. Hope I didn't miss anything and the apidocs probably don't follow the new standards.

Anyway, why I've found this issue..

In current HEAD, when viewing drupal in specific language (german in my case), field.module did not load fields that were marked as translatable but had only a LANGUAGE_NONE entry (aka: body), because it only tried to load the currently active language.

Beneath many other things (I assume, I have no clue about what the patch actually does), it does fix this. The body does now show up correctly.

Of course, I only found this after debugging my "hidden body" issue for hours and creating an ugly workaround for it.

Status: Needs review » Needs work

The last submitted patch, field_fallback-657972-28.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
field_fallback-657972-30.patch39.72 KB

Renamed too much :)

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

The reroll looks good at a first look, I'll give it a more thorough review later.

The problem reported by Berdir is caused by #710526: Improve multilingual field test coverage.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
field_fallback-657972-33.patch37.86 KB

Patch did not apply anymore.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
field_fallback-657972-34.patch41.19 KB

incomplete patch...

Status: Needs review » Needs work
Issue tags: -Performance, -translatable fields, -API clean-up, -D7 API clean-up

The last submitted patch, field_fallback-657972-34.patch, failed testing.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs work » Needs review
Issue tags: +Performance, +translatable fields, +API clean-up, +D7 API clean-up

#34: field_fallback-657972-34.patch queued for re-testing.

Cannot reproduce any error on my box.

yched’s picture

yched CreditAttribution: yched commented

I have to admit this dropped off my radar, sorry. Will try to get back asap.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

FYI: catch asked in IRC to rerun benchmarks with option -n1000.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
field_fallback-657972-39.patch41.8 KB

Patch updated after #612894: field_format() is just a pile of code that doesn't work commit.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented 
+++ modules/field/field.api.php	11 Mar 2010 13:22:46 -0000
@@ -1062,12 +1062,44 @@ function hook_field_attach_delete_revisi
+ * @param $display_language
+ *   A reference to an array of language codes keyed by field name.
...
+ *   - obj_type: The type of the entity to be displayed.
...
+function hook_field_display_language_alter(&$fallback, $context) {

At least the parameter names don't match.

+++ modules/field/field.api.php	11 Mar 2010 13:22:46 -0000
@@ -1062,12 +1062,44 @@ function hook_field_attach_delete_revisi
+ *   - obj_type: The type of the entity to be displayed.
...
+ *   - object_type: The type of the entity the field is attached to.

Isn't this 'entity_type' now?

+++ modules/field/field.module	13 Mar 2010 09:29:02 -0000
@@ -624,6 +624,27 @@ function field_view_field($entity_type, 
+function field_get_items($entity_type, $entity, $field_name, $langcode = NULL) {
+  $langcode = field_display_language($entity_type, $entity, $field_name, $langcode);
+  return isset($entity->{$field_name}[$langcode]) ? $entity->{$field_name}[$langcode] : FALSE;

I didn't look up where this is invoked, but is it a wise idea to return FALSE? Why not an empty array?

+++ modules/field/field.multilingual.inc	13 Mar 2010 09:40:24 -0000
@@ -14,62 +14,81 @@ function field_multilingual_settings_cha
+    if (in_array($language_suggestion, $available_languages)) {
+     $available_languages = array($language_suggestion);
+   }
   }

Odd indentation here.

+++ modules/field/field.multilingual.inc	13 Mar 2010 09:40:24 -0000
@@ -77,11 +96,30 @@ function field_multilingual_available_la
+ * Checks whether a field has language support.
...
+function field_multilingual_enabled($entity_type, $field) {
+  return $field['translatable'] && field_multilingual_enabled_handler($entity_type);

Hm - could we rename this to field_is_multilingual() or field_is_translatable() ?

+++ modules/field/field.multilingual.inc	13 Mar 2010 09:40:24 -0000
@@ -77,11 +96,30 @@ function field_multilingual_available_la
+ * Checks if a module is registered as a translation handler for a given entity.

@@ -94,26 +132,34 @@ function field_multilingual_content_lang
+function field_multilingual_enabled_handler($entity_type, $handler = NULL) {

Ditto, perhaps field_has_multilingual_handler() or similar?

148 critical left. Go review some!

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
field_fallback-657972-41.patch48.44 KB

This one takes care of #40. It also suppresses almost entirely the field_multilingual_* namespace as yched asked in #5. Not sure this can still happen, as this theorically qualifies as an API change not justified by a critical reason, but actually I think (and hope) no one is using the Field translation API yet, so this shouldn't be that dangerous. However, if webchick doesn't give her ok it should be fairly easy to revert the function names changes.

@sun:

I didn't look up where this is invoked, but is it a wise idea to return FALSE? Why not an empty array?

field_get_items is an API function not used anywhere in core, but talking with yched we agreed that it might turn useful. I don't think returning an empty array is the right choice because an empty array already represents an empty field value; a non-available translation simply does not appear in the $entity->{$field_name} array, so I'd choose between FALSE and NULL.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

wow, just wow - this patch is lovely!

+++ modules/field/field.attach.inc	22 Mar 2010 23:11:38 -0000
@@ -492,7 +506,7 @@ function _field_invoke_multiple_default(
-  $options = array('language' => field_multilingual_valid_language($langcode));
+  $options = array('language' => field_valid_language($langcode));

oh YES. Pretty please! Those "multilingual" prefixes were totally wonky.

+++ modules/field/field.module	22 Mar 2010 22:23:30 -0000
@@ -496,10 +496,8 @@ function field_view_value($entity_type, 
-    $langcode = field_multilingual_valid_language($langcode, FALSE);
-
     // Determine the langcode that will be used by language fallback.
-    $langcode = current(field_multilingual_available_languages($entity_type, $field, array($langcode)));
+    $langcode = field_display_language($entity_type, $entity, $field_name, $langcode);

I can't help, but this looks much much easier and grokable now :)

+++ modules/field/field.module	22 Mar 2010 22:23:30 -0000
@@ -624,6 +624,27 @@ function field_view_field($entity_type, 
+ * @param $langcode
+ *   Optional. The language code $entity->{$field_name} has to be displayed in.
+ *   If no value is given the current language will be used.

Minor: "Optional." should be "(optional)". (also elsewhere in this patch)

The second sentence can be shortened to "Defaults to the current language."

+++ modules/field/field.multilingual.inc	22 Mar 2010 23:11:38 -0000
@@ -127,16 +174,75 @@ function field_multilingual_check_transl
+function field_display_language($entity_type, $entity, $field_name = NULL, $langcode = NULL) {

I wonder why this is called field_display_language() and not just simply field_language() ?

AFAICS, there is only one language "type" for fields, and the other functions are just for getting available, and validating languages.

+++ modules/field/tests/field_test.module	22 Mar 2010 22:45:38 -0000
@@ -87,27 +87,23 @@ function field_test_field_test_op_multip
+ * Implements hook_field_languages_alter().
...
+ * Implements hook_field_display_language_alter().

aha, so now I realize that aforementioned shortening would lead to two almost identical hooks (singular/plural). Can we cope with that?

148 critical left. Go review some!

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
field_fallback-657972-43.patch48.29 KB

Fixed #42: renamed field_display_language() to field_language() and hook_field_languages_alter() to hook_field_available_languages_alter().

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented 
+++ modules/field/field.module	23 Mar 2010 10:44:43 -0000
@@ -624,6 +624,27 @@ function field_view_field($entity_type, 
+ * @param $langcode
+ *   (Optional) The language code $entity->{$field_name} has to be displayed in.

+++ modules/field/field.multilingual.inc	23 Mar 2010 10:58:00 -0000
@@ -89,31 +126,41 @@ function field_multilingual_content_lang
  * @param $handler
- *   The name of the handler to be checked.
+ *   Optional. The name of the handler to be checked.

@@ -127,16 +174,75 @@ function field_multilingual_check_transl
+ * @param $field_name
+ *   Optional. If specified only the display language of the given field will be
...
+ * @param $langcode
+ *   Optional. The language code $entity has to be displayed in. If no value is

"(optional)", all lower-case. See http://drupal.org/node/1354 for example snippets and detailed info.

Also: Not completely visible in this paste, all of the descriptions should continue without "If...", e.g.:

"(optional) The name of foo. Defaults to bar."

+++ modules/field/field.multilingual.inc	23 Mar 2010 10:58:00 -0000
@@ -127,16 +174,75 @@ function field_multilingual_check_transl
+function field_language($entity_type, $entity, $field_name = NULL, $langcode = NULL) {
+  $display_languages = &drupal_static(__FUNCTION__, array());
...
+  if (!isset($display_languages[$entity_type][$id][$langcode])) {

Is the static caching correct?

$display_languages[$entity_type][$id][$langcode]...

- is keyed by $entity_type, no $bundle_name, so the language for a field that appears in multiple bundles of the same entity is always the same...?

- is not keyed by revision...?

Not sure whether the static caching is a good idea. To me, this looks like we'd better avoid it for now and only introduce it if really required for performance.

+++ modules/field/tests/field_test.module	23 Mar 2010 10:59:23 -0000
@@ -87,27 +87,23 @@ function field_test_field_test_op_multip
+ * Implements hook_field_available_languages_alter().
...
+ * Implements hook_field_language_alter().

yay! Nice solution :)

+++ modules/locale/locale.module	23 Mar 2010 11:21:33 -0000
@@ -438,20 +452,57 @@ function locale_theme() {
+    locale_field_fallback($display_language, $context['entity'], $context['language']);
...
+function locale_field_fallback(&$display_language, $entity, $langcode) {

Shouldn't this be named locale_field_language_fallback() or would that be hook name clash?

148 critical left. Go review some!

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
field_fallback-657972-45.patch48.57 KB

Is the static caching correct?
$display_languages[$entity_type][$id][$langcode]...
- is keyed by $entity_type, no $bundle_name, so the language for a field that appears in multiple bundles of the same entity is always the same...?
- is not keyed by revision...?

Well, there is also the entity id as key, so AFAICT there should be no possibility that two distinct entities belonging to the same entity type accidentally share the same cache. All entity revisions will share the same translations set (not the same translation values!) per #629252: field_attach_form() should make available all field translations on submit, so a language valid for one revision should be valid for all revisions.

Not sure whether the static caching is a good idea. To me, this looks like we'd better avoid it for now and only introduce it if really required for performance.

I don't know what performance gain actually it gives, but I can tell you it saves lots of hook invocations.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented 
+++ modules/field/field.multilingual.inc	23 Mar 2010 13:44:15 -0000
@@ -89,31 +126,41 @@ function field_multilingual_content_lang
+function field_has_translation_handler($entity_type, $handler = NULL) {
...
   if (isset($handler)) {
     return isset($entity_info['translation'][$handler]) && !empty($entity_info['translation'][$handler]);
   }

While being here, we can drop the superfluous isset(), the !empty() is sufficient. (!empty() == isset() && != FALSE)

+++ modules/field/field.multilingual.inc	23 Mar 2010 13:44:15 -0000
@@ -89,31 +126,41 @@ function field_multilingual_content_lang
   $entity_info = entity_get_info($entity_type);
...
+    $handler_info = &drupal_static(__FUNCTION__, array());
...
+    return $handler_info[$entity_type];

1) The additional static cache here really doesn't seem to be necessary. $entity_info is cached statically already and this is merely iterating over the array.

2) Would it make sense to return an array of non-empty registered handlers instead of TRUE? This would equally equal TRUE when being tested elsewhere. (Just an idea, feel free to ignore)

Powered by Dreditor.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
field_fallback-657972-47.patch48.12 KB

Fixed #46.

2) Would it make sense to return an array of non-empty registered handlers instead of TRUE? This would equally equal TRUE when being tested elsewhere. (Just an idea, feel free to ignore)

Not sure about this: IIRC while coding the TF4 patch I never needed such a list, I just needed to know if a specific translation hanlder was enabled.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community

Awesome, thanks plach!

With this patch, it is much clearer what the common functions are that one should use with regard to field languages.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

Thanks, sun :)

Core committers should be aware that this patch is supposed to delete modules/locale/locale.field.inc and move its content into locale.module: I ain't sure that simply applying the patch will do the trick, locale.field.inc might have to be deleted manually.

Moreover #710526: Improve multilingual field test coverage should be commited first.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Spent 30 minutes looking at this code, and decided to commit it to CVS HEAD. Looks like an important improvement.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Fixed » Needs work

@Dries:

Thanks for your patience with this issue!

Unfortunately locale.field.inc is still there although it's empty, would it be possible to delete it?

yched’s picture

yched CreditAttribution: yched commented

Great ! thanks plach and sun for driving this home, sorry for my lack of availability :-/.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Reviewed & tested by the community

locale.field.inc needs to be removed.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Removed locale.field.inc. Thanks.

Status: Fixed » Closed (fixed)

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

boby_ui’s picture

boby_ui CreditAttribution: boby_ui commented

Hi, I can not get it to work, the language is not falling back using entity translation.. and the patch is not working for me..

can someone confirm please, thank you