Taxonomy terms disappear from node preview if previewed more than once

I've reproduced this on my CVS copy, going to try to figure it out. subscribing.

It doesn't have to be a new term or a new article. Reproducible by editing or creating any Article, with any tags, clicking preview, then clicking preview again without saving.

Did some more testing, it's any node with any taxo references. Also, the terms don't appear in the preview display of the node.

Reflecting this in title.

This bug is probably a side-effect introduced by #735800: node form triggers form level submit functions on button level submits, without validation. In that issue, we made the entity editing workflow preserve the same entity across the multi-step process, so after each "preview" click, you still have the same object in $form_state['node']. However, field_attach_prepare_view() only calls the field's hook_field_formatter_prepare_view() once per entity, so taxonomy_field_formatter_prepare_view() isn't called on the second "preview". But that's the function that adds 'taxonomy_term' to $item, and because the second "preview" click resets $items to what was submitted by the form during field_attach_submit(), the lack of taxonomy_field_formatter_prepare_view() running is causing the bug.

I suspect the solution is for $entity->_field_view_prepared to be cleared somewhere in the #builder_function pipeline, but I'm not sure where yet.

Since I think this is a generic entity/field system issue that is simply surfacing for taxonomy, I'm changing the issue component accordingly.

Obviously, now that we've uncovered this bug, we'll need to add tests.

As mentioned in #4, the entity editing workflow preserves the same copy of our entity across the multi-step process due to changes in #735800: node form triggers form level submit functions on button level submits, without validation.

The culprit in this specific instance is that _field_view_prepared gets set during preview, but allowing node_preview to alter our $node may have other unforeseen side-effects since node_preview makes a lot of operations on the node passed to it. It used to isolate some changes (the theme call) by creating a clone. It was only creating a clone before calling the theme function, however.

This patch creates a clone of the $node passed to node_preview at the beginning of that function which is the only $node any operations are performed on during the function call.

Tests are up next, but I want opinions on this solution.

Re-evaluating where we clone $node within node_preview() may have its own merits worth exploring in another issue, but I think this issue isn't just for nodes. Fields can be added to comments, users, and other entity types, so I think the solution to this issue belongs inside the Field API. How about this?

Agreed. A solution inside the Field API is better.

I've moved my patch into a new issue #844502: Calling node_preview modifies the previewed node.

It's a nice piece but it needs tests.

Field API maintainer recommends an API change.

I'm not a big fan of _field_view_prepared either, of course. We went that road for lack of a better solution. I fully agree that the ideal solution is the one Barry points (make field_attach_view() a multi-entity operation so that it takes care of calling field_attach_prepare_view() itself). Even back then, it was a daunting change.

I'd tend to agree with Barry's analysis and see the solution in the range of #6.
Althgough : theme_node_preview() currently also clones the node. so if we go with #6, we should probably remove that one.

