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:

<?php
     
// 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.

<?php
     
// 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.

Files: 
CommentFileSizeAuthor
#15 overlay-query-string-1146234-POST-APOCALYPSE.patch634 bytesxjm
PASSED: [[SimpleTest]]: [MySQL] 33,757 pass(es).
[ View ]
#10 overlay-query-string-1146234-10.patch614 bytesTor Arne Thune
PASSED: [[SimpleTest]]: [MySQL] 33,306 pass(es).
[ View ]
#1 overlay-query-string-1146234-1.patch653 bytesDavid_Rothstein
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch overlay-query-string-1146234-1.patch.
[ View ]

Comments

Version:7.x-dev» 8.x-dev
Status:Active» Needs review
StatusFileSize
new653 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch overlay-query-string-1146234-1.patch.
[ View ]

Here's a patch that should fix it.

Thanks—this fixed the issue I was having.

Status:Needs review» Reviewed & tested by the community

Looks good to me.

Issue tags:+needs backport to D7

tagging for backport.

Issue summary:View changes

Updated issue summary.

Added issue summary.

#1: overlay-query-string-1146234-1.patch queued for re-testing.

Issue summary:View changes

ksenzee++

This patch no longer applies and may need a quick re-roll.

error: patch failed: modules/overlay/overlay.module:139
error: modules/overlay/overlay.module: patch does not apply

Asking for a re-test.

Issue tags:-needs backport to D7

#1: overlay-query-string-1146234-1.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work
Issue tags:+needs backport to D7

The last submitted patch, overlay-query-string-1146234-1.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new614 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,306 pass(es).
[ View ]

Just a re-roll of the patch in #1. Also applies to D7.

Status:Needs review» Reviewed & tested by the community

Back to RTBC, as it's just a re-roll.

Issue tags:-needs backport to D7

Status:Reviewed & tested by the community» Needs work

The last submitted patch, overlay-query-string-1146234-10.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+needs backport to D7

StatusFileSize
new634 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,757 pass(es).
[ View ]

Rerolled for core/.

Status:Needs review» Reviewed & tested by the community

Was marked RTBC in #3 and has just been getting rerolled since. :)

Version:8.x-dev» 7.x-dev

Better 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.

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 7.x. Thanks!

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

Issue summary:View changes

Typo.