From a conversation started here: http://drupal.org/node/104027#comment-181397

The bug in Drupal 4.7+: node_preview cannot actually render a true "preview" since it does not call node_submit() which calls hook_submit, etc. which may alter the node's content.

suggested fix to node_preview (untested, real patch to follow)

     // Display a preview of the node:
     // Previewing alters $node so it needs to be cloned.
     if (!form_get_errors()) {
       $cloned_node = drupal_clone($node);
+      $cloned_node = node_submit($cloned_node);
       $cloned_node->in_preview = TRUE;
       $output = theme('node_preview', $cloned_node);
     } 
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

Status: Active » Needs review
FileSize
666 bytes

Tested the proposed solution- it fixes the bug for me in the test case of a node field computed in hook_submit from multiple form values.

patch attached for 5.x

pwolanin’s picture

FileSize
667 bytes

Patch attached for 4.7 also.

I'm considering moving this to critical.

To see the bug in action for 4.7, you can use the event module - note that without this patch previews do not show the event date/time correctly: http://drupal.org/project/event

You can also try my Volunteer timeslots add-on for the event module- without this patch the schedule does not appear correctly during previews: http://drupal.org/project/volunteer_timeslots

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

This is really a simple patch...

pwolanin’s picture

Title: node_preview should call node_submit » node previews broken by missing hook_submit

better title

drumm’s picture

Status: Reviewed & tested by the community » Needs work

There is now an extra call to node_teaser(). One is probably dead code. Actually, a lot of the code above the lines patched might be duplicated now.

pwolanin’s picture

hmmm, yes- I think maybe instead of node_submit() it may be sufficient to just add in:

  node_invoke($node, 'submit');
  node_invoke_nodeapi($node, 'submit');
pwolanin’s picture

Status: Needs work » Needs review
FileSize
2.82 KB

Ok, added the module invoke calls and trying to also clean up other problems with this function:

1) all manipulations should be done on the cloned node to insure consistent behavior between PHP 4 and 5

2) this function is never called if there are form errors (see node_form_add_preview), but it's probably harmless to re-check.

