(Change notice update) hook_field_formatter_prepare_view does not make use of hook_entity_view_mode_alter causing major errors

The original issue that added this is still open, see #1154382: View mode no longer can be changed because of a problem with it. Is this the same problem? If so then we should close either of those (I'd suggest to keep this open and mark the other as fixed). The tests that were added will have to be updated, not sure if the patch in the other issue is already doing this.

Yes, let's mark that one as fixed and point to this issue, taking the patches over. That issue has been sitting there for way too long, something that happens quite often when issues are kept open for follow-up fixes.

Adding tag.

Working on a patch with tests.

Ok, first patch ist tests-only, that one is expected to fail. The second one contains the test plus the unchanged fix from #1154382-113: View mode no longer can be changed.

Good, the test fails with the exact same message given in the original report.

@Berdir: Is there anything left to do here?

+++ b/modules/node/node.testundefined
@@ -2698,3 +2698,60 @@ class NodeEntityViewModeAlterTest extends NodeWebTestCase {
+    $edit["body[$langcode][0][value]"] = t('Data that should appear only in the body for the node.');
+    $edit["body[$langcode][0][summary]"] = t('Extra data that should appear only in the teaser for the node.');

Shouldn't use t() or maybe even randomName(), we have a custom assertion message anyway.

Call to t() removed, also in the preceding test which served as the copy-paste source.

This fixes the issue here.

+++ b/includes/common.inc
@@ -7833,6 +7833,55 @@ function entity_prepare_view($entity_type, $entities, $langcode = NULL) {
+ * If adding a new entity similar to nodes, comments or users, you should
+ * invoke this function during the ENTITY_build_content() or
...
+  // To ensure hooks are never run after field_attach_prepare_view()
+  // only process items without the entity_view_prepared flag.

The patch looks good, there were just a couple of comments not wrapping correctly.

I can confirm #12 fixes the issue. I can't spot anyhting bad with it and in #8 we can see test coverage is ok. I had a quick look to the D8 code and agree it should not be affected by this, we may want to forward-port tests.

Ugh, horrible issue - thanks for a great bug report and patch, though. Makes me wish we never added this new hook to Drupal 7, but it's too late :( I can't think of another way to fix it except something like this patch so I guess that's what we have to do.

I do see one major problem, though. To use nodes as an example (but it also applies to others too):

 function node_view_multiple($nodes, $view_mode = 'teaser', $weight = 0, $langcode = NULL) {
-  field_attach_prepare_view('node', $nodes, $view_mode, $langcode);
-  entity_prepare_view('node', $nodes, $langcode);
+  $nodes_by_view_mode = entity_view_mode_prepare('node', $nodes, $view_mode, $langcode);
+  foreach ($nodes_by_view_mode as $view_mode => $nodes) {
+    field_attach_prepare_view('node', $nodes, $view_mode, $langcode);
+    entity_prepare_view('node', $nodes, $langcode);
+  }
+
   $build = array();
   foreach ($nodes as $node) {
     $build['nodes'][$node->nid] = node_view($node, $view_mode, $langcode);

So the code at the top takes into account the possibility of a different returned $view_mode for each node (which it definitely has to) but then further down node_view() is called with the final $view_mode only. Since hook_entity_view_mode_alter() won't be invoked again during the node_view() call, this could actually break people's existing (and previously working) code. So I think we have to arrange to make sure the correct view mode for each particular node is always passed in during each node_view() call.

Other than that, everything looked great to me except a few minor nits:

1. entity_view_mode_prepare() is missing documentation of the $view_mode parameter.

2. +    // Hide the tags field on the default display
   +    $instance = field_read_instance('node', 'field_tags', 'article');
   

   Pretty sure that should be field_info_instance() instead?

3. +/**
   + * Tests changing view mode for nodes when teaser display shows a field which is missing on the default display.
   + */
   +class NodeEntityViewModeAlterMinimalDefaultDisplayTest extends NodeWebTestCase {
   +
   +  public static function getInfo() {
   +    return array(
   +      'name' => 'Node entity view mode minimal default display',
   +      'description' => 'Test changing view mode when teaser display shows a field which is missing on the default display.',
   +      'group' => 'Node'
   +    );
   +  }
   

   That's a pretty unwieldy class name and description (and the docs go way over 80 characters also) :)

   I'd suggest just ditching it as a separate class altogether and moving it into the existing NodeEntityViewModeAlterTest class as a new test method. So basically:

   /**
    * Tests changing view modes for nodes.
    */
   class NodeEntityViewModeAlterTest extends NodeWebTestCase {
   
   ....
   // Existing code that is already in this class.
   ....
   
     /**
      * Tests fields that were previously hidden when the view mode is changed.
      */
     function testNodeViewModeChangeHiddenField() {
       // Hide the tags field on the default display.
       $instance = field_info_instance('node', 'field_tags', 'article');
       $instance['display']['default']['type'] = 'hidden';
       field_update_instance($instance);
   .....
   

   And then basically the same test code that the patch added after that.

We also definitely should forward-port the tests to Drupal 8, but I think that can wait until after commit given that it seems pretty clear this bug doesn't exist in Drupal 8 (plus it's a relatively serious bug so better to fix in Drupal 7 sooner than later).

After Drupal 7 commit, I suppose we'll also have to update this change notification for the new pattern: https://drupal.org/node/1833086

Working on the patch...

Ugh, I need to think about how to handle the weights. Attempt from #17 obviously does not work.

I consider #17 as invalid, therefore interdiff is against #12.

I've tested this out and it works great for me. I'm using this with custom entity types extending the Entity API module. I've posted a patch for Entity API over here #2139639: Support updated hook_entity_view_mode_alter() to support this implementation. Sure would be nice to see this in the next point release of D7.

Hate to do this because I'd really like to see this patch go in now, but I don't think it's a good idea to remove the sorting behavior of functions like node_view_multiple(). Although drupal_render() will sort the entities correctly anyway (such that it shouldn't make a difference in the end), these are heavily-called functions and people could be relying on a consistent order of the returned array.

So here's a fix for that (and I also made the node module's internal variables consistent with the rest, i.e. $entities rather than $entitys).

Hopefully we can still get this patch in soon though.

I'm fine with #21. I cannot RTBC it though, another review is still necessary.

#21 works perfectly for me! :)

Thank you so much for your work!

Likely testbot glitch - moving back to RTBC.

#21 solved this issue for me: #2322031: Fields don't display when altering view mode

Committed to 7.x - thanks!

So we need a couple things here, based on my comment above:

We also definitely should forward-port the tests to Drupal 8, but I think that can wait until after commit given that it seems pretty clear this bug doesn't exist in Drupal 8 (plus it's a relatively serious bug so better to fix in Drupal 7 sooner than later).

After Drupal 7 commit, I suppose we'll also have to update this change notification for the new pattern: https://drupal.org/node/1833086

The first could be filed as a separate followup issue, I think.

Draft change record here: https://www.drupal.org/node/2369141

I decided it made more sense to create a new one rather than put everything in the old one.

But I also modified the old one from https://www.drupal.org/node/1833086 to point to it:
https://www.drupal.org/node/1833086/revisions/view/6918819/7799713