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()
Comment | File | Size | Author |
---|---|---|---|
#21 | 524652-19-comment-form-followup-and-test.patch | 4.85 KB | webchick |
#19 | 524652-19-comment-form-followup-and-test.patch | 4.85 KB | tic2000 |
#15 | comment-form-followup-just-tests.patch | 3.67 KB | tic2000 |
#11 | comment-form-followup-and-test.patch | 3.13 KB | tic2000 |
#7 | comment-form-followup.patch | 1.19 KB | yched |
Comments
Comment #1
kika CreditAttribution: kika commentedSubscribing
Comment #2
catchRead 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.
Comment #3
kika CreditAttribution: kika commentedSomewhat related issue? #275368: Allow disabling comment preview + unify with node preview settings
Comment #4
catchI think kika cross-posted, reset status if not.
Comment #5
kika CreditAttribution: kika commentedsorry for cross-post. Go!
Comment #6
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks!
Comment #7
yched CreditAttribution: yched commentedOops, $edit was removed from comment_preview() but it seems a couple instances failed the rename.
Comment #8
moshe weitzman CreditAttribution: moshe weitzman commentedComment #9
Dries CreditAttribution: Dries commentedShould we write some extra tests for this? That should not have happened.
Comment #10
yched CreditAttribution: yched commentedEr, honestly, I've been trying to add those tests for the last 2 hours, and it beats me :-(
Comment #11
tic2000 CreditAttribution: tic2000 commentedThe 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.
Comment #12
Dries CreditAttribution: Dries commentedWell, 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.
Comment #13
tic2000 CreditAttribution: tic2000 commentedThen 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.
Comment #14
yched CreditAttribution: yched commentedOK, 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.
Comment #15
tic2000 CreditAttribution: tic2000 commentedOK 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.
Comment #17
tic2000 CreditAttribution: tic2000 commentedI set this back to needs review. The tests are supposed to reveal exactly those 2 fails :)
Comment #18
yched CreditAttribution: yched commentedtic2000: 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.
Comment #19
tic2000 CreditAttribution: tic2000 commentedI think you're right.
Comment #20
yched CreditAttribution: yched commentedThanks !
Comment #21
webchickRe-uploading this to get the test bot to behave.
Comment #22
webchickCool! Tests now catch the bug. I made some minor formatting adjustments and committed to HEAD.
Comment #23
2createwdrupal CreditAttribution: 2createwdrupal commentedI 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.
Comment #24
yched CreditAttribution: yched commented@2createwdrupal : do *not* do this, this is called "hijacking a thread on a completely unrelated issue" and very very much frowned upon ;-)
Comment #25
2createwdrupal CreditAttribution: 2createwdrupal commentedSorry.
Although, I did not think it was completely unrelated. Apologies.