Follow-up to #922824: No way to specify the language to view entities in.

Somehow prepare view hooks did not get the $langcode parameter, hence while performing entity view every hook is able to tell which language use except prepare view ones.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

Status: Active » Needs review
FileSize
9.07 KB

Spoke with yched: he agrees we need $langcode also in field_attach_prepare_view().

plach’s picture

Issue tags: +translatable fields
yched’s picture

Status: Needs review » Needs work

Definite overlook from #922824: No way to specify the language to view entities in, agreed that this need to be fixed.

+++ b/includes/common.inc
@@ -7484,8 +7484,15 @@ function entity_get_controller($entity_type) {
-function entity_prepare_view($entity_type, $entities) {
+function entity_prepare_view($entity_type, $entities, $langcode = NULL) {
+  if (!isset($langcode)) {
+    $langcode = $GLOBALS['language_content']->language;
+  }
+

I'm not 100% sure the entity_prepare_view() func (and associated hook_entity_prepare_view() hook) need the $langcode param too. We might want to check with @catch.

+++ b/modules/field/field.attach.inc
@@ -1074,8 +1074,17 @@ function field_attach_delete_revision($entity_type, $entity) {
+  $null = NULL;

Minor : can you move that $null declaration just above the _field_invoke_multiple() call, like in field_attach_view() ? It feels off standing on its own like that.

+++ b/modules/node/node.module
@@ -1351,15 +1351,15 @@ function node_build_content($node, $view_mode = 'full', $langcode = NULL) {
-    $node = node_invoke($node, 'view', $view_mode);
+    $node = node_invoke($node, 'view', $view_mode, $langcode);

How come we are fixing this now ? Isn't that a separate bugfix ?

+++ b/modules/node/node.pages.inc
@@ -326,7 +326,7 @@ function node_form_build_preview($form, &$form_state) {
-function node_preview($node) {
+function node_preview($node, $langcode = NULL) {

Ouch. Adding a $langcode param to node_preview() seems like a sweeping API change, which sounds like it needs some discussion of its own.

Besides, node_preview() seems odd : the function explicitly calls f_a_prepare_view(), but defers the corresponding f_a_view() call to :
theme('node_preview') -> node_view() (twice, 'full' and 'teaser') -> node_build_content().
node_build_content() does call both f_a_prepare_view() (*again*) and f_a_view(). So it looks like the first f_a_prepare_view() call in node_preview() is not needed.

And, btw, theme('node_preview') has no way to transfer a $langcode, which means that the current f_a_view() call provides no $langcode either (NULL), so it seems pointless to add a $langcode to node_preview() to begin with ?

Powered by Dreditor.

plach’s picture

Issue tags: -API addition +API change
FileSize
9.9 KB

Partially addressed #3:

I'm not 100% sure the entity_prepare_view() func (and associated hook_entity_prepare_view() hook) need the $langcode param too. We might want to check with @catch.

Well, I'm pretty sure we want it there either: I discovered this bug while working on Title because I was missing the $langcode parameter. However I dropped a line to @catch, hopefully he will chime in.

How come we are fixing this now ? Isn't that a separate bugfix ?

Also this one is an overlook from the previous issue and since hook_node_view() is taking a $langcode parameter I'd say the fix is ok. Just tell me if you actually prefer a separate issue, but IMO everything is here should already have been committed in the previous issue.

[...] And, btw, theme('node_preview') has no way to transfer a $langcode, which means that the current f_a_view() call provides no $langcode either (NULL), so it seems pointless to add a $langcode to node_preview() to begin with?

Makes sense, reverted.

However, test results are not coming back (again) but the previous patch failed them badly. The cuplrit is the f_a_prepare_view hunk, which did not take into account the fact that multiple entities are passed in. This forced me to introduce an API change in _field_invoke_multiple, which probably we would need anyway since the previous behavior was flawed:

         // Unless a language suggestion is provided we iterate on all the
         // available languages.
         $available_languages = field_available_languages($entity_type, $field);
-        $languages = _field_language_suggestion($available_languages, $options['language'], $field_name);
+        $language = !empty($options['language'][$id]) ? $options['language'][$id] : $options['language'];
+        $languages = _field_language_suggestion($available_languages, $language, $field_name);
         foreach ($languages as $langcode) {
           $grouped_items[$field_id][$langcode][$id] = isset($entity->{$field_name}[$langcode]) ? $entity->{$field_name}[$langcode] : array();
         }

We need to have a field language suggestion for each entity, having a shared one simply makes no sense as field translations available in one entity might not be available in another one. This apparently looks scary but it is a backward compatible API change since language codes cannot be numeric: if _field_invoke_multiple is being called in some custom code using a single array of field language suggestions (unlikely) this will simply keep working as the $language variable will be assigned $options['language']. Moreover the only other place in core using this is f_a_load() which does not use language suggestions as it needs to act on all the available languages.

plach’s picture

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

Note that this should not be a performance regression as field_language is static cached and is called for every entity anyway in f_a_view (actually tested this).

Edit: we should have a performace gain on multilingual sites instead, since prepare view hooks are run only on a single field translation instead that on all the available ones.

plach’s picture

@yched:

Any pending concern?

plach’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7
Tor Arne Thune’s picture

Subscribing

sun’s picture

Status: Needs review » Reviewed & tested by the community

This patch looks ready for me. Quite an oversight in the API. Should definitely be part of 7.1.

yched’s picture

Truly sorry for letting this drop off my radar. The patch looks good to me too.

Dries’s picture

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

Committed to 8.x.

We'll want to backport this to 7.x as this is a big oversight. Moving to 7.x for @webchick.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Agreed. This is an extra param, so shouldn't break anything.

Committed to 7.x. Thanks!

sun’s picture

Title: Prepare view hooks do not receive the language parameter » [regression] Prepare view hooks do not receive the language parameter
Status: Fixed » Needs work

Unfortunately, this patch seems to cause PHP notices on existing sites:

# Notice: Undefined offset: 1 in _field_invoke_multiple() (line 351 of modules\field\field.attach.inc).
# Notice: Undefined offset: 1 in _field_invoke_multiple() (line 352 of modules\field\field.attach.inc).
# Notice: Undefined offset: 4 in _field_invoke_multiple() (line 351 of modules\field\field.attach.inc).
# Notice: Undefined offset: 4 in _field_invoke_multiple() (line 352 of modules\field\field.attach.inc).
# Notice: Undefined offset: 3 in _field_invoke_multiple() (line 351 of modules\field\field.attach.inc).
# Notice: Undefined offset: 3 in _field_invoke_multiple() (line 352 of modules\field\field.attach.inc).
# Notice: Undefined offset: 2 in _field_invoke_multiple() (line 351 of modules\field\field.attach.inc).
# Notice: Undefined offset: 2 in _field_invoke_multiple() (line 352 of modules\field\field.attach.inc).
plach’s picture

Status: Needs work » Postponed (maintainer needs more info)

@sun:

I actually tested that change while rolling the patch and never got those notices: are you able to describe how to reproduce them?

plach’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Needs review
Issue tags: -Release blocker
FileSize
4.36 KB
5.21 KB

Ok, I managed to reproduce notices. Pretty subtle: they appear only when viewing more than one entity, for each one at least one field implementing hook_field_prepare_view() is populated and field values are populated for different languages for different entities. Long story short:

$entity1->field_test == array('en' => array('value' => 1))
$entity2->field_test == array('und' => array('value' => 2))

I didn't see notices before because I tested per-entity language suggestions without populating image fields, which implement hook_field_prepare_view().

Note that this is not exactly a regression, simply the correct behavior of per-entity language suggestions is now revealing another bug, which previously was unnoticeable. The attached patch should fix it and extend tests to capture it.

plach’s picture

Status: Postponed (maintainer needs more info) » Needs review
Issue tags: -translatable fields, -API change, -Needs backport to D7

#15: language-1089174-15-test.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +translatable fields, +API change, +Needs backport to D7

The last submitted patch, language-1089174-15-test.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
5.48 KB
6.27 KB

The previous tests did not capture the bug reliably due to some randomness in language weights. The attached ones should always do.

Status: Needs review » Needs work

The last submitted patch, language-1089174-18-test.patch, failed testing.

plach’s picture

Status: Needs work » Needs review

cool!

xatoo’s picture

whoops, wrong issue. Luckily a re-test won't hurt.

Status: Needs review » Needs work
Issue tags: +translatable fields, +API change, +Needs backport to D7

The last submitted patch, language-1089174-18-test.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
webchick’s picture

Issue tags: +Release blocker

Noting as a release blocker so we don't ship 7.1 with notices.

webchick’s picture

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

Well, wait though. Doesn't this need to be in 8.x first?

plach’s picture

sure, patches have been rolled against 8.x actually

sun’s picture

Status: Needs review » Reviewed & tested by the community

Applied the patch on the (otherwise untouched) site throwing notices and they disappeared.

The added condition looks correct to me. Didn't have a close look at the tests, but I trust @plach to get them right, and the additional test-only patch is excellent proof for me that this patch fixes the bug.

Dries’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Release blocker

Committed the patch in #18 to 8.x. Moving back to 7.x for @webchick.

droplet’s picture

across from #1164240: Notice: Undefined offset: 188 in _field_invoke_multiple() (line 351 of /home/.../field.attach.inc)

+        if (isset($grouped_items[$field_id][$langcode][$id]) && ($grouped_items[$field_id][$langcode][$id] !== array() || isset($entity->{$field_name}[$langcode]))) {

can't it using empty() ??

if (!empty($grouped_items[$field_id][$langcode][$id]) || isset($entity->{$field_name}[$langcode])) {
plach’s picture

They are not equivalent: we need to ensure that only populated values for existing translations are actually set back into the entity.

webchick’s picture

Title: [regression] Prepare view hooks do not receive the language parameter » Prepare view hooks do not receive the language parameter
Version: 7.x-dev » 8.x-dev
Priority: Major » Minor
Status: Reviewed & tested by the community » Needs work
Issue tags: -Release blocker

Awesome! Thanks for the expanded tests to prove that this newly identified bug stays fixed!

Committed to 7.x.

This all looked good, except for this:

+++ b/modules/field/tests/field.test
@@ -2729,29 +2733,55 @@ class FieldTranslationsTestCase extends FieldTestCase {
+    $null = NULL;
+    $grouped_results = _field_invoke_multiple('test_op_multiple', $entity_type, $entities, $null, $null, $options);
+    foreach ($grouped_results as $id => $results) {

Ew. Why this level of indirection? It's certainly not to save keystrokes; $null is longer than NULL. :) Can we get a quick fix in for that, or am I missing some context?

Powered by Dreditor.

plach’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs work » Fixed

The parameters getting $null are expected to be a variable reference, passing NULL will cause PHP to complain.

Anyway, thanks!

plach’s picture

Priority: Minor » Major
yched’s picture

About $null : yup, that is a not-too-beautiful pattern already present in other spots where _field_invoke() / _field_invoke_multiple() are called.

plach’s picture

@yched:

Are there possible alternative approaches?

yched’s picture

@plach : I tried but could not find any back when I was writing those $null vars :-(. Generic field ops invoker...

webchick’s picture

Oh, fascinating. Stupid PHP. :P Thanks!

yched’s picture

plach’s picture

@yched:

Yes, unfortunately it seems that only file_field_prepare_view() triggers those notices, while image_field_prepare_view() doesn't, and I tested everything with images.

Really sorry about this :(

plach’s picture

fietserwin’s picture

Can #1164230: Invalid argument supplied for foreach() in taxonomy_field_formatter_prepare_view() line 1429 of taxonomy.module be linked to this change or to the mentioned #1169426: Field invoke multiple does not handle field language correctly? As others, I do get the warning "Invalid argument supplied for foreach() in taxonomy_field_formatter_prepare_view() line 1429 of taxonomy.module" since my upgrade to 7.2. I tried debugging it, and came to the following ideas:

- Entities listed on the front page (or probably better: on any list that shows a set of teasers) that have none of their taxonomy fields filled in have a value of NULL in the $items array passed to function taxonomy_field_formatter_prepare_view().
- This NULL probably originates from the fact that the language suggestion results in 'und' instead of the language of the node.
- I traced changes in this part between 7.0 and 7.2 back to a commit related to this issue (http://drupalcode.org/project/drupal.git/commit/2a419f61aafc5ad490eef0be...)

What happens goes just to deep for me to follow, so I don't dare to suggest a patch for #1164230: Invalid argument supplied for foreach() in taxonomy_field_formatter_prepare_view() line 1429 of taxonomy.module, but will be cross linking all issues.

Status: Fixed » Closed (fixed)

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

plach’s picture

@yched: @sun:

I think we have a RTBC candidate for #38 on #1169426-22: Field invoke multiple does not handle field language correctly (which has become a release blocker), a review would be welcome.

henwu’s picture

Subscribing

andypost’s picture