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.
Found in #652834: Field formatters as render arrays
When submitting a new comment, comment_reply() does a node_build() for nothing, user is redirected to node/%nid before the array is rendered.
Comment | File | Size | Author |
---|---|---|---|
#23 | comment-comment_reply-structure-654854-23.patch | 5.08 KB | cburschka |
#21 | comment-node_build-654854-21.patch | 4.47 KB | cburschka |
#19 | comment-node_build-654854-19.patch | 2.08 KB | cburschka |
#13 | comment-node_build-654854-13.patch | 1.93 KB | cburschka |
#7 | comment-node_build-654854-7.patch | 1.88 KB | cburschka |
Comments
Comment #1
marcingy CreditAttribution: marcingy commentedThis removes the build statement for a node
Comment #2
moshe weitzman CreditAttribution: moshe weitzman commentedComment #3
webchickYay for less code. :)
Committed to HEAD.
Comment #4
cburschkaAfter this patch, NodeTitleTestCase no longer passes.
HEAD at 2009-12-25 12:16:00 (before this commit):
HEAD at 2009-12-25 12:20:00 (after this commit):
The test clients are out of service since last night, showing the same test failure as above.
Comment #5
cburschkaThe problem is not limited to the test case -
comment/reply/$nid
no longer shows the node being replied to, which appears to be a functional problem.Comment #6
cburschkaIn fact, this patch looks stranger the more I look at it.
Starting with the premise:
That's not true. The code is this:
If the node has open comments, and the user has access to post comments, then the second branch runs, and no drupal_goto() is ever called.
This node_build simply cannot be removed. It's possible that the code can be cleaned up in a different way, but not like this... :(
Comment #7
cburschkaI was going to suggest a rollback, but then saw how the conditions might be better arranged after all.
Here's a new patch that results in correct display and passes the test.
There's a bit of a WTF involved in the access check order: "Can this user post a comment? Yes? If so, can he access content at all? No? Then show him a comment form, but not the node he's replying to."
But I suppose if the permissions are set sensibly, this would not happen.
Comment #8
cburschkaComment #9
webchickMargh! My bad. :( That'll teach me for thinking that "simple" patches can't break the test bot. :P
Arancaytar's fix is more involved than I'm comfortable with committing without a proper review, and I really want the test bot working again. So instead I've reverted #1 for now. This is going to require a re-roll of this patch, I'm pretty sure.
Comment #10
cburschkaGood idea. I think we should take more time looking at those permissions, anyhow. Good to see head working again. :)
Comment #11
int CreditAttribution: int commentedThe patch it's reverted, so don't faill the tests
Comment #12
cburschkaComment #13
cburschkaHere's a re-rolled patch.
Comment #16
yched CreditAttribution: yched commentedLooks sensible to me. Let's see what the bot says.
Comment #18
cburschkaOops, yes. I reproduced these failures locally last night too. There's a mistake in there.
Comment #19
cburschkaAh. It's caused by the fuzzy merge. The re-roll should actually have removed the elseif that was first removed and then restored here. In merging, it was just stuck back in.
Here's another reroll.
Comment #20
cburschkaComment #21
cburschkaWhoops, my patch also fails to understand the actual flow of conditions here. Only instead of breaking node-replying, I broke comment-replying.
Here is another patch that hopefully cleans the structure up correctly. Instead of first rendering the parent comment/parent node, I moved the access check to the top. The parent is now only rendered if the form will actually be displayed.
Comment #22
yched CreditAttribution: yched commentedRetitling - node_build() is now node_view()...
"Are the node ..." :-)
Other than that, the refactoring looks great and results in cleaner code, so RTBC after the comment is fixed.
However, I don't think this addresses the initial bug report :-) - which, admittedly, was not too detailed.
- Go to a node/%nid page, on a node with comments enabled
- Add a new comment with the form at the bottom of the page
- comment_form() sets $form['#action'] to
url('comment/reply/' . $comment->nid)
, so comment_reply() is executed on form submission, and computes a node_view(). It also includes adrupal_get_form('comment_form')
, which lets submit handlers be executed.- comment_form_submit() redirects to 'node/%nid' and the node_view() above is just ditched.
Yikes.
This review is powered by Dreditor.
Comment #23
cburschkaAh! Now I understand the original problem. :-)
I may be wrong here (form API has many surprises), but it looks as though the branch entered on "previewing" a comment already does exactly what is needed - calling drupal_get_form(), and not building the node or parent comment. Maybe the same branch can be used for the "save" action as well?
Fixed the comment, and added this change to address the initial bug.
Comment #24
ardi1983 CreditAttribution: ardi1983 commented#21: comment-node_build-654854-21.patch queued for re-testing.
Comment #25
retester2010 CreditAttribution: retester2010 commented#23: comment-comment_reply-structure-654854-23.patch queued for re-testing.