Split from #844388: Taxonomy terms disappear from node preview if previewed more than once.

Currently, since node_preview is passed it's node as a reference, anything operations performed on the previewed node take effect outside of node_preview.

For instance, #844388: Taxonomy terms disappear from node preview if previewed more than once: previewing a node twice resulted in an error since field_attach_prepare_view() was called during preview, caused side-effects and due to changes in #735800: node form triggers form level submit functions on button level submits, without validation the node and its side-effects were passed along to the second preview step, where the error occurred. #844388: Taxonomy terms disappear from node preview if previewed more than once solved the issue in the Field API, but other core or contrib modules that expect node_preview not to alter a node may encounter similar bugs.

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. Any current (or future) operations performed on the node are isolated.

This allows developers of modules that add features to the node edit form to avoid writing a test which previews a node twice, to make sure both previews have the same result.

Tests to come.

CommentFileSizeAuthor
#9 844388-preview-noclone-field-prepare-docs.patch2.67 KBlotyrin
#1 844502-node_preview.patch1.73 KBlotyrin
844388-clone-node.patch1.09 KBlotyrin

Comments

lotyrin’s picture

lotyrin commented
Issue tags: -Needs tests
StatusFileSize
new 844502-node_preview.patch1.73 KB

Now with a test.

effulgentsia’s picture

effulgentsia commented

+1 from me. Should we do the same for comment_preview()?

lotyrin’s picture

lotyrin commented

If the same is done for comment_preview, I think it should be a separate issue.

chx’s picture

chx commented

I disagree. The onus of cloning is on the caller. node_view changes the node too and noone protests.

Edit: cloning is expensive and the called function can't know whether it's ok to change the object or not. The caller knows that it will need the object later or not.

bjaspan’s picture

bjaspan commented
Status: Needs review » Needs work

In #844388: Taxonomy terms disappear from node preview if previewed more than once I suggest that Field API's behavior is "correct" (I wish it were different, but it is what it is) but I posted a patch which documents the dependency between field_attach_prepare_view() and field_attach_view() more explicitly. If that is correct, this issue should inherit that issue's criticallity.

+++ modules/node/node.test
@@ -387,6 +387,16 @@ class PagePreviewTestCase extends DrupalWebTestCase {
+    $this->assertTrue($node == $before, 'Node unchanged by preview.');

This does not perform a deep comparison. Proof:

$node->foo = "foo";
$node->bar->foo = "bar";
$before = clone $node;
print(serialize($node == $before))."\n";
$node->bar->foo = "quux";
print(serialize($node == $before))."\n";

outputs

b:1;
b:1;

37 critical left. Go review some!

damien tournoud’s picture

damien tournoud commented

@chx: the clone was already there, just not in the correct place. But I have to agree that we should move it even higher, to node_form_build_preview().

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Priority: Normal » Critical
Issue tags: +Needs tests

Critical instead of aforementioned issue. Also needs (better) tests, as described in #5.

effulgentsia’s picture

effulgentsia commented
Status: Needs work » Closed (duplicate)

Let's finish this off in #844388: Taxonomy terms disappear from node preview if previewed more than once. Neither patch is that big on its own, so they can be reviewed together, and I think doing so helps keep the specific use-case front and center.

lotyrin’s picture

lotyrin commented
StatusFileSize
new 844388-preview-noclone-field-prepare-docs.patch2.67 KB

Ignore this. Crosspost.