Node preview removes file values from node edit form for non-displayed items

Thanks for the very clear bug report!

Haven't fully thought through the consequences, but I wonder if just cloning the node object before previewing it (as in the attached patch) would be the right way to fix it? Seems to work at any rate.

It looks like this same code change would be needed in Drupal 8, but for whatever reason I can't reproduce the bug in Drupal 8 at the moment, so leaving at Drupal 7 for now.

Elevating to critical issue, because of data loss issue.

Also not sure if cloning the node is the best way to do this, but it solved the issue for me. I couldn't recreate the issue in d8 either.

Since effectively the same code exists in Drupal 8, I think we need to fix this there first (even if for whatever reason the symptoms aren't as bad). It still seems to be the case in Drupal 8 that calling node_preview($node) will modify the provided $node object, and this could have other unexpected results.

Perhaps we need tests also?

I'm curious if this issue is fixed by the patch in #1289336: Calling node_view for the same node with multiple view modes on the same page during node preview does not correctly attach fields also, by the way.

David,

I confirmed that the fix from #1289336: Calling node_view for the same node with multiple view modes on the same page during node preview does not correctly attach fields does NOT resolve this issue (in D7).

Edit: And confirmed that the patch from #1 does.

I have also tested the patch in #1 and had success resolving this issue in D7

Here's a patch for D8 for testing...

I've also removed the unset line ... the whole cloned object is no longer used...

unset($cloned_node->in_preview);

Needs tests to check that the regression is fixed.

Adding tag since this issue affects D7 Drupal.org, we'll have to live with a patch till it's in core.

.

Tried reproducing this on D8, but works fine, so I guess we don't need the code here, but only tests for D8 then ?

Yep. Re-titling, leaving at critical since this is still nasty for 7.x / d.o

Possible test attached.

15: test_file_preview-2001308-15.patch queued for re-testing.

D8 test looks OK.

Lets get this in and move on the D7 fix which is also ready.

Committed and pushed to 8.x, thanks!

Marking down to 7.x, and needs work since we need a patch that combines the fix + the test for D7.

1: node-preview-2001308-1.patch queued for re-testing.

@David_Rothstein patch in #1 still applies.

I was able to confirm that it is still a bug and that the patch fixes it.

As @webchick says though, we need tests for D7.

Adding tests or not is not up to me. That's a question for David_Rothstein.

We have been running #1 on Drupal.org for awhile, along with #1289336: Calling node_view for the same node with multiple view modes on the same page during node preview does not correctly attach fields. That looks likely to be be related, but I don't know the internals well enough to comment credibly.

To clarify, the tests are already written and are in the D8 patch, we just need a single unified patch for D7 that contains both the fix and the tests.

This patch combines the fix in #1 and the test as requested by @webchick. Patch is red with test only and green with fix+test so assuming this should be RTBC soon enough.

+++ b/modules/file/tests/file.test
@@ -917,6 +918,17 @@ class FileFieldDisplayTestCase extends FileFieldTestCase {
+    // Test that fields appear as expected after during the preview.

Can we get this comment's grammar fixed? We'll have to do a follow-up to fix it in D8 also.

Otherwise the test looks good. I don't know why Testbot ignored the tests-only patch, so I ran the test locally and the second assertion did fail.

I decided to go ahead and RTBC #27. Maybe @David_Rothstein can fix the comment on commit. I'm pretty sure it's supposed to say Test that fields appear as expected during the preview. If there's a chance that it can still get into 7.33 then it seems a shame to hold up a critical issue because of a comment.

Hm, I could, although I am also not sure what the 'cardinality' => FIELD_CARDINALITY_UNLIMITED change is doing in this patch...

This was RTBC pretty late and I think I'm running out of steam to get this into 7.33.... sorry. Also, the bug was never fixed in Drupal 8? See my comment above:

Since effectively the same code exists in Drupal 8, I think we need to fix this there first (even if for whatever reason the symptoms aren't as bad). It still seems to be the case in Drupal 8 that calling node_preview($node) will modify the provided $node object, and this could have other unexpected results.

The Drupal 8 code in question may not exist anymore though.. I'm not sure.

Oh well. It was worth a shot.

In #13 @swentel said it couldn't be reproduced in D8 anymore.

The unlimited cardinality was a backport from the D8 test, I assume this is just in order to add multiple files?

If I look at that test the comment meant to say during the preview as the assertion happens after clicking the preview button.

Oh, oops - sorry!... for some reason when I read that FIELD_CARDINALITY_UNLIMITED line in the patch I didn't think it was part of the test but rather thought it was in a completely different file. I have no idea why I thought that.

The comments above about not being able to reproduce on Drupal 8 were all about the specific steps to reproduce in this issue, at least as far as I can tell. But the code was still there, and calling node_preview($node) could still change the node rather than simply returning the preview HTML. However, I looked at the code now and I think as of a couple months ago it's all gone from Drupal 8, since #1510544: Allow to preview content in an actual live environment changed how node previews work entirely (they now happen at their own URL).

So I think we can consider this RTBC for Drupal 7 after all (minus the code comment fix which can be done on commit).

Good! Here's the comment fix so you guys don't have to do it on commit :)

for some reason when I read that FIELD_CARDINALITY_UNLIMITED line in the patch I didn't think it was part of the test but rather thought it was in a completely different file. I have no idea why I thought that.

I'm thinking it was because you were looking at it in the very early morning hours at the end of a marathon session of testing and committing issues. I certainly wouldn't blame you for being mentally exhausted. ;)

Thanks @stefan.r! #36 looks good to me. RTBC.

reviewed again... works as expected.

Looks like a testbot fluke.

Committed to 7.x - thanks!