"Add new comment" appears directly above comment form / "post comments" does not work without "access comments" permission

This is true for all themes. It is confusing because the "Add new comment" link / anchor is pointing to the comment form which is the very next element on the page. I think this can be considered as a bug.

This patch removes the "Add new comment" link from the full node view if the comment form is displayed below the node.

I dont know if this breakes other use cases.

And if it is a bug, it should be fixed for D7.

I don't see how this change could be remotely able to fix the described bug.

Better title.

Patches should re-start from scratch and start with tests. We need to test the 2-3 different possible configurations and precisely verify our expectations.... if I'm not mistaken, then we already have a CommentTestHelper::postComment() method that asserts various comment settings; perhaps we can even squeeze those additional assertions into that.

Effectively, I think that the resulting logic that's required here is negated to the logic of the patch in #3. But yeah, let's start with tests first, ensure that tests are actually failing in HEAD, and afterwards, complete the patch to fix 'em.

Cross-linking #583576: HTML link 'Add new comment' should not be display on book printer-friendly page

I can't help, but this entire function is totally b0rked. Attached patch should also fix the "@count NEW comments" string never appearing... (?)

Also, next to writing tests, someone needs to describe, explain, and reason the expected behavior. Currently, I guess the link is output under the assumption that there may be 100ths of comments, and if the comment form is output "below" on the same page, then you may find that link helpful to skip scrolling.

Bartik hides the 'comment-add' link if the comment form is displayed (and also hides the link on teasers).

For Garland, ideally I'd like to see the comment-add link hidden only when there are no comments. If there are comments, if works well as a skip-link to go straight to the form.

Patch attached shows this (NB: this only affects the Garland theme, and should be viewed as an addition to the other patches mentioned in this thread - not a replacement).

#14: drupal.comment-add-link.14.patch queued for re-testing.

Since #850740: unset Add New Comment link is not unset has been marked as a dup, please remove the cruft from Bartik's node tpl as part of this patch, else we can mark #850740 as postponed until this gets in and then fix Bartik later.

Right now, I'm considering any custom code in core themes for this link as bogus, and it likely needs to be removed. Instead, we want to fix the default behavior of Comment module.

However, as mentioned above, we need clear expectations in a test, so as to be able to fix Comment module's behavior accordingly.

Following patches should only add tests, so we see what actually fails. Afterwards, we merge back in the patch from #14.

I agree with Sun. This sort of logic belongs in the module itself, not the themes.

> For Garland, ideally I'd like to see the comment-add link hidden only when there are no comments. If there are comments, if works well as a skip-link to go straight to the form.

This sounds like sensible behaviour to me.

Note that the comments I add call it the "post comment" form, which is wrong as it does not match the UI, but is consistent with the old comment a few lines up. I think it's best to keep consistent now and fix both of those in a later issue.

The property needed checking with empty().

New patch; ran the failing tests locally and they pass now.

Thanks - merged earlier clean-ups and fixes into most recent patch.

Let's get some tests in here that assure we're getting the behaviour we want. I was not able to easily parse this out of the issue, nor the patch.

@#34 -- yes, it's meant to. Where's the comment form set to appear?

> I was not able to easily parse this out of the issue, nor the patch.

As I understand it (and there are many cases, so everyone else please chime in)

- comment form on separate page:
-> link is to separate page at comment/reply/NID
- comment form on same page as node:
- node has comments
-> link is to the anchor on the form below the existing comments
- node has no comments
-> no link; the form is right there with the 'Add a comment' heading

I'm not really happy with the patch in #30 -- it makes the logic with the href for the link harder to follow, and these cleanups don't seem entirely related to this issue. But by all means let's file another issue to clean up the code first and then come back here :)

Details on #34: Comment form on same page. 0 comments exist. View of an anonymous user, having "post comments" but not "view comments" permission.

UGH! That reminds me... #34 is off-topic for this issue: #799998: "post comments" permission does not work without "access comments"

There's no need to split those additional clean-ups into a separate issue.

> View of an anonymous user, having "post comments" but not "view comments" permission.

Ah. That'll be the thing then! The logic counts the actual comments on the node, not the ones visible to the current user.

> There's no need to split those additional clean-ups into a separate issue.

I find they make the patch quite a bit harder to take in :/

In principal, I think each (node) page should provide links/info for:

- Add comment
- View comments (including comment-count)
- View new comments

The layout of some themes may make some of these links redundant, in which case they can (and should) be safely removed by the theme. For example, a skip-link to the "Add comment" form would be redundant if the add-comment form was directly after the position of the skip-link.

The links should be made available for every theme by the core comment module, leaving it up to the theme to choose how to handle the links.

The attached patch adds tests for these (using Stark as the theme, because Bartik has - probably intentional - behaviour which deviates from the norm).
Four of these tests fail, because the comment module does not add all the links which I'd like to see added.

It'd also be good to see these tests extended to cover the other core themes too.

OK, I've changed my mind :-)
Links to new comments need not be shown, because the user is viewing the node page, so there are no new comments.

