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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Haza’s picture

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.

ksenzee’s picture

Title: Links to comments in the unapproved comments queue are broken » Links outside the overlay with existing fragments are broken
Version: 7.x-dev » 8.x-dev
Status: Active » Needs review
FileSize
1.11 KB

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.

grendzy’s picture

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

scor’s picture

Status: Needs review » Reviewed & tested by the community

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

webchick’s picture

Issue tags: +Needs backport to D7

This looks pretty safe to backport, yeah?

ksenzee’s picture

Yes, should be safe to backport.

catch’s picture

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.

scor’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.11 KB

reroll for D7. tested and works in D7.

grendzy’s picture

Status: Needs review » Reviewed & tested by the community

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

webchick’s picture

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.