Posted by Jacine on October 25, 2011 at 5:54pm
10 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | comment.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | needs backport to D7, Needs tests, Novice |
Issue Summary
The following code produces unwanted markup when the user does not have permission to post comments and there are no other links:
<?php if ($links = render($content['links'])): ?>
<nav><?php print $links; ?></nav>
<?php endif; ?>The resulting markup is:<nav><ul class="links inline"><li class="comment-forbidden first last"></li></ul></nav>
The following code in comment_links() causes this to happen:
<?php
else {
$links['comment-forbidden']['title'] = theme('comment_post_forbidden', array('node' => $node));
$links['comment-forbidden']['html'] = TRUE;
}
?>
Comments
#1
Not sure if this is the proper way to fix this, but here's a patch to review.
#2
I think there's a big meta issue somewhere about whether to use this sort of pattern when an element should be empty. Unfortunately I can't find it at the moment, though...
In any event, we'll need to reroll this now. 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.
#3
Re-rolled for #2 D8 /core. After botcheck - back to big meta issue discussion.
#4
I'm not sure where the meta discussion is, but the patch in #3 still applies cleanly to D8 head, so tagging for tests.
#5
I can't replicate this issue on the latest 8.x-dev, looks like it's been fixed. If a user doesn't have the right permissions, they'll see the following markup:
<ul class="links inline"><li class="comment-forbidden odd first last"><span><a href="/user/login?destination=node/1%23comment-form">Log in</a> or <a href="/user/register?destination=node/1%23comment-form">register</a> to post comments</span></li></ul>There's also a test within the assertCommentLinks() method.
#6
Just created a patch for D7 that is based on the patch #3.
#7
The last submitted patch, comment-forbidden-1321248-06-d7.patch, failed testing.
#8
Issue needs to be set to 7.x for testing the patch in #6.
#9
#6: comment-forbidden-1321248-06-d7.patch queued for re-testing.
#10
The last submitted patch, comment-forbidden-1321248-06-d7.patch, failed testing.
#11
I modified the patch for D7. There was a little typo in it.
#12
#13
D8 first
#14
According to #5, this has been fixed in D8. The patch in #11 needs to be tested for D7, and test needs to be written.
#15
Works for me, does this really need tests? Silly issue.
#16
Well if we already have tests for D8 (as indicated in #5), we should get 'em backported to D7.