Problem/Motivation
When a user attempts to view a non-administrative page in overlay, they are typically redirected to the same page without overlay. However, if the path includes a query string (e.g. ?page=1
), the query string is incorrectly discarded and the user is redirected to the base path only. This is because of the following code in overlay_init(), which does not include the query string:
// If this page shouldn't be rendered here, redirect to the parent.
if (!path_is_admin($current_path)) {
overlay_close_dialog($current_path);
}
Proposed resolution
Add the current query string to the redirection path, if any. Patch #1146234-1: Overlay redirect does not include query string implements this change.
Remaining tasks
None. The patch is a straightforward bug fix and the overlay maintainer has signed off on the change.
User interface changes
If the user attempts to open a non-admin page in overlay, they will be correctly redirected to the same page, rather than the base path of that page.
API changes
None.
Original report by @jtwalters
The following code in overlay.module does not take into consideration any query string parameters in the current path.
// If this page shouldn't be rendered here, redirect to the parent.
if (!path_is_admin($current_path)) {
overlay_close_dialog($current_path);
}
If the redirect destination includes a query string, such as when using a pager (e.g. ?page=1), the overlay module incorrectly redirects without the page in the query string.
I would expect the redirect to include any existing query string parameters.
Comment | File | Size | Author |
---|---|---|---|
#15 | overlay-query-string-1146234-POST-APOCALYPSE.patch | 634 bytes | xjm |
#10 | overlay-query-string-1146234-10.patch | 614 bytes | Tor Arne Thune |
#1 | overlay-query-string-1146234-1.patch | 653 bytes | David_Rothstein |
Comments
Comment #1
David_Rothstein CreditAttribution: David_Rothstein commentedHere's a patch that should fix it.
Comment #2
jtwalters CreditAttribution: jtwalters commentedThanks—this fixed the issue I was having.
Comment #3
ksenzeeLooks good to me.
Comment #4
catchtagging for backport.
Comment #4.0
xjmUpdated issue summary.
Comment #5
xjmAdded issue summary.
Comment #6
xjm#1: overlay-query-string-1146234-1.patch queued for re-testing.
Comment #6.0
xjmksenzee++
Comment #7
Dries CreditAttribution: Dries commentedThis patch no longer applies and may need a quick re-roll.
Asking for a re-test.
Comment #8
Dries CreditAttribution: Dries commented#1: overlay-query-string-1146234-1.patch queued for re-testing.
Comment #10
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedJust a re-roll of the patch in #1. Also applies to D7.
Comment #11
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedBack to RTBC, as it's just a re-roll.
Comment #12
casey CreditAttribution: casey commented#10: overlay-query-string-1146234-10.patch queued for re-testing.
Comment #14
xjm#10: overlay-query-string-1146234-10.patch queued for re-testing.
Comment #15
xjmRerolled for
core/
.Comment #16
xjmWas marked RTBC in #3 and has just been getting rerolled since. :)
Comment #17
catchBetter get it in then!
Committed/pushed to 8.x, moving back to 7.x.
Patch in #10 should apply so leaving RTBC. This is sufficiently minor I don't think it is worth adding a test for.
Comment #18
webchickCommitted and pushed to 7.x. Thanks!
Comment #19.0
(not verified) CreditAttribution: commentedTypo.