[Tests] Submitting a form in Overlay shows content + dsm() for a split second before redirecting to front-end theme

It's almost surely an overlay issue: look at the address bar.

Yeah, this is coming up in nodes as well, not related to languages specifically.

I'm going raise this to normal, although I think it might be major. This is a pretty significant regression from Drupal 7's Overlay behaviour. Some git bisect action might help determine when it was introduced.

Tagging with JavaScript. I'll also update the issue summary in a second.

I've noticed this with creating a node. Just pointing that out since it's slightly easier to test than submitting the modules page.

Issue summary updated.

And not like we need more major bugs atm, but this actually is a pretty significant visual regression, and it's both visually jarring, and also prevents you from reading any pertinent status messages at all.

If this gets picked up for office hours, http://webchick.net/node/99 has a tutorial on using git bisect. We know it was before November 22, at least. :D

Seems the same as #1622934: Replace delivery callbacks by leveraging the HTTP kernel, fix the overlay module (and that explains the cause) but not sure which should be closed as a duplicate.

"Overlay autoclose behaviour broken, attempts to use nonexistent hook_page_delivery_callback_alter" is a good explanation of the bug, but that issue was retitled. I'll set that one back.

Yeah, overlay attempts to use dilvery callbacks which got removed altogether, but overlay was left hanging.

Ran git bisect:
568f9c sep 13 12 good 411220 feb 9 13 bad
6f194 feb 14 12 good 9fdd40f may 19 12 bad
1bc549 feb 29 12 good 675744 mar 23 12 bad
b49fa1b mar 19 12 good 675744 mar 23 12 bad

ae05a8de86 mar 13 12 good
aab8b66 mar 13 12 weird *
b32d67729 mar 14 12 weird *
dd2bfb73 mar 12 12 weird *

* http://d8.localhost:8082/index.php/node/add Not found << likely caused by a local issue (?) as the problem occurred several times.

At this point, I don't have the exact commit reference b/w bad and good, but if the issue is still open tomorrow, I will refresh and continue.

Thanks for doing that, @no_angel, but since we know the root cause (see above) I'm not sure if git bisect is needed here anymore?...

I looked for a change notification describing the issue - there wasn't one but there is #1677304: Remove drupal_deliver_page(), ajax_deliver(), drupal_deliver_html_page() open to add it (and that's also the issue that presumably broke this originally).

So basically, the task is to take everything that overlay_page_delivery_callback_alter() is doing and, uh, rewrite it and put it somewhere where the code will actually run :)

After doing some digging, it seems to me to be a case of two different bugs here.
One where the javascript to refresh the parent page is triggered to late, in the case of the modules page,
and one where a page is shown in the overlay before closing because of the overlay module using hook_page_delivery_callback_alter.
I have a suggestion how to fix bug nummer two, patch attached, I will see if I can figure out how to fix bug one, but so far, no luck.
I'm not sure the patch I made is the right way to go, feedback would be appreciated (first core patch).

Are all page redirections universally expected to run through drupal_goto(), so their redirections can be altered reliably with drupal_goto_alter?

Yes, the use of drupal_goto was what I was unsure of. But so far as I could see, all form submits and redirects in general seem to go through drupal_goto, the keyword unfortunately being "in general".

About the bug on the module list page.
I'm making the assumption that the wanted behavior is that the page reloads in the overlay since this is the behavior in D7 and on other admin pages?
I used a lot of time debugging this, and finally found out it is caused by drupal_static being reset sometime during form submit because of all caches being flushed.
If I implement a normal static in overlay_set_mode() then I get the expected result, the admin list rendered in the overlay, and no replacing of the system messages.
Can anybody give input to as whether this is a viable solution? I've added the small change to the above patch.

This is abit over my head so I will take me off the "assigned". Sorry.

I used a lot of time debugging this, and finally found out it is caused by drupal_static being reset sometime during form submit because of all caches being flushed.