3) I think the access checks are not needed (as per http://drupal.org/node/83288)

(If necessary I can break this into multiple patches)

pwolanin’s picture

same patch for 4.7.x

Dries’s picture

Mmm, doesn't the submit hook invoke database queries? We don't want to store the node when still previewing it.

pwolanin’s picture

@Dries-
No, at least not as I understand the API. Saving data to the DB should only be done at hook_insert/update. Hook_submit is for making any changes needed before this step, but after validation. In general, I would think that this is the state of the node that's suitable for viewing.

I'm not sure the developer docs really provide a good example:

http://api.drupal.org/api/HEAD/function/hook_submit

I'm more used to the sort of thing exemplified by these implementations in core:

http://api.drupal.org/api/HEAD/function/poll_submit
http://api.drupal.org/api/HEAD/function/book_submit
http://api.drupal.org/api/HEAD/function/forum_submit

pwolanin’s picture

Hmmm, unfortunately, this gives a funny result for the poll module. Apparently since it's a multistep form, node_preview gets called twice. This causes drupal_set_message() to get called twice with the patched version because the patched version ends up with a body and teaser that are not identical for a poll.

However, I think this is a bug somewhere in the multistep form handling. It's only because poll happened not to show a separate teaser and body that his does not occur usually. If you add $form['#multistep'] = TRUE; to *any* node type (e.g. forum) you'll see the same behavior.

pwolanin’s picture

Ok, Attached patch includes a temporary fix for poll module for 5.x

beginner’s picture

I have tested on HEAD with the event.module (volunteer_timeslots does not work with HEAD).

The patch fixes the preview, as advertised.
The removal of the permission check is discussed in the issue mentioned above. I don't feel competent to comment on that one, but apparently a similar patch has been committed before, so it should be ok.

What do you mean by 'temporary' fix for the poll.module? The poll.module fix seems ok.

You also imply that the check if (!form_get_errors()) { is unnecessary. If so, why keep it? It can be removed in another patch, in another issue, and not in this one, though. It is a different issue and we don't want to make the patch harder to read.

here:

 else if (isset($cloned_node->uid) && ($cloned_node->uid > 0)) {

you could simply use the following, no?

   else if (!empty($cloned_node->uid)) {

Beside this, I think it could be RTBC.

pwolanin’s picture

I meant "temporary" in that this is a work-around for the problem of having a call to drupal_set_message in an #after_build element in a #multistep form.

A "permanent" fix mix involve a change to form.inc such that during multistep form processing the first set of messages from drupal_set_message() are erased, or some such.

eaton’s picture

However, I think this is a bug somewhere in the multistep form handling. It's only because poll happened not to show a separate teaser and body that his does not occur usually. If you add $form['#multistep'] = TRUE; to *any* node type (e.g. forum) you'll see the same behavior.

This is due to the fact that in multistep mode, two copies of the form ARE being built. The first is an internal copy for validation purposes, using JUST the fields that were present in the 'original' copy of the form. The second is for display if more options are added based on the input from the first copy (in the case of poll.module, it's because more options may need to be added).

pwolanin’s picture

@Eaton - yes, thanks, I did manage to understand this (more-or-less) from looking at form.inc, but maybe I wasn't clear in my description. As I mentioned, this causes ugliness with the node form in particular, since the node preview (as part of the #after_build) calls drupal_set_message(). I opened a new issue for this problem with form building: http://drupal.org/node/106634

beginner’s picture

Status: Needs review » Needs work

see #13.

pwolanin’s picture

Version: 5.x-dev » 6.x-dev
Status: Needs work » Needs review
FileSize
3.5 KB

Ok, patch re-worked since there were some other patch to this function in the meantime. Apples cleanly to both 5.x and HEAD (6.x).

note- I think this is really important for 2 reasons:

1) the node is cloned at the start, so the manipulation of values cannot affect the original object.

2) running the node through the submit hooks allows additional processing of form values by modules like event so the preview becomes meaningful.

beginner’s picture

Status: Needs review » Needs work

ok.

I think this comment can be removed, because it seems out of context where it is. It looks like a remnant of much older code.

      // The use of isset() is mandatory in the context of user IDs, because
      // user ID 0 denotes the anonymous user.

On the other hand, there is no reason to remove that comment:

     // Load the user's name when needed:

Otherwise, the code is ok, and the patch should be RTBC soon.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
3.54 KB

This patch is only different from the last WRT the code comments, per beginner's suggestions.

beginner’s picture

Status: Needs review » Reviewed & tested by the community

Ok.

pwolanin’s picture

FileSize
1.63 KB

for extra credit, here's a patch for the HEAD developer docs too.

Steven’s picture

Status: Reviewed & tested by the community » Needs work

We can do the node_submit() code in node_preview(), but invoking nodeapi submit during preview is out of the question. Submit is clearly an operation for saving data.

It sounds like the real solution is a nodeapi preview hook, where a module can do the same things that would happen by submitting it and retrieving it back from the database.

pwolanin’s picture

@Steven- I disagree- the submit hook is the step before saving the data- the step to convert the form values into the form that's needed for saving (and thus probably for display). The API docs clearly say that the data should be written out at hook_update/hook_insert or hook_nodeapi with the insert/update op.

Are there any modules that save data at the submit stage? Certainly none of the core modules do.

beginner’s picture

Status: Needs work » Needs review

+1 for the patch as it is. It is simple enough.

During preview, no data is actually saved.
What Steve actually proposes is adding some cruft to the code, by adding yet another hook or $op, while the existing API already does the job perfectly.

Why add yet more code??

beginner’s picture

btw, this issue seems related:

New attachments not shown in node preview http://drupal.org/node/68690

Does the current patch fix it?

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

@Steven - you are mistaken. We added the submit hook as a 'pre-save' hook. It's function is to *prepare* the node object for saving. At this point, modules know that the node has pased validation and can alter $node as needed. But they are not too perform any actions. Actions are to be performed in the insert/update operations. Anwyay, the nid isn't even knows during submit hook. What could modules do there?

If you still doubt, I will find the issue where JVD introduced this hook and we all argued extensively about what this hook meant and its proper use is. I assure you that this patch properly invokes the submit hooks and no unwanted side effects will arise.

pwolanin’s picture

patch re-rolled for HEAD. Should apply with offset to 5.x too.

pwolanin’s picture

bump - need to re-roll patch

Dries’s picture

Status: Reviewed & tested by the community » Needs work

Needs to be re-rolled, assuming it is still a relevant patch.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
3.6 KB

I think it is still quite relevant. Here's a re-roll of the node module part. I'll need to come up with a test case.

eaton’s picture

There are several changes to how node previews work in D5 after the FAPI3 patch. Those fell through the cracks when the docs were being written (my apologies) and I will be updating them shortly. Basically, adding #submit[] = 'my_submit_handler' to the node form, rather than implementing nodeapi op submit and hook_submit, are now sufficient. Using that hook to modify the values should now work.

I haven't looked closely at this patch yet but it seems relevant...

pwolanin’s picture

@Eaton - while I appreciate you taking a look, this is the a node type hook_submit (i.e. typehook_submit or what should have been called hook_fixup), and should be unrelated to FAPI 3 changes.

Anyone willing to rethink the opposition to distinguishing different type of hooks in the developer docs? http://drupal.org/node/107225

pwolanin’s picture

oh, wait- maybe I misunderstood you.

The point of this patch is that (for D5 at least) every module gets a chance to alter the node via hook_submit or hook_nodeapi('submit') before it goes to hook_save or hook_nodeapi('save'). The same pre-saving (fixup) step should happen before the preview also.

Clearly if the process is different now, that's important. However, this seems really confusing for new developers if you have to form_alter the node form and write a separate submit function when before you could just put a couple lines in hook_nodeapi()?

eaton’s picture

Good question.

With the changes in FAPI3, #submit handlers for the node form in particular get fired in BOTH preview AND submit. chx worked to integrate some of the core ideas in this patch to the porting of the node form, so I think that some of the fundamental concerns are no longer necessary.

hook_submit and nodeapi op submit have never really been about altering a node -- they have been about altering $form_values so that the node_save() function can understand it. Keeping it in the form handling, rather than a separate set of hooks, seems like the cleanest solution since any module that needs this functionality is already messing with the form by definition.

Am I missing something, or..?

pwolanin’s picture

@Eaton - that's starting to make sense. However, I think there are still some problems with node_preview- for example, the access checks there are (I think) totally unneeded, and it still seems to me that all the operations should be done on a cloned copy of the object.

Is hook_submit gone or just discouraged in D6?

eaton’s picture

Good points.

Actually, I think that if this patch WERE to go through, it could focus on removing the access checks and ensuring that all the modifications to the node occurred to $form_state['node'], or something like that, rather than a by-ref copy of $form_values. Those two things would really clean things up quite a bit.

hook_submit and nodeapi op submit are no longer called during in node.module's submit function -- which means that unless someone else was out there using them for crazy things, they're gone. I'm writing up the docs on the change, though it might make sense for me to wait until this patch is finished since your work will likely tweak it...?

catch’s picture

Status: Needs review » Needs work

This no longer applies, and I've a feeling it's been fixed but don't know where that issue was.

eaton’s picture

Status: Needs work » Fixed

This should indeed be fixed by D6's new node review system. I've run it through some basic tests and believe the patch is no longer necessary. If anyone feels otherwise, please post an explanation of the use case that isn't covered by D6's new mechanisms. (i.e., add $form['#submit'] handlers that process the values for previewing).

Anonymous’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.