This means that the links should be:
- Comment count (with in-page link to the comment section).
- Add comment (with link to the comment form, whether there's in-page or a separate page).

This patch:
- Adds these links in the comment module.
- Adds tests for Stark and Bartik
- Patches Bartik so that the link to the add-comment form is only removed when there are no comments (because in this case, the comment form is displayed directly beneath the link).

There are dozens more tests which can be added to this patch, accommodating for different themes, permissions, comment settings (open/closed, in-page/separate-page), and number of comments, but here's a good start.

Oh, lets kick the testbot off.

Hmm.

In node.tpl:

/**
 * Other variables:
 * - $comment_count: Number of comments attached to the node.
 */

It doesn't point out that this variable may not be set all the time (and therefore you should check isset($comment_count) before using it). Probably one for a separate issue.

I've updated the patch from #41 to check that $comment_count is set before using it (in the Bartik node template).

In an attempt to fix this once for really ultimately all...

That's what I'd call a test. mwuahahahahah

  /**
   * Tests comment links.
   *
   * This test verifies whether the expected comment links are output.
   *
   * The output of those links depends on two fundamentally different categories
   * of environment conditions:
   * - Internal conditions: Various Comment module configuration settings and
   *   user access permissions.
   * - External conditions: Whether the user is authenticated or not, and
   *   whether any comments exist.
   *
   * To account for all possible cases, this test implements two matrixes, one
   * for configuration conditions, and another one for environmental conditions.
   * Since the expected behavior of comment links is different for each
   * environment, each set of configuration settings needs to be checked against
   * each set of environmental conditions.
   *
   * The matrixes leverage the fact that most environment/configuration settings
   * can be expressed in 0 or 1.
   */

Note: Contains tests only. Expected to fail.

Alright - here we go! Based on these tests, it doesn't make sense to tackle #799998: "post comments" permission does not work without "access comments" in a separate issue anymore.

Forgot to remove commented out tests.

Luckily, it seems we're going to revise that iteration over multi-dimensional conditions. Thus, just for later reference, the current and known to be working test results of this patch:

321 passes, 0 fails, 0 exceptions, and 92 debug messages

(Those shouldn't change)

> * - Internal conditions: Various Comment module configuration settings and
* user access permissions.
* - External conditions: Whether the user is authenticated or not, and
* whether any comments exist.

I'd think of those more as configuration and, erm, another word I can't think of. But never mind about that.

* To account for all possible cases, this test implements two matrixes, one

The plural of matrix is matrices :)

Lots of credits for this revised implementation go to dmitrig01 and chx.

What. A. Night. -- Awesomeness!

+++ modules/comment/comment.module	28 Nov 2010 03:10:53 -0000
@@ -656,30 +654,32 @@ function comment_node_view($node, $view_
+          if ($comment_form_location == COMMENT_FORM_SEPARATE_PAGE || !empty($node->comment_count)) {

This will still fail if there are comments but the user can't see them, no?

+++ modules/comment/comment.module	28 Nov 2010 03:10:53 -0000
@@ -656,30 +654,32 @@ function comment_node_view($node, $view_
+              'href' => "node/$node->nid",
+              'fragment' => 'comment-form',
+              'html' => TRUE,
+            );
+            if ($comment_form_location == COMMENT_FORM_SEPARATE_PAGE) {
+              $links['comment-add']['href'] = "comment/reply/$node->nid";

Can we put the two different hrefs in an if/else pattern so it's clear these are two alternatives, rather than the current overwriting pattern? The 'fragment' key belongs in that too.

+++ modules/comment/comment.test	28 Nov 2010 09:40:31 -0000
@@ -473,6 +473,278 @@ class CommentInterfaceTest extends Comme
+    // Bartik theme alters comment links, so use a different.

... a different theme.

+++ modules/comment/comment.test	28 Nov 2010 09:40:31 -0000
@@ -473,6 +473,278 @@ class CommentInterfaceTest extends Comme
+      'authenticated'   => array(0, 1),
+      'comment count'   => array(0, 1),
+      'access comments' => array(0, 1),
+      'post comments'   => array(0, 1),
+      'form'            => array(0, 1),

Not all of these are immediately obvious -- 'form', for instance. Could we maybe have a brief comment trailing each line, eg
'form' => blah, // Setting to the comment form location. Or will that just make it noisy?

Powered by Dreditor.

Incorporated #55 and accounted for already existing logic in theme_comment_post_forbidden(), which takes user registration settings into account.

Forgot to remove commented out tests again.

Updated the test code comments. Looks RTBC to me.

I'm not sure about this: the module is making assumptions about the theme's presentation.

+          // Show the "post comment" link if the form is on another page, or
+          // if there are existing comments that the link will skip past.
+          if ($comment_form_location == COMMENT_FORM_SEPARATE_PAGE || (!empty($node->comment_count) && user_access('access comments'))) {
+            $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',
+            );

This is assuming that the theme will render (in this order):
- node
- links
- comments
- comment form
and therefore, a link to the comment-form is redundant if there are no comments.

However, a theme might render in a different order, or put the comment-form in a sidebar. It's better for the module to provide the link even if there are no comments, and leave it up to the theme to choose whether that link should be rendered.

@manarth: The order in which links and page contents are rendered has no impact on the code that generates the links. This patch does not change how links can be rendered in a theme, or, what a theme can or cannot do with regard to comments and comment links.

The problem is this: when the comment-count is 0, the module does not provide a link to the add-comment form.

This is based on an assumption that a link to the add-comment form would be unnecessary, because the add-comment form would be rendered directly following the link. I.e. the module is assuming that a theme will display the data in a particular way. If a theme then uses a different layout where a link would be useful, it has to duplicate the module's functionality in order to build the link to the add-comment form.

It would be better for the module to generate the link to the add-comment form (for all cases where the user has permission to add comments), and the theme can choose not to render the link, if that's appropriate for the way the theme lays out the content.

In that case, the theme can still output a custom generated link manually. Modules and themes can change and alter lots of things, but if they do want to do things differently, then of course they may also need to care for possible further tweaks.

This patch is about Drupal core's defaults, which - in terms of the sub-issue you are raising - make sense for every theme I've ever built so far. Note that the raised edge-case scenario also contradicts the original purpose of this issue.

Lastly, the scenario you're describing actually rather maps to the "comment form on separate page" comment setting...

--
FWIW, also updating issue title, since #799998: "post comments" permission does not work without "access comments" has been merged into this patch and marked as duplicate.

Given how many bugs + regressions this patch fixes, bumping to major.

Fixed the example snippet in the phpDoc of generatePermutations().

Only #799998: "post comments" permission does not work without "access comments" has been merged into this patch. Thus, comment links as well as the comment form should properly appear according to comment settings and user permissions.

The code has been reviewed multiple times by joachim and manarth already. As the permutations test code reveals, it's close to impossible to test all possible configurations manually. Thus, manual testing could only be done for a few random samples. Aside from that, the code looks bullet-proof to me.

#66: drupal.comment-links.66.patch queued for re-testing.

This patch performs extensive testing of almost all possible scenarios in which comment links may appear.

It should get in before D7 is released, as it fixes quite a couple of bugs exposed under certain conditions, from which most are even regressions to D6. I'm not sure what I can do to speed up the process.

#66: drupal.comment-links.66.patch queued for re-testing.

Subscribe. I'd like this to get in before D7 release, and I'm going to do some testing later today.

#66: drupal.comment-links.66.patch queued for re-testing.

I can try if I have a few minutes tomorrow. My desktop was borked last night, so I need to re-set up MAMP / my D7 testbed.

Comment links seem to function correctly in all situations - testing in Garland, of course. Bartik hides the Add new comment link on teasers by default, so no problems there, and this patch fixes the odd behavior of the link in Garland.

The 'Add new comment' link shows up again after a comment has been left, and simply links down to the comment form (also a desired behavior). I second EvanDonovan's RTBC. Commit, please!

#66: drupal.comment-links.66.patch queued for re-testing.

Testbot hiccup.

I asked sun for an issue summary, since I can't figure out how this could possibly have been manually tested properly without one, and got something in reply basically like "It tests all permutations of comment link settings + permissions + something else," but my IRC client crashed and I lost what the exact wording was.

Reading the patch, this appears to be true. The new generatePermutations() function added by this patch and called by testCommentLinks() definitely provides much more robust testing than we currently have. And reading through assertCommentLinks() does a fairly decent job of explaining how the code is supposed to work.

I'm not a huge fan of all this abstraction... testCommentLinks(), setEnvironment(), assertCommentLinks(). It makes the test more difficult to follow than it needs to be. However, I can see that if you were to combine all of these down into one big-ass testX function, you'd end up with a pretty huge mess on your hands (ala comment_node_view(), oh snap ;)).

I asked sun if setEnvironment() ought to be moved into DrupalWebTestCase as a more general function and he said no, that it was just one of many possible ways to use the output of generatePermutations(), which is fair enough. I'm also not sure how I feel about it in general, so keeping it limited only to Comment module for now seems safer.

I'm very nervous to commit something so far-reaching this close to Drupal 7.0's release, now < 60 hours away. However, it does appear that due diligence was done to ensure that every possible combination of this stuff is tested properly, and it clearly fixes some long-outstanding bugs. Additionally, the manual testing that Evan and Jeff did, while not covering every single combination this patch touches, at least didn't run across anything too out of the ordinary in manual testing, which bodes well. That generatePermutations() function also looks like a really useful thing to provide to contrib modules for testing, as well, so it'd be nice to get it in for 7.0.

Therefore, committed to HEAD. Thanks a lot for all the hard work on this, folks!

Re-opened #850740: unset Add New Comment link is not unset for the remaining Bartik comment links fix.

Copy of the revision from October 6, 2011 - 01:38.