Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#8 | 1006042-8.overlay-comment-links.patch | 1.11 KB | scor |
#3 | 1006042-3.overlay-comment-links.patch | 1.13 KB | grendzy |
#2 | 1006042-2.overlay-comment-links.patch | 1.11 KB | ksenzee |
Comments
Comment #1
HazaSeems 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
That means we can have the same problem with any link, inside the overlay, that ships with his own fragment.
Comment #2
ksenzeeYes, 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.
Comment #3
grendzy CreditAttribution: grendzy commentedThis works for me. Here's a post-corepocalypse reroll.
Comment #4
scor CreditAttribution: scor commentedThanks @ksenzee + @grendzy. patch tested and works.
Comment #5
webchickThis looks pretty safe to backport, yeah?
Comment #6
ksenzeeYes, should be safe to backport.
Comment #7
catchMakes sense, commited/pushed to 8.x, moving back to 7.x.
Comment #8
scor CreditAttribution: scor commentedreroll for D7. tested and works in D7.
Comment #9
grendzy CreditAttribution: grendzy commentedThanks for the port scor. I have this patch running on several D7 sites.
Comment #10
webchickAwesome, thanks!
Committed and pushed to 7.x.