This is a prerequisite for #504666: Make comments fieldable, thus marking as critical (got separated in its own patch for kitten confort)

The code handling comments previews is still D5 style: added on #after_build when $_POST['op'] == 'Preview'.
Yuck + incompatible with #ahah additions on comment forms..
More generally, edit forms for fieldable entities need to support multi-step rebuilding - because that's exactly what Fiald API's 'add more values' button does: make the form multistep...

This patch moves comment preview to use the exact same workflow than node previews:
- Preview button is a '#type' => 'submit' FAPI element with a 'comment_form_build_preview' submit callback, instead of a '#type' => 'button' + #after_build.
- comment_form() builds a multistep form, merging the original edited comment with previous form values saved in $form_state['comment']
- comment_submit() builds a comment object from submitted form values.
- comment_form_submit_build_comment() calls comment_submit() and prepares for a form rebuild.
- comment_preview() generates the output for a comment preview.
- comment_form_build_preview() calls comment_preview() and stores the output in $form_state['comment_preview'].
- comment_form() outputs $form_state['comment_preview'] if it finds any.

New or reworked functions that directly map to node.module:
comment_submit() / node_submit()
comment_form_submit_build_comment() / node_form_submit_build_node()
comment_form_build_preview() / node_form_build_preview()
comment_preview() / node_preview()

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kika’s picture

Subscribing

catch’s picture

Status: Needs review » Reviewed & tested by the community

Read through this, all the code style makes sense and we already verified it works for multi-value fields in the comments as fields issue, existing tests pass as well, so RTBC.

kika’s picture

Status: Reviewed & tested by the community » Needs review
catch’s picture

Status: Needs review » Reviewed & tested by the community

I think kika cross-posted, reset status if not.

kika’s picture

sorry for cross-post. Go!

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

yched’s picture

Category: task » bug
Priority: Critical » Normal
Status: Fixed » Needs review
FileSize
1.19 KB

Oops, $edit was removed from comment_preview() but it seems a couple instances failed the rename.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community
Dries’s picture

Should we write some extra tests for this? That should not have happened.

yched’s picture

Er, honestly, I've been trying to add those tests for the last 2 hours, and it beats me :-(

tic2000’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
3.13 KB

The patch in #7 with a test for comments preview. I hope this is the thing that needed a test. If not, tell me exactly what do you want tested and I'll try to write one.

Dries’s picture

Well, the tests look good but they don't actually demonstrate the bug. Add the test to Drupal, but don't apply the fix for comment.module and you'll see that the tests pass. So, while this is a great start, it doesn't seem sufficient.

tic2000’s picture

Then my next question is how the bug manifest itself?
I tried previewing a comment and then saving it without the patch in #7 and everything works as expected.

But what I've noticed is that posting comments without approval permission is bugged. At least for anonymous, didn't try with an user other than user 1.

yched’s picture

OK, I had been trying to integrate those tests in the existing 'Test Comment interface' tests, but this approach is probably simpler, thanks tic2000 !

The bug actually shows when a user with 'administer comments' permission *edits an existing comment* and
- sets the 'author' to a different username than himself
- or sets the date to a different date than current
(those are under the collapsed 'Administration' fieldset on top of the edit comment form)
The preview displays the name of the currently logged in user and a date of "now"

True that comment approval seems broken right now, BTW. I enabled 'post comment without approval for anon users', and anon comment are still unpublished. + they remain unpublished even after I click the 'approve' link at the bottom of the comment as an admin.
That's for another thread, though.

tic2000’s picture

OK here is a patch just for the tests.

Easier to test. Apply this patch, run comment preview test. 2 fails. Apply patch in #7, run the same test. 0 fails.

Status: Needs review » Needs work

The last submitted patch failed testing.

tic2000’s picture

Status: Needs work » Needs review

I set this back to needs review. The tests are supposed to reveal exactly those 2 fails :)

yched’s picture

Status: Reviewed & tested by the community » Needs review

tic2000: I'd suggest you roll a patch with tests from #15 and fixes from #7. That way we can have a patch with green tests at the bottom of the thread, will be easier to commit.

tic2000’s picture

I think you're right.

yched’s picture

Status: Needs review » Reviewed & tested by the community

Thanks !

webchick’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
4.85 KB

Re-uploading this to get the test bot to behave.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Cool! Tests now catch the bug. I made some minor formatting adjustments and committed to HEAD.

2createwdrupal’s picture

Version: 7.x-dev » 6.13

I know you are all talking about Drupal 7, but wouldn't someone look at this:

http://drupal.org/node/543302

I am getting an "error in line 992" of Drupal's Comment module. Comment exist, were showing up, but now they are not.

Any help appreciated.

yched’s picture

Version: 6.13 » 7.x-dev

@2createwdrupal : do *not* do this, this is called "hijacking a thread on a completely unrelated issue" and very very much frowned upon ;-)

2createwdrupal’s picture

Sorry.

Although, I did not think it was completely unrelated. Apologies.

Status: Fixed » Closed (fixed)

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