(hm, looking at the code in node_preview(), I cannot make sense of the

_field_invoke_multiple('load', 'node', array($node->nid => $node));

line. It possibly made sense before the changes to entity persistency in multi-step forms, but now, I'm not sure what it stands for. Sadly, no time to investigate right now)

+++ modules/field/field.attach.inc	5 Jul 2010 13:04:50 -0000
@@ -1036,10 +1036,16 @@ function field_attach_delete_revision($e
  * Prepare field data prior to display.

@@ -1072,12 +1078,15 @@ function field_attach_prepare_view($enti
- * Returns a renderable array for the fields on an entity.
+ * Return a renderable array for the fields on an entity.

The third person form is actually correct. So we either fix "Prepares" accordingly or we just leave both summaries alone.

37 critical left. Go review some!

Looks good to me.

#16 is excellent. Let's now fix node_preview() to comply and add tests for the originally reported problem.

+++ modules/field/field.attach.inc	5 Jul 2010 15:11:09 -0000
@@ -1036,10 +1036,16 @@ function field_attach_delete_revision($e
+ * This function and field_attach_view() are mutually dependent. For
+ * any specific entity, you must call this function before
+ * field_attach_view() and you must call field_attach_view() before
+ * calling this function again.

Wait a sec! I'm interpreting this to mean: "it is wrong to call f_a_prepare_view() twice for the same entity before calling f_a_view() for that entity". But isn't that the whole reason we have the _field_view_prepared flag? So that the same entity can appear in multiple sets, and each set can be "prepared" before any of the sets is viewed? If we follow the new documentation to the letter, why have the flag at all?

From #10:

I don't see why field_attach_submit() is the correct place to clear the flag. That just ties Field API's behavior even more tightly to form/page load semantics.

Even if we were to follow the new docs, isn't the following sequence still possible?
- f_a_prepare_view()
- f_a_submit()
- f_a_view()
If we fix node_preview() to clone in a sane way, we probably won't encounter that sequence any more, but nothing in the Field API, even with the #16 docs, tells me that sequence is wrong. But that is the sequence that causes this issue's bug, because f_a_submit() overwrites whatever f_a_prepare_view() did with $items without also clearing the _field_view_prepared flag, and that leaves $entity in an inconsistent state.

But isn't that the whole reason we have the _field_view_prepared flag? So that the same entity can appear in multiple sets, and each set can be "prepared" before any of the sets is viewed?

Not really... the prepare view steps run based on formatters, which can differ depending on the view mode. The main reason for the prepared flag is that http://api.drupal.org/api/function/node_build_content/7 and http://api.drupal.org/api/function/node_view_multiple/7 both call those functions, but node_view_multiple() also calls node_build_content() via node_view() - so there has to be a way to stop those running twice.

I'd like to get this taken care of, but what solution are we going with here? The latest patch is only a documentation change. Do we roll #23 and #7 together?

Wrong status.

This is #6 and #23

The problems with #6 and thereby #27, if we go that route are as chx said in #844502: Calling node_preview modifies the previewed node., the onus of cloning should be on the function which calls node_preview since only the caller knows whether it's safe for its node to be modified, and it is expensive to assume that it's not safe and always clone the node.

However, node_preview was cloning the node before this patch anyways (just not for the entirety of the call), so we might be risking some kittens' lives if we start considering removing the clone.

Besides, the test is broken, since it doesn't do a deep comparison, as bjaspan pointed out in #844502: Calling node_preview modifies the previewed node..

Also, isn't #846356: node_preview() calls _field_invoke_multiple() a duplicate? That call is why this issue occurs.

Overlapped with the bot.

Fixed the syntax error.

Here's another approach. This bug is because we do half of field_attach_(prepare)_view, clone the node, then do the other half. My solution was to clone the node, then to both parts.

After thinking about and agreeing with chx that node_preview should be allowed to modify the node, and therefore cloning it is unneccesary, why was the clone in there at all, all it was doing was breaking our fields.

This patch removes the clone altogether.

Edit: See below, this is the wrong file.

Wrong file on that last comment.

Note that theme_node_preview() also clones the node...

We are cloning since I'm using Drupal, so I don't know the real reason. However, I believe that we're cloning, since in previous Drupal versions, certain node properties were identical and thus altered and overwritten when viewing and previewing a node. I think that this is no longer the case with D7, so we should be fine without cloning. Unless we have similar code like the @todo in http://api.drupal.org/api/function/user_comment_view/7 somewhere in this game.

Re #35: I propose that we don't clone the node at all, unless we can discover somewhere that it breaks something. So far, no tests have failed with it gone.

My guess is with sun, that whatever we were protecting from here in Drupal's history has since been fixed.

If this clone really needs to be done, it needs better docs for why, and a test for the corner case that it's protecting against.

Off topic: As for why theme_node_preview is cloning the node, it only does it for the teaser, so perhaps that's related to #721754: A node cannot be displayed in different view mode on its own page?

unset() seems right.

I agree with not cloning unless we find we have to.

I don't know about the architecture of the patch but I applied it and it solves the problem of disappearing tags as well as the huge amount of error messages upon previewing.

I can roll a new patch with the unset() later this evening, but I think the biggest issue here is the lack of tests.

Any ideas for what a test would even do? Do we test for exactly the case that discovered the issue (make a node with taxo terms, preview twice)? Or do we add tests to field API?

Okay. I'll add the unset() and the test strategy from #42.

Here's a quick stab at the test. I know it's broken since it passes with and without the changes to node.pages.inc.

I'm not sure how exactly to test this, since doing two drupalPost() in a row seems to start two separate edit workflows, clearing the intermediate state which causes our problem. Do I need to do something different with my drupalPost() calls? Is there another Simpletest call I should be making instead?

I think you should pass NULL as the $path for drupalPost() the second time. See http://drupal.org/node/265762

Re #45: Thanks, I was reading through the docs trying to figure it out but didn't see that.

This patch includes my removal of the clone from node_preview, bjaspan's doc changes, and tests of node previews with taxo terms (one of which previews an already previewed node, testing the case that discovered our issue).

I've verified that the test fails on the CVS version of node_preview and passes with my changes.

failed to retrieve [844388 (1).patch] from project client

I wonder if it didn't like the filename. Trying again.

Obviously I agree.

#48: 844388.patch queued for re-testing.

OK, these changes are actually small and have been signed off by all the maintainers. Committed to CVS HEAD.

Use this patch, it will work fine
http://drupal.org/files/issues/field_default_prepare_view-934726-36.patch

I'm really reluctant to open this fairly epic issue, and it may be better to open a new issue, but the history of the comments above go through the problem.

Issue #1025870: Previewing a node causes $node->uri['path'] to be set to 'node/' in hook_node_insert() is a problem that exists because the $node object is not cloned when previewing. Taxonomy fields rely on hook_field_formatter_prepare_view() being invoked to display properly (which caused this issue originally and probably #1062396: Taxonomy terms fails in node preview page if hidden format selected too), but this won't happen if that pesky $node->_field_view_prepared is set. Surely previewing a node shouldn't affect the data in the $node object that is being created/edited on the node form? Currently, it does.

Since lotyrin said way back in comment #37:

I propose that we don't clone the node at all, unless we can discover somewhere that it breaks something.

...I suggest that since not cloning does break something after all, it needs fixing. I cannot understand why previewing would be allowed to alter the original node object like this. If I edit or create a node, I don't expect previewing to change my node!

Of course, I realise it's not as simple as that, I respect that this issue has already gone through much discussion.

This issue history is interesting and relevant to the linked discussions, but this issue is resolved. We can consider cloning or other solutions in the other issues.

Sorry to drag up an old issue, is this really fixed? I'm running 7.21 and I'm seeing the same problem.

Having a little dig around, it seems to me that $items in taxonomy_field_formatter_view (taxonomy.module) does not receive the expected taxonomy names, and so for taxonomy_term_reference_plain at least, no term appears at all (on a second preview).
I'm happy to log a new bug if this is completely unrelated.

Here's my workaround if anyone is interested:

function mymodule_field_formatter_info() {
  return array(
    'taxonomy_term_reference_plain' => array(
      'label' => t('Plain text'),
      'field types' => array('taxonomy_term_reference'),
    ),
  );
}

function mymodule_content_type_field_formatter_view($entity_type, $entity, $field, $instance, $langcode, $items, $display) {
  $element = array();
  if ($display['type'] == 'taxonomy_term_reference_plain') {
    foreach ($items as $delta => $item) {
      $name = taxonomy_term_load($item['tid']);
      $name = ($name->name) ? $name->name : '';
      $element[$delta] = array(
        '#markup' => check_plain($name),
      );
    }
  }

  return $element;
}

PS. I initially thought this may have something to do with this line of code in taxonomy.module (that looks like a bug) but it turned out to be unrelated:
$name = ($item['tid'] != 'autocreate' ? $item['taxonomy_term']->name : $item['name']);