To reproduce, simply create a comment and unpublish it so it appears in the unapproved comments queue. Click on the link to the unpublished comment and see how it does not link to the comment itself because the fragment contains extra overlay cruft: comment/1#comment-1=&overlay-context=node/1. Not sure yet whether this is a bug in comment.module or overlay.module.

Files: 
CommentFileSizeAuthor
#8 1006042-8.overlay-comment-links.patch1.11 KBscor
PASSED: [[SimpleTest]]: [MySQL] 37,868 pass(es).
[ View ]
#3 1006042-3.overlay-comment-links.patch1.13 KBgrendzy
PASSED: [[SimpleTest]]: [MySQL] 33,937 pass(es).
[ View ]
#2 1006042-2.overlay-comment-links.patch1.11 KBksenzee
PASSED: [[SimpleTest]]: [MySQL] 31,438 pass(es).
[ View ]

Comments

Component:comment.module» overlay.module

Seems to be a overlay bug. (I can't reproduce that if overlay is disable).

Overlay try to add is own fragment to the url. As long as the link to the comment append a fragment too, we have two fragment in the url (comment link + overlay context).
Maybe something to look in overlay-parent.js near

        // Add the overlay-context state to the link, so "overlay-restore" links
        // can restore the context.
        $target.attr('href', $.param.fragment(href, { 'overlay-context': this.getPath(window.location) + window.location.search }));

That means we can have the same problem with any link, inside the overlay, that ships with his own fragment.

Title:Links to comments in the unapproved comments queue are brokenLinks outside the overlay with existing fragments are broken
Version:7.x-dev» 8.x-dev
Status:Active» Needs review
StatusFileSize
new1.11 KB
PASSED: [[SimpleTest]]: [MySQL] 31,438 pass(es).
[ View ]

Yes, this will be a problem with any link that's meant to close the overlay and already has a fragment. I'm not 100% sure what we want it to do though. The overlay-context behavior was added for the block demonstration page, but it's useful in any situation where you have a "go back to the admin page" type of link.

Maybe we should check the link before we add the overlay-context parameter to it, and if it has a fragment already, we leave it alone. Here's a patch that takes that approach. Comments welcome.

StatusFileSize
new1.13 KB
PASSED: [[SimpleTest]]: [MySQL] 33,937 pass(es).
[ View ]

This works for me. Here's a post-corepocalypse reroll.

Status:Needs review» Reviewed & tested by the community

Thanks @ksenzee + @grendzy. patch tested and works.

Issue tags:+needs backport to D7

This looks pretty safe to backport, yeah?

Yes, should be safe to backport.

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Makes sense, commited/pushed to 8.x, moving back to 7.x.

Status:Patch (to be ported)» Needs review
StatusFileSize
new1.11 KB
PASSED: [[SimpleTest]]: [MySQL] 37,868 pass(es).
[ View ]

reroll for D7. tested and works in D7.

Status:Needs review» Reviewed & tested by the community

Thanks for the port scor. I have this patch running on several D7 sites.

Status:Reviewed & tested by the community» Fixed

Awesome, thanks!

Committed and pushed to 7.x.

Status:Fixed» Closed (fixed)
Issue tags:-needs backport to D7

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