Wow, great find, @dysrama! It does look like drupal_flush_all_caches() in Drupal 8 is doing this:

  // Reset all static caches.
  drupal_static_reset();

I don't know why it does that offhand, but it seems like a deeper problem (or at least something that needs to be documented). Basically it's assuming that all drupal_static() usages in Drupal are for cached data only (rather than for storing data in one phase of the request and accessing it later, like Overlay is doing here)... There are quite likely other places in Drupal that are affected by the same issue.

As for the other part:

+  $items['overlay/close'] = array(
+    'title' => '',
+    'page callback' => 'overlay_deliver_empty_page',
+    'access arguments' => array('access overlay'),
+    'type' => MENU_CALLBACK,
+  );

I'm not sure about making overlay_deliver_empty_page() a menu callback, since it prints HTML directly and then exits. Normally menu callbacks are just supposed to return so that their content can be inserted into the overall page. Short-circuiting the page building process in the middle of that seems like it would break people's expectations.

That is why it was a delivery callback rather than a menu callback in the first place, since delivery callbacks were the accepted way to completely replace the response handling with something else. I am not sure off the top of my head how that is supposed to work in Drupal 8... but in theory there should be an "official" supported method for doing that.

I agree that using a menu callback to close the overlay goes against people's expectations, but I can't find any way to do something like the delivery_callback_alter in d8. I've tried numerous different approaches other than the menu callback one, but to no avail. Any suggestions or pointers in the right direction would be greatly appreciated.

Regarding the static issue, I can see the reasoning behind clearing drupal statics on a general clear cache, but you're right that this behavior likely affects other areas in Drupal. Whether the behavior is correct though, and the solution is to not use drupal static, or the resetting of static caches should be removed for clear cache, I have no idea. So I guess this part of the issue is stuck for now until somebody with the proper insight can comment?

@Crell asked me to explain what does overlay has to do with page delivery callback :) I think the code speaks for itself especially with its code comments, so I looked up the relevant 3 functions still in HEAD:

/**
 * Stores and returns whether an empty page override is needed.
 *
 * This is used to prevent a page request which closes the overlay (for
 * example, a form submission) from being fully re-rendered before the overlay
 * is closed. Instead, we store a variable which will cause the page to be
 * rendered by a delivery callback function that does not actually print
 * visible HTML (but rather only the bare minimum scripts and styles necessary
 * to trigger the overlay to close), thereby allowing the dialog to be closed
 * faster and with less interruption, and also allowing the display of messages
 * to be deferred to the parent window (rather than displaying them in the
 * child window, which will close before the user has had a chance to read
 * them).
 *
 * @param $value
 *   By default, an empty page will not be displayed. Set to TRUE to request
 *   an empty page display, or FALSE to disable the empty page display (if it
 *   was previously enabled on this page request).
 *
 * @return
 *   TRUE if the current behavior is to display an empty page, or FALSE if not.
 *
 * @see overlay_page_delivery_callback_alter()
 */
function overlay_display_empty_page($value = NULL) {
  $display_empty_page = &drupal_static(__FUNCTION__, FALSE);
  if (isset($value)) {
    $display_empty_page = $value;
  }
  return $display_empty_page;
}

/**
 * Implements hook_page_delivery_callback_alter().
 */
function overlay_page_delivery_callback_alter(&$callback) {
  if (overlay_display_empty_page()) {
    $callback = 'overlay_deliver_empty_page';
  }
}

/**
 * Prints an empty page.
 *
 * This function is used to print out a bare minimum empty page which still has
 * the scripts and styles necessary in order to trigger the overlay to close.
 */
function overlay_deliver_empty_page() {
  $empty_page = '<html><head><title></title>' . drupal_get_css() . drupal_get_js() . '</head><body class="overlay"></body></html>';
  print $empty_page;
  drupal_exit();
}

