Clean up overlay_close_dialog() and related code

Can we leave out args? (Haven't tested that.)

I'm pretty sure 'args' isn't being used at all, at least by core, so this patch seems safe. I wonder if we could just remove it altogether, although would need to ask @ksenzee or someone if it's there for a reason.

Anyway, for now I rerolled the patch to be in a format that can actually be applied :) I also properly capitalized NULL.

We can take out $args altogether. It was originally meant to be passed as a JS setting, but the JS function that originally used it doesn't even take arguments anymore.

Seems like if we do that we should take out 'statusMessages' also?

Even with this patch, I seem to get this problem when I try to manually run the module update check.

Agreed with Dries, the error I get is:

Notice: Undefined variable: args in overlay_close_dialog() (line 525 of /modules/overlay/overlay.module).

With the patch applied I don't see any "undefined variable" notice when doing this, and I'm not sure how there could be one - the variable is defined :)

I do see that manually running the module update check doesn't seem to succeed at all. @triversedesigns, I guess that's what you meant in #649650: Overlay Close Error - looks like I was wrong to downgrade that from critical, I guess. In any case, I think that's actually a separate problem. The module update check runs in a batch, so the reason it's failing is very likely due to #649224: Cannot run certain batch processes (tests, update manager, etc) with the overlay enabled, not this issue. I'm retitling that one now.

Let's talk about what this means for 'statusMessages' per comment #5.

Even with this patch, I can't run the Simpletests from within the browser. Might be related or not -- still investigating.

I get the same error

#10 is not directly related, see #649224: Cannot run certain batch processes (tests, update manager, etc) with the overlay enabled for that issue.

Patch reuses overlay_close_dialog() in overlay_form_submit().

I think the $args variable exists to allow other modules to pass custom data to the client to be used in javascript. For example to only respond on closing the overlay in certain situations.

Applying the latest patch gives me a

------------------------------------------
Access denied
You are not authorized to access this page.
------------------------------------------

and not a crash when clicking on checking manually with overlay enabled..

BUT I do have access as I am an administrator with every permission enabled and yes i checked if it worked with the overlay disabled, and it does.

This need to be fixed before alpha, cause you can't check manually for updates with the overlay enabled... (it just crashes)
I don't dare to change the status to critical, cause i made some mistakes with that the past days...

What you are experiencing is being fixed in #649224: Cannot run certain batch processes (tests, update manager, etc) with the overlay enabled.

After testing this issue now seems to be resolved due to #649224 patch.

I think it still must be possible to reproduce this somewhere, since the undefined $args variable is definitely still in the code:

function overlay_close_dialog($redirect = NULL) {
  if (!isset($redirect)) {
    $redirect = current_path();
  }
  $settings = array(
    'overlayChild' => array(
      'closeOverlay' => TRUE,
      'statusMessages' => theme('status_messages'),
      'args' => $args,

I think we should continue to use this issue for overall cleanup around overlay_close_dialog(), since it seems to be a bit of a mess still.

I haven't got any problems with closing overlays since january...
Can we close this and reopen if necessary?

No, still an issue. what about patch in #12?

Defintely still an issue. Subscribing.

Reroll.

Moved statusMessages into the $args. I think this is exactly why $args is there; passing variables from the closing overlay to the parent window.

After this issue I'll have a look at #744976: Overlay suppresses drupal_set_message() after node update.

#20: overlayclose.patch queued for re-testing.

The patch in #20 fixes the notice.

Sorry, patch introduces some other problems. I'll post a new one.

Like already proposed in #2 - #5 we can leave out $args altogether. At first I still saw some usecases for passing the args. But as this isnt used by overlay itself and as there are multiple other ways to pass such data, I agree to leave it out.

Patch might fix #744976: Overlay suppresses drupal_set_message() after node update (not tested though).

Patch does not apply anymore.

#25: overlayargs.patch queued for re-testing.

#29: overlayargs.patch queued for re-testing.

So you remove the statusMessages is this patch, but how do you get them over to the parent window then? (Still trying to understand how to fix #744976: Overlay suppresses drupal_set_message() after node update)

If you don't call drupal_get_message() on one request, the messages are preserved to the next one, aren't they?

This patch is great and cleans up a lot of things. However, it needed a few updates. I think the attached is what we want.

The overlay_request_dialog_close() function no longer makes any sense after this patch (actually probably never made sense before it), and therefore should be removed. The problem is that it doesn't work as written, since if you tried to call it outside the place where the overlay module calls it, it would only work, oh, about 50% of the time :) In particular, it would only have worked on form submissions, and if you tried to set it to FALSE, the overlay module would most of the time unhelpfully set it back to TRUE. I removed it because I don't think it does anything you can't do with hook_js_alter() - and if you do it with that, it actually should work 100% of the time!

This patch does solve #744976: Overlay suppresses drupal_set_message() after node update, since it prevents displaying the messages inside the overlay only to have the overlay close a second later so you can't actually read them - as @casey says, the messages will stay in $_SESSION and are correctly displayed on the next page in the parent window.

This patch will also make #662856: Submitting a form that closes the overlay causes the form to flash on the screen again before it closes work again. (The patch that was committed there used to work, but apparently stopped working at some point, I guess due to the mess here but am not really sure :)

Good work, nice cleanup and this also fixes #744976: Overlay suppresses drupal_set_message() after node update

Committed to CVS HEAD. Thanks.