follow-up to #26966: Fix comment links when paging is used.

I'd like to tweak the D7 behavior just a bit - I really think the comment form should not redirect to comment/$cid, but we can rather calculate the page before the redirect. I find it very disconcerting (i.e. a bug) to lose context after comment submission by having the path (especially an aliased path) change.

We should also make a helper function for finding the page.

CommentFileSizeAuthor
#5 comment-redirect-581534-5.patch1.99 KBpwolanin
#3 comment-redirect-581534-3.patch1.91 KBpwolanin

Comments

pwolanin’s picture

pwolanin commented
Title: Redirect after coment form to node/$nid&page=X#comment-$cid » Redirect after comment form to node/$nid&page=X#comment-$cid
sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Issue tags: +API clean-up

Tagging.

pwolanin’s picture

pwolanin commented
Status: Active » Needs review
StatusFileSize
new comment-redirect-581534-3.patch1.91 KB

Looks like we already have the necessary help function to find the page.

Status: Needs review » Needs work

The last submitted patch failed testing.

pwolanin’s picture

pwolanin commented
Status: Needs work » Needs review
StatusFileSize
new comment-redirect-581534-5.patch1.99 KB

This changes the behavior less (so the test should pass) and also fix a couple little bits of odd/broken code.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented 
+++ modules/comment/comment.module
@@ -2064,16 +2064,21 @@ function comment_form_submit($form, &$form_state) {
+    $page = comment_get_display_page($comment->cid, $node->type);
+    if ($page > 0) {

I think we can simply move the $page = ... into the if() here, no?

+++ modules/comment/comment.test
@@ -46,7 +46,7 @@ class CommentHelperCase extends DrupalWebTestCase {
-    preg_match('/#comment-([^"]+)/', $this->getURL(), $match);
+    preg_match('/#comment-([0-9]+)/', $this->getURL(), $match);

Why not \d ?

This review is powered by Dreditor.

pwolanin’s picture

pwolanin commented

@sun - I don't understand the first comment - we have to find the value for $page before we can test it.

I find [0-9] much more readable at first glance versus \d, but that's personal preference.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented 
// Find the current display page for this comment.
if ($page = comment_get_display_page($comment->cid, $node->type)) {
  $query['page'] = $page;
}

...should be the same?

pwolanin’s picture

pwolanin commented

@sun - functionally the same, but probably less clear about what we are testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community
catch’s picture

catch
he/him
Primary language English
commented

Just a note that this partially circumvents the comment permalinks - however I think that's a good trade-off to keep the context of the node url (and possibly alias) when posting a comment on it, so +1.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

This looks nice and easy. Committed to CVS HEAD.

Would be great to have tests, so marking this 'code needs work'.

tic2000’s picture

tic2000 commented

I opened yesterday #583296: Comment form redirect is hardcoded without knowing of this issue, and I looked for a comment redirect issue (I think, so a -1 for me).
Why is the redirect added to the submit function so there is no way to form alter it? You can only circumvent it by hacking core.

pwolanin’s picture

pwolanin commented

@tic2000 - you can add another submit handler that runs after this one and changes the redirect path. Why do you have to hack anything?

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Fixed

We already have tests for this, which work regardless of the specific URL and went in with the original patch - this is only a cosmetic change, doesn't change what's outside the address bar:

// Confirm a new comment is posted to the correct page.
    $this->setCommentsPerPage(2);
    $comment_new_page = $this->postComment($this->node, $this->randomName(), $this->randomName(), TRUE);
    $this->assertTrue($this->commentExists($comment_new_page), t('Page one exists. %s'));

Status: Fixed » Closed (fixed)

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

purabdk’s picture

purabdk commented

I solved the issue using following code. you can put following code in your module.

function modulename_form_alter(&$form, $form_state, $form_id) {

if ($form_id == 'comment_form') {
$form['#action'] = request_uri();
$form['#submit'][] = 'noredirect_comment_form_submit';
}
}

/**
* Implementation of hook_comment_form_submit().
*/

function comment_no_redirect_comment_form_submit(&$form, &$form_state) {
$form_state['redirect'] = ltrim($form['#action'], '/');
}

ayesh’s picture

ayesh commented

I have also created a (sandbox) module that redirects comment/cid to node/nid/?page=x#comment-cid URLs by altering the page callback of the comment/cid pages.
http://drupal.org/sandbox/Ayesh/1578662

To be honest I'm not sure if this is a good way to alter things but it works.