In short, it short-circuits the page delivery with an alternate callback so the page can be delivered super-quick (small payload) and the overlay can be closed and reload the page underneath that would display any messages set.

Since no such feature seems to be in Drupal 8(?! tThe current proposed approach up in #17 (http://drupal.org/node/1848210#comment-7053372) is to introduce a full new path that overlay would change redirects to and that would close the overlay and avoid messages printed. So instead of an internal short-circuit in the request, it actually uses one more request round-trip with the browser. David Rothstein did not like the concept of this in #20.

Any better ways this can be done with the current request handling?

Hm. Well if the goal is "don't use a normal page delivery callback, use a fake one that does nothing", the place to do that now would be to modify RouteProcessorSubscriber:

  public function onRequestSetController(GetResponseEvent $event) {
    $request = $event->getRequest();

    // @todo This should all get converted into one or more RouteEnhancers.
    if (!$request->attributes->has('_controller') && $this->negotiation->getContentType($request) === 'html') {
      if ($request->attributes->has('_form')) {
        $request->attributes->set('_controller', '\Drupal\Core\HtmlFormController::content');
      }
      elseif ($request->attributes->has('_content')) {
        $request->attributes->set('_controller', '\Drupal\Core\HtmlPageController::content');
      }
      else {
        throw new \InvalidArgumentException('No valid subcontroller key was found');
      }
    }
  }

And detect that we're in overlay mode, then set a different controller. That's how the Ajax system will be working any minute now:

#1843220-67: Convert form AJAX to new AJAX API

As soon as that's in, though, I am going to refactor that code so that rather than a hard coded list we just have a series of route-enhancers. So you'd write an Overlay route enhancer that switches to an Overlay-sensitive controller.

And I'd go a step further and say that, like Ajax, the overlay should be sending requests with an alternate mime type, application/vnd.drupal-overlay+html or something like that. That way any page request can become an overlay-able request just by flipping the mime type of the request. That's exactly what the Ajax patch above is doing, and it's quite straightforward.

I've updated the change notice here http://drupal.org/node/1937056 to describe one method for accomplishing the functionality of hook_page_delivery_callback_alter() using a Symfony event listener and changing the request controller mid-request.

Just a note that the patch in #1911728: Remove hook_init() appears to fully re-implement overlay using the new routing system, including ditching hook_init for kernel.request.

Related: #1938980: Fix Ajax system; the last remnants of the old API must be swept away

What we'll want to do for Overlay is basically the same thing as the Ajax system is doing now.

Bug confirmed w/ fresh D8 install (OSX v10.8.2)

Patch #17 then applied, bug not seen on Firefox 19.0.12.
Checked: Google Chrome 25.0.1364.172 and Safari 6.0.2 {copied http://d8a.localhost:8082/node/x in browser and added new content. Display as expected: Article xxxx has been created. {not sure if this is how to test}

commit 0d56b7bb67958d26f79487a505142669c839fb7b
Author: no_angel
Date: Sat Mar 30 18:24:43 2013 +1100

fix apply for overlay display bug, no_angel's first manual patch test, yippie

192-168-1-5:drupal8 admin$ git diff
diff --git a/core/modules/overlay/overlay.module b/core/modules/overlay/overlay.module
index 197d006..65af03e 100644
--- a/core/modules/overlay/overlay.module
+++ b/core/modules/overlay/overlay.module

NOTE: this is my first manually tested patch so please review and provide feedback on test and documentation.

I am happy this has a high priority, this is incredibly jarring. Hoping we find a solution

Removed "Needs manual testing"

per http://drupal.org/node/1489010

If the patch does resolve the issue and (if appropriate) has been tested in all supported browsers and/or all core themes, remove the "Needs manual testing" tag if it is present.

Hope this is appropriate!

The real issue here per @crell in #28 is that the Overlay module/feature needs to be re-implemented using the new Symfony routing system and updated Drupal AJAX framework. Any patch to the current Overlay code will be inevitably beside the point? I am assuming the necessity of Overlay in core has already been settled.

AFAIK, the only reason this issue remains open is that the change notice in #26 is still in flux; @crell also addresses the (once-pending) API change in #25:

...we just have a series of route-enhancers. So you'd write an Overlay route enhancer that switches to an Overlay-sensitive controller.

And I'd go a step further and say that, like Ajax, the overlay should be sending requests with an alternate mime type, application/vnd.drupal-overlay+html or something like that. That way any page request can become an overlay-able request just by flipping the mime type of the request. That's exactly what the Ajax patch above is doing, and it's quite straightforward.

Regarding, the jarring-ness of the Overlay in the current D8-dev, it's unessential, right?. Disable it?

Let's close this and start another issue for implementing Overlay module with the new routing and AJAX systems (if that issue doesn't exist already)?

We've had an incredibly bad user-facing regression introduced by WSCCI for almost a year now. It'd be nice to get this fixed. If this patch fixes it, I'd love to get it committed, then do the ultimate awesome thing in another issue.

 function overlay_set_mode($mode = NULL) {
   global $base_path;
-  $overlay_mode = &drupal_static(__FUNCTION__);
+  static $overlay_mode;

I came across this earlier but forgot to mention it here... There has actually been another issue open for a while about the particular bug on the admin/modules page which this is fixing: #1788888: Overlay at admin/modules closes on submitting the form.

Not sure if that means we should leave it out of this patch (since's it not what this issue was originally about). In any case, I'll at least add a link to this issue over there, and mention some of the concerns about it as well. The patch in that issue does the same thing but has a code comment explaining why.

Unsubscribing :-) Last I heard Overlay was the ultimate optional, the first thing most people in-the-know disable upon stumbling upon a vanilla Drupal 7 site. I don't understand dinking around with the current code in D8 when the easiest solution is to merely disable overlay in its current out-of-step incarnation.

Good luck! Looking forward to the "ultimate awesome thing"!

#17: overlay-closing_overlay-1848210-17.patch queued for re-testing.

Looks like unrelated test failures.

Again, see the pending issue here: http://drupal.org/node/1911728#comment-7051588

Only one example of major API patches which directly affect and necessarily break Overlay.

I can't imagine why Overlay is not postponed until all that is settled.

I don't see how D8 Usability testing is helped by temporarily making the old Overlay code work. It will only lead to a lot of irrelevant testing given that the ultimate implementation of Overlay will be so inevitably different?

@krlucas: how would the proposed backend refactorings affect the user interface?

@gabor They shouldn't, right? But why try keep re-implementing Overlay under shifting ground. Overlay is a wholly optional UI feature. I don't see why we are so worried about making it work outside of the trajectory of the other AJAX and routing enhancements in D8. I'm all for documenting how Overlay works now, and how Overlay SHOULD work in the future. And I'm happy that Overlay is very broken in D8 right now. I just don't see the advantage of spinning our wheels to make it work with deprecated code.

@krlucas: I don't see how having a stop-gap tested fix committed for overlay that fixes a regression introduced by WSCCI spinning wheels. I see how discussing it for eternity is spinning it though.

@gabor I agree. Go for it.

Yeah, I don't even know how one would verify that post-routing system conversion Overlay even works, because right now it's very obviously broken and has been since June 2012. The brokenness keeps tripping up people testing things totally unrelated to Overlay (e.g. multilingual features, new modules page, content previews, etc.) and slowing down other issues on bogus "needs work" status changes that aren't actually those issues' fault.

Probably makes sense to roll in the comments from the issue that David Rothstein pointed out and mark that issue as a dupe, and just fix it all here. When/if the routing system conversion of Overlay is done, then we can commit that. Until then, a stop-gap that quits sapping the sanity of everyone working on anything UI-facing is something i'm comfortable committing.

So like @no_angel and @YesCT I can confirm that the patch in #17 fixes this bug. If the idea is to commit something, anything for the sake of the "sanity of everyone working on anything UI-facing" then what more is there to do? Sorry if I'm being dense.

David Rothstein's concerns in #1788888 are:

1. If drupal_static() cannot be used as a replacement for regular PHP statics anymore (but rather is only "allowed" to be used for static caches) then it's very poorly named :) And at a minimum this limitation should be documented.
2. It is quite likely there is other code out there that is using drupal_static() in a similar way and therefore has similar bugs.

Both of these concerns seem outside the scope of a "stop-gap" patch endorsed by @webchick and @Gábor Hojtsy. @webchick, you marked as needs work. What needs work?

#14: overlay-closing_overlay-1848210-14.patch queued for re-testing.

#17: overlay-closing_overlay-1848210-17.patch queued for re-testing.

tagged with "needs issue summary update" with updated proposed solution.

Sorry, needs work for:

"Probably makes sense to roll in the comments from the issue that David Rothstein pointed out and mark that issue as a dupe, and just fix it all here."

Grrr.

If we're OK with fixing this by bypassing the normal page request/exit process (as #17 already does), then why do we need the extra menu callback? Here's a simpler patch that just closes the overlay directly. Seems to work for me.

I've also kept around some of the useful code comments from the deleted function.

I have left out the fix from #1788888: Overlay at admin/modules closes on submitting the form. for now since I can only keep one bug in my head at the same time :) Anyway, the people on that issue deserve commit credit for that fix.

To address the concern about fixing this bug now only to have it potentially reoccur later if the Overlay is refactored, I think we should try to write automated tests for this. It's not immediately obvious to me why we can't.

This seems to work.

The tests could probably be backported too (even though the bug itself doesn't exist in Drupal 7).

I still think that the way overlay works is not compatible with the new page flow in the first place; there are much cleaner ways to implement it. Unfortunately, the correct way to do that is to add another Route Enhancer to set an Overlay specific controller and use mime types... which is blocked on #1938980: Fix Ajax system; the last remnants of the old API must be swept away, which is blocked on the Ajax system: #1941288: Regression: ajaxPageState not being updated with AjaxResponse, assets (js/css) being added twice. (Kill me now...)

So I'm OK with this hack for now, because it doesn't make the situation any worse. We do need, however, to rethink how Overlay works in the first place because there are much better options available to us now that we can be leveraging.

A little commenting standards correction.

Forgot a backslash.

From the commenting standards document

* Contains \Fully\Qualified\Namespace\And\NameOfTheClass.

This bug drove me bonkers when I was trying to record some D8 videos. The patch works beautifully!

Without the patch: https://www.dropbox.com/s/zkf5w0o9h7xidl0/overlay_redirect_bug.mp4

With the patch: https://www.dropbox.com/s/bpqtaw4tgah3olb/after_overlay_patch.mp4

It took me awhile to sort out what was going on in this issue (the summary could really use an update for the current status), but I agree that:

1. We need to fix this regression, not wait on a new API that may take an indeterminate amount of time.
2. Now that we have a test, we know we won't break it again.
3. We can fix #1788888: Overlay at admin/modules closes on submitting the form. separately.

Awesome-sauce!! Great work on this, folks.

Committed and pushed to 8.x. Thanks!

Moving down to Drupal 7 for potentially backporting the tests.

I just tried writing some similar tests for #1788888: Overlay at admin/modules closes on submitting the form. by the way, but it didn't work due to some peculiarities with that form... oh well. So the one line patch might be all that we can do for it. Feel free to bump that issue back up to "major" to get it more attention if necessary (it is a bit less important than this one, though, since it affects less heavily-used forms.)

I think that's #1788888: Overlay at admin/modules closes on submitting the form., isn't it?

closed #1999218: Help text disappears before being read as a duplicate, this confuses people :/

Updating the issue summary, based on new issue scope/definition.