When a node with a multiple value file field contains items that are set to not be displayed, previewing changes to the node will remove all but the first undisplayed item from the field values.

Steps to reproduce in a fresh Drupal 7 installation:

  1. Add a file field to a content type.
  2. Set the number of values to unlimited.
  3. Enable Display field.
  4. Enable Files displayed by default.
  5. Log in as a regular user who can create content.
  6. Create a node and attach at least two files to the field. Save the node.
  7. Edit the node and uncheck the Display box for these files. (Or uncheck them when first creating the node. The result is the same.)
  8. Click Preview. All but the first of the file values will have disappeared from the list.
  9. If you save the node, it will save what the form says, so the other files will no longer be associated with the node (or at least the current revision.)

I suspect that this may be due to file_field_prepare_view() being called during the node preview, because that function simply removes non-displayed items from the entity. It might also be related to #1990818: node_preview() shouldn't call hook_field_load(), but I haven't had a chance to look at the actual code, so I'm really just guessing at the reasons behind this.

Marking this bug as major because it's very easy for content editors to corrupt data simply by previewing their changes to a node.

Files: 
CommentFileSizeAuthor
#36 2001308-preview_remove_files-36.patch3.67 KBstefan.r
PASSED: [[SimpleTest]]: [MySQL] 41,139 pass(es).
[ View ]
#36 interdiff.txt543 bytesstefan.r
#27 2001308-preview_remove_files-27.patch3.67 KBstefan.r
PASSED: [[SimpleTest]]: [MySQL] 40,916 pass(es).
[ View ]
#27 2001308-preview_remove_files-27-testonly.patch1.36 KBstefan.r
FAILED: [[SimpleTest]]: [MySQL] 41,099 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#15 test_file_preview-2001308-15.patch1.54 KBmarthinal
PASSED: [[SimpleTest]]: [MySQL] 60,177 pass(es).
[ View ]
#9 drupal8.node-module.2001308-9.patch969 byteshelmo
PASSED: [[SimpleTest]]: [MySQL] 58,799 pass(es).
[ View ]
#1 node-preview-2001308-1.patch2.32 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 41,348 pass(es).
[ View ]

Comments

David_Rothstein’s picture

Version:7.22» 7.x-dev
Component:field system» node.module
Status:Active» Needs review
StatusFileSize
new2.32 KB
PASSED: [[SimpleTest]]: [MySQL] 41,348 pass(es).
[ View ]

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.

micahw156’s picture

Yes, even if this isn't the final solution, it solves the problem and is a viable workaround for now. Thanks!

Bill Choy’s picture

Priority:Major» Critical

Elevating to critical issue, because of data loss issue.

jiff’s picture

Status:Needs review» Reviewed & tested by the community

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.

David_Rothstein’s picture

Version:7.x-dev» 8.x-dev
Status:Reviewed & tested by the community» Needs work
Issue tags:+needs backport to D7

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?

David_Rothstein’s picture

jthorson’s picture

David,

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

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

twardnw’s picture

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

helmo’s picture

Status:Needs work» Needs review
StatusFileSize
new969 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,799 pass(es).
[ View ]

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

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

<?php
unset($cloned_node->in_preview);
?>
webchick’s picture

Status:Needs review» Needs work
Issue tags:+Needs tests

Needs tests to check that the regression is fixed.

tvn’s picture

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

tvn’s picture

Issue tags:+affects Drupal.org

.

swentel’s picture

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

catch’s picture

Title:Node preview removes file values from node edit form for non-displayed items» Tests for: Node preview removes file values from node edit form for non-displayed items
Issue summary:View changes

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

marthinal’s picture

StatusFileSize
new1.54 KB
PASSED: [[SimpleTest]]: [MySQL] 60,177 pass(es).
[ View ]

Possible test attached.

marthinal’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 15: test_file_preview-2001308-15.patch, failed testing.

marthinal’s picture

Status:Needs work» Needs review
helmo’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs tests

D8 test looks OK.

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

webchick’s picture

Title:Tests for: Node preview removes file values from node edit form for non-displayed items» Node preview removes file values from node edit form for non-displayed items
Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Needs work

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.

mgifford’s picture

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

mgifford’s picture

Component:node.module» ajax system
Issue tags:+Needs tests

@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.

drumm’s picture

Status:Needs work» Needs review
micahw156’s picture

We've been using the patch from #1 in all of our Drupal 7 builds since it was first posted over a year ago. It solves the problem for us, and we still break content in the rare instances when we forget to reapply this when updating core.

We include this patch in all of our drush make files, and then of course have to reapply it every time we update core. Yes, it probably needs tests but it would be incredibly nice to just have this patch committed and be done with it.

If @drumm's status change on this implies the possibility of moving forward without adding tests, then yes, RTBC.

drumm’s picture

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 does not correctly attach fields. That looks likely to be be related, but I don't know the internals well enough to comment credibly.

webchick’s picture

Status:Needs review» Patch (to be ported)

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.

stefan.r’s picture

Status:Patch (to be ported)» Needs review
StatusFileSize
new1.36 KB
FAILED: [[SimpleTest]]: [MySQL] 41,099 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new3.67 KB
PASSED: [[SimpleTest]]: [MySQL] 40,916 pass(es).
[ View ]
stefan.r’s picture

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.

dcam’s picture

Status:Needs review» Needs work
+++ 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.

dcam’s picture

Status:Needs work» Reviewed & tested by the community

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.

The last submitted patch, 27: 2001308-preview_remove_files-27-testonly.patch, failed testing.

David_Rothstein’s picture

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.

dcam’s picture

Oh well. It was worth a shot.

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

stefan.r’s picture

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.

David_Rothstein’s picture

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).

stefan.r’s picture

Issue tags:-needs backport to D7, -Needs tests
StatusFileSize
new543 bytes
new3.67 KB
PASSED: [[SimpleTest]]: [MySQL] 41,139 pass(es).
[ View ]

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

dcam’s picture

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.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 36: 2001308-preview_remove_files-36.patch, failed testing.

Status:Needs work» Needs review
stefan.r’s picture

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 36: 2001308-preview_remove_files-36.patch, failed testing.

Status:Needs work» Needs review
helmo’s picture

Issue summary:View changes
Status:Needs review» Reviewed & tested by the community

reviewed again... works as expected.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 36: 2001308-preview_remove_files-36.patch, failed testing.

Status:Needs work» Needs review

David_Rothstein’s picture

Status:Needs review» Reviewed & tested by the community

Looks like a testbot fluke.