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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marcingy’s picture

Status: Active » Needs review
FileSize
711 bytes

This removes the build statement for a node

moshe weitzman’s picture

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

Status: Reviewed & tested by the community » Fixed

Yay for less code. :)

Committed to HEAD.

cburschka’s picture

Title: unneeded node_build() when creating a new comment » [HEAD broken] unneeded node_build() when creating a new comment
Priority: Normal » Critical
Status: Fixed » Needs work

After this patch, NodeTitleTestCase no longer passes.

HEAD at 2009-12-25 12:16:00 (before this commit):

Node title 15 passes, 0 fails, and 0 exceptions

HEAD at 2009-12-25 12:20:00 (after this commit):

Node title 14 passes, 1 fail, and 1 exception

Test run duration: 8 sec
Detailed test results:
----------------------

---- NodeTitleTestCase ----
[...]
Exception  Warning    node.test                      1146                      
    current(): Passed variable is not an array or object
Fail       Node       node.test                      1146                      
    Node preview title is equal to node title.

The test clients are out of service since last night, showing the same test failure as above.

cburschka’s picture

The 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.

cburschka’s picture

In fact, this patch looks stranger the more I look at it.

Starting with the premise:

When submitting a new comment, comment_reply() does a node_build() for nothing, user is redirected to node/%nid before the array is rendered.

That's not true. The code is this:

      elseif (user_access('access content')) {
        $build['comment_node'] = node_view($node);
      }

      // Should we show the reply box?
      if ($node->comment != COMMENT_NODE_OPEN) {
        drupal_set_message(t("This discussion is closed: you can't post new comments."), 'error');
        drupal_goto("node/$node->nid");
      }
      elseif (user_access('post comments')) {
        $edit = array('nid' => $node->nid, 'pid' => $pid);
        $build['comment_form'] = drupal_get_form('comment_form', (object) $edit);
      }
      else {
        drupal_set_message(t('You are not authorized to post comments.'), 'error');
        drupal_goto("node/$node->nid");
      }

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... :(

cburschka’s picture

Status: Needs work » Needs review
FileSize
1.88 KB

I 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.

cburschka’s picture

Title: [HEAD broken] unneeded node_build() when creating a new comment » [HEAD broken] NodeTitleTestCase is failing, node not displayed in comment form.
webchick’s picture

Status: Needs review » Needs work

Margh! 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.

cburschka’s picture

Good idea. I think we should take more time looking at those permissions, anyhow. Good to see head working again. :)

int’s picture

Title: [HEAD broken] NodeTitleTestCase is failing, node not displayed in comment form. » NodeTitleTestCase is failing, node not displayed in comment form.
Priority: Critical » Normal

The patch it's reverted, so don't faill the tests

cburschka’s picture

Title: NodeTitleTestCase is failing, node not displayed in comment form. » Clean up conditions in comment_reply(), avoid node_build() when unnecessary.
cburschka’s picture

Status: Needs work » Needs review
FileSize
1.93 KB

Here's a re-rolled patch.

yched’s picture

Looks sensible to me. Let's see what the bot says.

Status: Needs review » Needs work

The last submitted patch, comment-node_build-654854-13.patch, failed testing.

cburschka’s picture

Oops, yes. I reproduced these failures locally last night too. There's a mistake in there.

cburschka’s picture

Ah. 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.

cburschka’s picture

Status: Needs work » Needs review
cburschka’s picture

Whoops, 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.

yched’s picture

Title: Clean up conditions in comment_reply(), avoid node_build() when unnecessary. » Clean up conditions in comment_reply(), avoid node_view() when unnecessary.
Status: Needs review » Needs work

Retitling - node_build() is now node_view()...

+++ modules/comment/comment.pages.inc	29 Dec 2009 10:47:18 -0000
@@ -45,51 +45,48 @@ function comment_reply($node, $pid = NUL
+      // Are the node open for comments, and is the user allowed to comment?

"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 a drupal_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.

cburschka’s picture

Status: Needs work » Needs review
FileSize
5.08 KB

Ah! 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.

ardi1983’s picture

#21: comment-node_build-654854-21.patch queued for re-testing.

retester2010’s picture

Status: Needs review » Needs work

The last submitted patch, comment-comment_reply-structure-654854-23.patch, failed testing.

  • webchick committed c19b766 on 8.3.x
    #654854 by marcingy and yched: Remove unneeded node_build() when...
  • webchick committed bdcf08c on 8.3.x
    Roll-back of #654854: this causes tests to fail.
    
    

  • webchick committed c19b766 on 8.3.x
    #654854 by marcingy and yched: Remove unneeded node_build() when...
  • webchick committed bdcf08c on 8.3.x
    Roll-back of #654854: this causes tests to fail.
    
    

  • webchick committed c19b766 on 8.4.x
    #654854 by marcingy and yched: Remove unneeded node_build() when...
  • webchick committed bdcf08c on 8.4.x
    Roll-back of #654854: this causes tests to fail.
    
    

  • webchick committed c19b766 on 8.4.x
    #654854 by marcingy and yched: Remove unneeded node_build() when...
  • webchick committed bdcf08c on 8.4.x
    Roll-back of #654854: this causes tests to fail.
    
    

  • webchick committed c19b766 on 9.1.x
    #654854 by marcingy and yched: Remove unneeded node_build() when...
  • webchick committed bdcf08c on 9.1.x
    Roll-back of #654854: this causes tests to fail.