The 'Add new comment'-link in comment_node_view() always links to the separate page at comment/reply/$node->nid, ignoring the setting of the node type comment style. When set to COMMENT_FORM_BELOW, the link should lead the user to node/$node->nid

          $links['comment-add'] = array(
            'title' => t('Add new comment'),
            'href' => "comment/reply/$node->nid",
            'attributes' => array('title' => t('Add a new comment to this page.')),
            'fragment' => 'comment-form',
          );

The non-teaser display does take the setting into account:

            $links['comment-add'] = array(
              'title' => t('Add new comment'),
              'attributes' => array('title' => t('Share your thoughts and opinions related to this posting.')),
              'href' => "node/$node->nid",
              'fragment' => 'comment-form',
            );
            if ($comment_form_location == COMMENT_FORM_SEPARATE_PAGE) {
              $links['comment-add']['href'] = "comment/reply/$node->nid";
            }

The attached patch copies the behavior to the teaser view for consistend 'add comment' links. I guess it's too late to backport this to D7?

CommentFileSizeAuthor
#14 comment-add-link-destination-1313300-13-tests-only.patch1.05 KBjwilson3
#14 comment-add-link-destination-1313300-13.patch2.08 KBjwilson3
#12 comment-add-link-destination-1313300-9.patch2.56 KBjwilson3
#11 comment-add-link-destination-1313300-9.patch2.56 KBjwilson3
#3 wrong-uri-in-add-comment-link-if-set-to-comment-form-below-1313300-3.patch1.02 KBrjgoldsborough
comment_add_comment_link.patch1 KBmrharolda

Comments

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Active » Needs review
xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Needs work
Issue tags: +Novice

Yeah, probably too late for D7.

Thanks for looking into this. Note that the Drupal 8.x patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). When the patch has been rerolled, please set the issue back to "Needs Review."

Tagging as novice for the task of rerolling the Drupal 8.x patch.

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

rjgoldsborough’s picture

rjgoldsborough commented
Status: Needs work » Needs review
StatusFileSize
new wrong-uri-in-add-comment-link-if-set-to-comment-form-below-1313300-3.patch1.02 KB

Rerolled for d8

rjgoldsborough’s picture

rjgoldsborough commented

Rerolled for d8

mrharolda’s picture

mrharolda commented

Nice! Thanks, rjgoldsborough!

oriol_e9g’s picture

oriol_e9g
Primary language Catalan
Location Barcelona
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs tests

Maybe can we write a test?

oriol_e9g’s picture

oriol_e9g
Primary language Catalan
Location Barcelona
commented
Status: Reviewed & tested by the community » Needs review
jwilson3’s picture

jwilson3
Primary language English
Location Ecuador
commented
StatusFileSize
new comment-add-link-destination-1313300-9.patch2.56 KB

Here you go, with tests.

jwilson3’s picture

jwilson3
Primary language English
Location Ecuador
commented
StatusFileSize
new comment-add-link-destination-1313300-9.patch2.56 KB

So, I got the name of the link wrong in the test comment. Should be "Add new comment", not "Add a comment".

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Needs work
Issue tags: -Needs tests, -Novice

This looks great! There's just one small thing I'd change about the tests:

+++ b/core/modules/comment/comment.testundefined
@@ -920,6 +920,17 @@ class CommentInterfaceTest extends CommentHelperCase {
+            $this->assertLinkByHref("comment/reply/$nid#comment-form", 0, t('Comment form link destination is on a separate page.'));
+            $this->assertNoLinkByHref("node/$nid#comment-form");
...
+            $this->assertLinkByHref("node/$nid#comment-form", 0, t('Comment form link destination is on node.'));
+            $this->assertNoLinkByHref("comment/reply/$nid#comment-form");

The assertion messages (last argument) should not be translated. See #500866: [META] remove t() from assert message.

When you revise the patch, could you upload a test-only version and the combined version in the same comment, to expose the failures on testbot without the fix? Thanks!

jwilson3’s picture

jwilson3
Primary language English
Location Ecuador
commented
Status: Needs work » Needs review
StatusFileSize
new comment-add-link-destination-1313300-13.patch2.08 KB
new comment-add-link-destination-1313300-13-tests-only.patch1.05 KB

Sadly, a week has passed and no volunteers to summarize #500866: [META] remove t() from assert message. I'm somewhat of of the mind that the onus should be on *that* issue to continue to rebase and find all instances of newly committed offending code. This keeps the commits logical until the new proposed standard actually makes it in. It seems likely based on the size of this issue compared to the size of that issue, that this one will make it in *before* that issue gets a green light. If that is the case, then personally, my vote would go towards committing the patch in #12 that matches current best practices. Whereupon I would be more than happy to then reroll #500866: [META] remove t() from assert message to remove the t() in the piece of code I've written here.

That being said, I'll comply with xjm's request in #13 to remove the t(), (just in case ;) but more specifically, in order to see the fails. The -tests-only.patch is expected to have various failures. Hope it works ;)

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

The patch in #14 looks correct. Thanks!

The reason the other issue is NW is for the patch itself there, not the policy change. Also, you could help out by starting a summary yourself. The issue is near 200 comments long so I'm unsurprised no one summarized it yet. It's probably a good hour of work at least. :)

mrharolda’s picture

mrharolda commented

Looking good!

Can this be considered a bug in Drupal 7? D7 is going to be around for another couple of years before D8 kicks in. I can't imagine anybody relying on the old behavior.

Huh? Where did those tags came from and how do I remove them?!?

xjm’s picture

xjm
she/her
Primary language English
commented

We eventually figured out that a spammer posted between your post and mine. :)

Unfortunately, I don't think this change is backportable, because it does change an existing behavior, which we generally can't do in a release version of core. (It may be possible to work around the issue in D7 with a contrib module.) Reference: http://drupal.org/node/767608

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Makes sense to me. Committed/pushed to 8.x.

Status: Fixed » Closed (fixed)

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