Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
I think there's a problem in comment.module. Form validation can fail for no reason when replying to certain node types, but there's never a problem when replying to another comment. I've posted what I know here: http://drupal.org/node/63554
Comment | File | Size | Author |
---|---|---|---|
#51 | drupal-HEAD_4.patch | 679 bytes | sun |
#46 | comment_reply_suffix.d6.patch | 678 bytes | dww |
#44 | comment_reply_suffix.patch | 853 bytes | hunmonk |
#32 | multi_form_validation.txt | 1.24 KB | chx |
#29 | multi_form_comment_validation_47_0.txt | 1.46 KB | Heine |
Comments
Comment #1
mjolley@buy-hot.com CreditAttribution: mjolley@buy-hot.com commentedI guess I should summarize.
One theory is related to the number of forms on the page. I think it's an invalid reference to the edit buffer, possibly caused by unintended recursion via callbacks. I'm pretty sure that the content of the database is not the issue so I'm thinking it's the stack. This appears to be "read only" corruption in that behavior is predictable, and only the form validator is affected. This bug screams "bad stack frame."
Comment #2
kmillecam CreditAttribution: kmillecam commentedI'm running 4.7.2 and am still seeing this validation error.
I've confirmed that it only occurs when commenting on a node, not when commenting on another comment.
The error I used to get with 4.7.0 included a phrase about "the comment field is empty". This phrase no longer appears.
The current error reads: "Validation error, please try again. If this error persists, please contact the site administrator."
Kevin
Comment #3
kmillecam CreditAttribution: kmillecam commentedThis is still a problem on my systems.
I've found the code that generates the error but still don't know the cause. Has anyone else made any progress?
From form.inc, line 169:
Comment #4
Dries CreditAttribution: Dries commentedWhen looking at the form submission form, check whether there is a hidden token field. Is there is no such field, something is wrong. Can we inspect your form somewhere?
Do you happen to use the backport module or the forms_api_backport.module? If so, try disabling it.
Comment #5
Heine CreditAttribution: Heine commentedThis happens with polls. When previewing a comment on a poll the $form['#token'] is for example 'comment3' but $form_values contains values from the poll form upon comment preview:
Comment #6
kmillecam CreditAttribution: kmillecam commented"Can we inspect your form somewhere?" ... sure http://dev1.webwiseone.com Try to post a comment.
"This happens with polls." ... and, it appears, with other modules that display extra forms on a page (like nodevote).
Comment #7
Heine CreditAttribution: Heine commentedCorrect, I think that form_values should be keyed on form_id, anyway not just overwritten.
Comment #8
kmillecam CreditAttribution: kmillecam commentedThis behavior can be reproduced on a clean 4.7.2 install.
1) create a poll with comment capability
2) comment on poll
Kevin
Comment #9
kmillecam CreditAttribution: kmillecam commentedSome good solid research regarding this problem is documented here: http://drupal.org/node/70509
Comment #10
Heine CreditAttribution: Heine commentedHere's a callstack
The second call to drupal_get_form obliterates earlier $form_values by setting it to array().
Comment #11
kmillecam CreditAttribution: kmillecam commentedA solution to this problem is posted here: http://drupal.org/node/70509#comment-132162
Comment #12
Heine CreditAttribution: Heine commentedI can no longer preview comments when trying to use:
They get submitted directly.
Please keep the discussion in this issue. I don't care much about money, but I want this fixed properly.
Comment #13
carlmcdade CreditAttribution: carlmcdade commentedTry and change your session id . The key may be set already and when you refresh to test the post clears automatically.
Comment #14
webchickThis appears to be tied to the 'vote on polls' permission.
When I first tried this with just a dummy uid 2 I too wasn't exhibiting this problem. After checking all permissions for authenticated users I was again. I unchecked "vote on polls" and the problem went away.
Comment #15
carlmcdade CreditAttribution: carlmcdade commentedI just did another test on a fresh install of 4.7.2 and preview works fine for both anonymous and authorised users.
Comment #16
carlmcdade CreditAttribution: carlmcdade commentedYes, adding in vote on polls makes a direct comment without a preview. Another problem I think. Checking on a possible solution.
Comment #17
webchickYes, you won't be able to reproduce this on a fresh install. You need to give anonymous/authenticated users "vote on polls" permissions, which they won't have by defauilt. Then you'll see this behaviour.
Comment #18
Heine CreditAttribution: Heine commentedChx had the idea to put the contents of node_view(node_load($edit['nid')) already in the comment forms array, for example as type markup, then render that, so you avoid the two nested drupal_get_form calls.
I'll try this route.
Comment #19
Heine CreditAttribution: Heine commentedBetter suiting title
Comment #20
carlmcdade CreditAttribution: carlmcdade commentedThe problem with loading it up that way is that it does not solve the problem of actually needing to call drupal_get_form multiple times for other usages. This is where my checkbox comparisons get overwritten. There has to be a more robust solution.
Comment #21
Heine CreditAttribution: Heine commentedAnother approach would be a keyed array: $form_values[$form_id]
1 - this would break a lot
2 - what happens when a form builds another instance of itself while in drupal_get_form?
Comment #22
Heine CreditAttribution: Heine commentedSaving the globals in a static variable proved easy enough.
However, the subsequent rendering of nested forms is not valid html. Attached patch also renders the node as suffix of the form to prevent this from happening.
Note: I've tested most comment scenarios. The patch against 4.7 CVS requires thorough review however.
Comment #23
Heine CreditAttribution: Heine commentedAnd a patch against HEAD.
Comment #24
Heine CreditAttribution: Heine commentedThanks to chx a more elegant & general patch against HEAD.
Comment #25
carlmcdade CreditAttribution: carlmcdade commentedI think a better solution would be to not allow the poll to validate. This way only the cause is influenced and does not get in the way of proper usage of globals.
$form = form_builder($form_id, $form);
if (!empty($_POST['edit']) && (($_POST['edit']['form_id'] == $form_id) || ($_POST['edit']['form_id'] == $callback))) {
if($_POST['edit']['form_id'] == 'poll_view_voting'){
exit;
}
else{
drupal_validate_form($form_id, $form, $callback);
}
This works without causing a direct post of a comment.
Comment #26
carlmcdade CreditAttribution: carlmcdade commentedscratch that. Only works for anonymous.
Comment #27
carlmcdade CreditAttribution: carlmcdade commentedThis has gotten very confusing as there is no consistent behaviour in the forms. When anonymous is previewed it is previewed with each press of the preview button. When logged in the preview button only works once then redirects back to the default form view. This makes it hard to know what is design and what bug.
Also the question arises why does an untouched form.inc work perfectly with comments and polls for anonymous but not authorised users?
Comment #28
Heine CreditAttribution: Heine commentedApparantly poll is not the only module node type with commenting problems.
Is this behaviour after the patch in #24?
Comment #29
Heine CreditAttribution: Heine commentedAnd here's a patch against the 4.7 branch for testing (#24 contains the patch against HEAD)
Comment #30
carlmcdade CreditAttribution: carlmcdade commentedI took sometime to think about the flow of all this. What I came to was that when a user is previewing a comment they do not need to see the poll form. They are in a process that they have already decided upon. They either have voted or if they do want to vote will do so after they make a comment. So the vote form is not needed in the comment preview mode.
changing line 309 in poll.module
I like this because it can easily become a coding convention for other modules that use comment forms.
Comment #31
webchick@HiveMinds, that's not an acceptable fix... First, because it breaks default behaviour. But secondly, because this problem will replicate itself anytime a form is displayed in a node.
Heine's solution looks more on target, I'll try testing later today.
Comment #32
chx CreditAttribution: chx commentedI have modified a very little bit Heine's version so that form_values are available during rendering. This is for HEAD. But I suspect it would apply against 4.7, too...
Comment #33
carlmcdade CreditAttribution: carlmcdade commentedThe last patch does work and supresses the error. Which is good for users.
Sadly it did not solve the problem of needing to render groups of checkboxes or rather multiple calls to drupal_get_form. I no longer get errors but you can't call the function twice without it overwritting and only showing the last instance of the call. And it still runs each form through validation which I don't want.
Comment #34
Heine CreditAttribution: Heine commentedThanks for reviewing the patch.
It was not intended to do so. Unless you can show how this is related to the fapi & comment validation bugs, I suggest you open a new issue and include some examples of the problem.
Comment #35
drummTested on 4.7 while logged in and using a module I wrote (karma) which adds a form to the node.
I have no clue what Hiveminds Magazine is talking about since they haven't provided a proper patch or complete description of the problem they are having.
Comment #36
mjolley@buy-hot.com CreditAttribution: mjolley@buy-hot.com commentedI think he's saying that the underlying cause of the problem is that multiple forms are sharing a single input buffer. This makes sense from what I was seeing. The error being displayed on the screen was correct because the comment buffer was empty -- It had been overwritten.
Comment #37
drummCommitted to HEAD.
Comment #38
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedalso to 4.7
Comment #39
Dries CreditAttribution: Dries commentedLooks like a hack to me...
Comment #40
(not verified) CreditAttribution: commentedComment #41
mercmobily CreditAttribution: mercmobily commentedHi,
So... anonymous (unverified) closed the issue straight after Dries writing "It looks like a hack to me".
Is this issue *actually* closed? I am struggling with it a _lot_ in Drupal 4.7.2. I run Free Software Magazine, which is a major web site, and I am a little disturbed to see that anonymous closed an issue straight after the "big boss's" complaint.
We would like to enable "votenode", but now I am scared.
Comment #42
StevenPatzIssues are closed automatically 2 weeks after being marked fixed.
Comment #43
dwwDries is totally right in #39 -- this is a hack, and this is now causing much pain and grief for issue followups as comments.
comment_form_add_preview() is trying to be sneaky about displaying the original node at the bottom of the preview form when you're replying to the original post (instead of replying to a specific comment), with the following code:
The idea is that if the original $node itself contains a form, you can't have nested
<form>
HTML, so we stick the original node in the #suffix so that it comes after the</form>
of our preview form. The problem is that$form['#suffix']
is already set to something at this point in many cases. For example, in my testing, printing out the value of the form's #suffix at that point yielded:"</div></div></div>"
. Needless to say, once this was clobbered, all sorts of CSS went haywire, blocks were showing up at the bottom of the form, etc, etc.Core should never be destroying form values like this. If we could assume that
$form['#suffix']
was itself an array that we could non-destructively append our own stuff to, that'd be one thing, but that's not how #suffix works. :(Furthermore, in the project_issue case, it'd be nice to undo this behavior entirely, since our previewing logic is already printing the original node. I've tried various approaches and none of them are fully working in both cases yet. I've got to run right now, but I promised hunmonk I'd write up my findings before I left so that we don't duplicate effort. ;) Point being, this is definitely still broken, but I think I'm fairly close to a solution.
Worst case is we check if (!empty(#suffix)) and append instead of clobber in that case. project_issue would still have trouble trying to unset this node_view(), but at least we wouldn't be destroying form values anymore.
Ideally, #suffix could be an array, and comment_form_add_preview() would put the node_view() in a well-known location in that array, so that other modules could use #after_build to unset it if they wanted...
Comment #44
hunmonk CreditAttribution: hunmonk commentedsimple implementation of our fallback strategy: not clobbering existing #suffix values. i'm not seeing a better solution here so far. AFAIK we don't have the complex data structures in place to solve this more elegantly -- #suffix is the $node->body of drupal 4.6 days.
this fix would be enough to solve our project_issue challenges for issue followups -- we can just slap on the comments after the node output in #pre_render. not pretty, but workable for now until we have a better rendering mechanism for comments.
Comment #45
dwwThat patch is RTBC for D5, but doesn't apply to D6.
Comment #46
dwwHere's a version for D6.
Comment #47
chx CreditAttribution: chx commentedFix D5. Let's contemplate D6 a bit more.
Comment #48
dwwI believe chx meant RTBC. ;)
Comment #49
dwwJust to be clear: I tested the two worst possible cases: previewing a comment on a poll, and the IFAC issue followup preview. Both actually work with this patch. Of course, a simple comment on a regular, non-form post also works. Just wanted to let everyone know this isn't a blind RTBC...
Comment #50
drummCommitted to 5.x
Comment #51
sunSwitched empty() to !empty(), didn't test it.
Comment #52
hunmonk CreditAttribution: hunmonk commented@sun: not sure what that buys us, and i think not using not is less confusing... ;)
Comment #53
sunSorry, I just thought it would be more consistent with the rest of core. IMHO, we should try to (somehow) protect core committers from having to remark things like that.
Comment #54
hunmonk CreditAttribution: hunmonk commentedif we're contemplating a different solution, then this probably needs work.
Comment #55
chx CreditAttribution: chx commented*sniff* so many things are possible here, but none of them are elegant. comment module needs a rewrite... i think i have my task for seven.
Comment #56
hunmonk CreditAttribution: hunmonk commentedfor code consistency (with the patch that was applied to D5), we should probably use the patch in #46.
Comment #57
Gábor HojtsyIndeed, committed #46 for consistency. Thanks.
Comment #58
(not verified) CreditAttribution: commented