Hey, funny thing, I needed a module like this, couldn't find it, made one and actually ended up naming it FlatComments, then when I searched for that I found this lol. Anyway, your code helped me realize a lot, especially that the $form['pid']['#value'] can be set to NULL, I was setting it to $form['nid']['#value'].
One thing I noticed when I was working on mine was, when you set the $form['pid']['#value'] to anything within form_alter it wont preview the correct post. I think it's because it uses $form['pid']['#value'] to get the post it will preview and hitting preview alters the form changing the pid.
In my case I switched $form['pid']['#value'] to $form['nid']['#value'] and if you reply to any post it shows the correct post you're replying to, but then preview it, now it shows the First Post not the post you were replying to because $form['pid']['#value'] got changed. I was wondering if the same thing was happening in yours because $form['pid']['#value'] is changed in form_alter?
If it does I think I figured out a solution, this only works for comments in forums right now but it could easily go in your module and work on any node, I just hadn't gotten that far, then I found this so now I just need to find a tutorial on writing patches. :X
/**
* Alter $a1['pid']['#value'] if FlatComments is on
*/
function flatcomments_comment($a1, $op) {
if ($op == 'validate') {
if (variable_get('flatcomments_enabled',0)) {
global $form_values;
if($form_values['nid'] && is_numeric($form_values['nid'])) {
$parent_node = node_load($form_values['nid']);
if ($parent_node->type == "forum") $form_values['pid'] = NULL;
}
}
}
}
This used hook_comment('validate') to do the check after the form is finally submitted and right before it goes into the database I think. I was also wondering if this was a good way to do this, I struggled for a while with hook_submit, hook_nodeapi, and hook_form_alter but this is what worked out in the end and gave me the correct preview post with FlatComments so they could be deleted individually.
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | flatcomments-preview-fix2.patch | 2.38 KB | JirkaRybka |
| #3 | flatcomments-preview-fix.patch | 2.44 KB | JirkaRybka |
Comments
Comment #1
WhatTheFawk commentedOops forgot
Thats a bit easier on the eyes :D
Comment #2
dragonwize commentedThis is actually caused because the reply to is specified in the link (comment/reply/35/2) instead of what is being used in form alter. Then the form entry is used on preview.
The link format is comment/reply/[nid]/[cid]. You can get the correct reply to shown by going to comment/reply/[nid] instead.
This will be fixed in the next version.
I am either going to remove the specific reply to links or mod their URLs or a combination depending. I have fully decided yet.
Comment #3
JirkaRybka commentedI have a patch for this one.
My solution is to leave the from workflow unchanged (no change to pid early, through hook_form_alter() ), so that all previews and stuff work as usual. But we set explicitly a form submit handler (therefore overriding the default one in comment.module) to 'flatcomments_comment_form_submit()' - which is just a wrapper around the original comment_form_submit(), doing the alteration just before the original handler is invoked, and so the comment saved to the database. The advantage is, that now the pid-changing code only fires after the form is finally submitted, so we won't break previews, nor anything else in the form itself.
I also considered the option to add our submit handler alongside to the original one, to be more in line with possible further alterations from other modules, but it's not worth the trouble. It would require some array_unshift() gymnastics to ensure our new handler fires before anything else, and in the end, we'll end up with adding the original comment_form_submit() to the array explicitly too, because it no more fires as a default fallback. So in the end, the other module would find the #submit array changed all the same, and if relying on the default being overridden by anything what's added, it'll break all the same. But this is all just theory, I guess, as little-to-none modules are going to alter submit on a comment form anyway.
With the patch, preview shows the post we're replying to, even where flatcomments is active, even after preview. Actually, I can use the individual "reply" links on comments to choose, which one I want to see on the comment form, to help me with typing a reply, so the links make a bit of sense again.
Comment #4
dragonwize commentedI use array_unshift all the time to add handlers to the front of the array without incident. You do not explicitly have to add the default handler. I do not follow your logic in what can go wrong there. In fact overriding it is a much worse idea IMO, that can cause issues with other modules.
Example: panels has a wrap around comment form and I have had to adjust my Better Formats module accordingly. In fact, I need to add the same integration feature into flatcomments.
So now every module that does something with the comment form has to have support for that module. The same would happen for us. Which is something I would like to avoid at all costs.
Feel free to convince me otherwise, but my vote is for array_unshift.
Comment #5
JirkaRybka commentedI was under impression, that default handler is not added, if anything is explicitly defined in #submit, so all the other ideas derived from that. Only now I see that I was probably wrong in how I understood that. I must have done something wrong while examining the workflow (which I did, certainly), so that when I inserted a debug code, I didn't see the default handler being there. Weird.
If the default handler is in the '#submit' array at the time of hook_form_alter(), then everything is much, much simpler, and we can/should use that array_unshift() indeed. Feel free to change that; otherwise I'll be back some time next week (leaving for a few days now). I really don't know what's a common practice here, as I've never seen a module altering the comment form this way (which is not to say they don't exist, just that I didn't come across one yet).
Comment #6
JirkaRybka commentedSo let's say this "needs work" ;-) As long as the move from hook_form_alter() to form submit handler is agreed on, I'm sure we can sort this out soon.
Comment #7
JirkaRybka commentedPatch re-rolled with the submit handler added through array_unshift() instead of creating a wrapper. Works fine for me. No other changes.
Comment #8
dragonwize commentedCommitted.