Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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);
}
Comment | File | Size | Author |
---|---|---|---|
#31 | submit_node_preview_60_7.patch | 3.6 KB | pwolanin |
#28 | submit_node_preview_60_6.patch | 3.54 KB | pwolanin |
#22 | submit_docs_1.diff | 1.63 KB | pwolanin |
#20 | submit_node_preview_50_5.diff | 3.54 KB | pwolanin |
#18 | submit_node_preview_50_4.diff | 3.5 KB | pwolanin |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedTested 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
Comment #2
pwolanin CreditAttribution: pwolanin commentedPatch 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
Comment #3
pwolanin CreditAttribution: pwolanin commentedThis is really a simple patch...
Comment #4
pwolanin CreditAttribution: pwolanin commentedbetter title
Comment #5
drummThere 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.
Comment #6
pwolanin CreditAttribution: pwolanin commentedhmmm, yes- I think maybe instead of
node_submit()
it may be sufficient to just add in:Comment #7
pwolanin CreditAttribution: pwolanin commentedOk, 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)
Comment #8
pwolanin CreditAttribution: pwolanin commentedsame patch for 4.7.x
Comment #9
Dries CreditAttribution: Dries commentedMmm, doesn't the submit hook invoke database queries? We don't want to store the node when still previewing it.
Comment #10
pwolanin CreditAttribution: pwolanin commented@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
Comment #11
pwolanin CreditAttribution: pwolanin commentedHmmm, 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.Comment #12
pwolanin CreditAttribution: pwolanin commentedOk, Attached patch includes a temporary fix for poll module for 5.x
Comment #13
beginner CreditAttribution: beginner commentedI 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:
you could simply use the following, no?
Beside this, I think it could be RTBC.
Comment #14
pwolanin CreditAttribution: pwolanin commentedI 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.
Comment #15
eaton CreditAttribution: eaton commentedThis 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).
Comment #16
pwolanin CreditAttribution: pwolanin commented@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
Comment #17
beginner CreditAttribution: beginner commentedsee #13.
Comment #18
pwolanin CreditAttribution: pwolanin commentedOk, 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.
Comment #19
beginner CreditAttribution: beginner commentedok.
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.
On the other hand, there is no reason to remove that comment:
Otherwise, the code is ok, and the patch should be RTBC soon.
Comment #20
pwolanin CreditAttribution: pwolanin commentedThis patch is only different from the last WRT the code comments, per beginner's suggestions.
Comment #21
beginner CreditAttribution: beginner commentedOk.
Comment #22
pwolanin CreditAttribution: pwolanin commentedfor extra credit, here's a patch for the HEAD developer docs too.
Comment #23
Steven CreditAttribution: Steven commentedWe 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.
Comment #24
pwolanin CreditAttribution: pwolanin commented@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.
Comment #25
beginner CreditAttribution: beginner commented+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??
Comment #26
beginner CreditAttribution: beginner commentedbtw, this issue seems related:
New attachments not shown in node preview http://drupal.org/node/68690
Does the current patch fix it?
Comment #27
moshe weitzman CreditAttribution: moshe weitzman commented@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.
Comment #28
pwolanin CreditAttribution: pwolanin commentedpatch re-rolled for HEAD. Should apply with offset to 5.x too.
Comment #29
pwolanin CreditAttribution: pwolanin commentedbump - need to re-roll patch
Comment #30
Dries CreditAttribution: Dries commentedNeeds to be re-rolled, assuming it is still a relevant patch.
Comment #31
pwolanin CreditAttribution: pwolanin commentedI 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.
Comment #32
eaton CreditAttribution: eaton commentedThere 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...
Comment #33
pwolanin CreditAttribution: pwolanin commented@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
Comment #34
pwolanin CreditAttribution: pwolanin commentedoh, 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()?
Comment #35
eaton CreditAttribution: eaton commentedGood 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..?
Comment #36
pwolanin CreditAttribution: pwolanin commented@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?
Comment #37
eaton CreditAttribution: eaton commentedGood 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...?
Comment #38
catchThis no longer applies, and I've a feeling it's been fixed but don't know where that issue was.
Comment #39
eaton CreditAttribution: eaton commentedThis 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).
Comment #40
(not verified) CreditAttribution: commentedAutomatically closed -- issue fixed for two weeks with no activity.