Problem/Motivation

If you submit the admin/modules form in the overlay, the form submits, but a second reload appears to close the overlay, which also removes any drupal_set_message()s which were shown (for a fraction of a second) in the overlay before it closed.

Steps to reproduce:

  1. Install a fresh D8
  2. Go to example.com/#overlay=admin/modules https://skitch.com/alanevans/ewcx2/extend-testd8cache.localhost
  3. Choose a random module and check its box https://skitch.com/alanevans/ewct8/extend-testd8cache.localhost
  4. Submit the form https://skitch.com/alanevans/ewctj/extend-testd8cache.localhost

Expected result:
Forms submits and overlay remains open; Any message confirming module enabling is displayed in the overlay.

Actual result:
The overlay including messages appears for a fraction of a second to load, then the modules form reloads in the parent page, thereby closing the overlay. https://skitch.com/alanevans/ewctw/extend-testd8cache.localhost

Additional observations:

  • The POST request triggered by submitting the form returns an explicit redirect to the non-overlay url. On my test site, the target of the POST request is http://testd8cache.localhost:8083/admin/modules/list/confirm?render=overlay and the url from the Location: header is http://testd8cache.localhost:8083/admin/modules
  • The issue appears to be caused by a drupal_static_reset in drupal_flush_all_caches called when any module is enabled or disabled. The reasoning behind flushing all caches including static on module enable is sound, but has the effect of killing statically cached data in, say, the overlay (and probably some other areas that rely on it).

I believe this is major in that it is a regression from previously working behaviour, but can also see a case for it being downgraded to "normal" given the relatively low impact (it doesn't seem to be destructive to form data).

Proposed resolution

A couple of options:

  1. don't use drupal_static for the overlay
  2. add logic to drupal_static which allows for certain items to be protected from general wipes like this

Remaining tasks

  1. Pick a resolution
  2. Patch
  3. Test + Review
  4. Is it testable?
CommentFileSizeAuthor
#29 1788888-test-screenshot.png477.72 KBsjbassett
#16 overlaymoduleafter1.png49.57 KBandymartha
#14 overlay-modules-form-submit-1788888-14-WITH-TESTS.patch2.83 KBDavid_Rothstein
#14 overlay-modules-form-submit-1788888-14.patch742 bytesDavid_Rothstein
#9 overlay-prevent-loss-on-modules-form-submit-1788888-9.patch738 bytesjsacksick
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Alan Evans’s picture

Alan Evans CreditAttribution: Alan Evans commented
Issue summary: View changes

Updated issue summary.

Alan Evans’s picture

Alan Evans CreditAttribution: Alan Evans commented

appears that overlay_drupal_goto_alter has lost the overlay status of the form - up until that point, the overlay mode is "child", but in that function, it is empty.

Alan Evans’s picture

Alan Evans CreditAttribution: Alan Evans commented

Getting lost somewhere between drupal_process_form and drupal_redirect_form on the form submit.

Alan Evans’s picture

Alan Evans CreditAttribution: Alan Evans commented

overlay_get_mode()'s value of "child" gets lost durin form_execute_handlers('submit' ...), shouldn't be too hard to work out where.

Alan Evans’s picture

Alan Evans CreditAttribution: Alan Evans commented

overlay_get_mode()'s value of "child" gets lost durin form_execute_handlers('submit' ...), shouldn't be too hard to work out where.

Alan Evans’s picture

Alan Evans CreditAttribution: Alan Evans commented

On module enable, a drupal_flush_all_caches() is run, which includes these lines:

  // Reset all static caches.
  drupal_static_reset();

That's what kills the overlay's status.

I imagine this has other side effects.

Alan Evans’s picture

Alan Evans CreditAttribution: Alan Evans commented

Appears to have been introduced by the patch committed for #1404198: Separate database cache clearing from static cache clearing and data structure rebuilding

Alan Evans’s picture

Alan Evans CreditAttribution: Alan Evans commented
Issue summary: View changes

Additional notes on the request flow

Alan Evans’s picture

Alan Evans CreditAttribution: Alan Evans commented

The reasoning behind the patch for 1404198 seems sound, so I think we'd need to find a new solution for the overlay.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Priority: Major » Normal

Does this need backport to D7?

jsacksick’s picture

jsacksick CreditAttribution: jsacksick commented
Status: Active » Needs review
FileSize
overlay-prevent-loss-on-modules-form-submit-1788888-9.patch738 bytes

Here's a patch that uses static instead of drupal_static.

hass’s picture

hass CreditAttribution: hass commented

#1116326: Support admin overlay in exposed forms may be a duplicate, but code has lot of differences.

hass’s picture

hass CreditAttribution: hass commented
Issue tags: +Needs backport to D7
David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented

Note the patch at #1848210: [Tests] Submitting a form in Overlay shows content + dsm() for a split second before redirecting to front-end theme is about a slightly different issue, but also contains the same code change here (it is fixing both bugs at once at the moment, because they're visually similar).

The change probably makes sense but I'll mention here a couple concerns I brought up there:

  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.

These might be good candidates for followup issues, though.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented

Also, this bug doesn't occur in Drupal 7.

I'm not removing the "needs backport to D7" tag just yet, though, because there might be an argument for making the same code change in Drupal 7 anyway... I'm not sure.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented
FileSize
overlay-modules-form-submit-1788888-14.patch742 bytes
overlay-modules-form-submit-1788888-14-WITH-TESTS.patch2.83 KB

I tried to write tests for this issue (along the same lines as the tests that were added in #1848210: [Tests] Submitting a form in Overlay shows content + dsm() for a split second before redirecting to front-end theme) but it didn't work because the modules page explicitly sets $form['#action'] to a specifc URL, and it turns out that in that case, the only reason the form submission stays within the overlay under normal operation is due to JavaScript. So there's no way to write a test that will pass. (And I couldn't come up with a simple way to remove the explicit $form['#action'] either.)

I'm attaching my efforts anyway, though. The one without the tests is the one that might be committable (I tweaked the code comments a bit as well; otherwise it is the same as the earlier patches here).

Bojhan’s picture

Bojhan CreditAttribution: Bojhan commented
Priority: Normal » Major

Not sure why this was demoted, we can't have the whole model of the interaction change once you decide to add a module.

andymartha’s picture

andymartha CreditAttribution: andymartha commented
Status: Needs review » Reviewed & tested by the community
FileSize
overlaymoduleafter1.png49.57 KB

Hey, nice simple fix David_Rothstein!
I can confirm that after applying patch overlay-modules-form-submit-1788888-14.patch from David_Rothstein in #14, the modules page stays in the overlay after changing a module status, and it doesn't seem to affect the site with/without the overlay. Would like more tests or opinion, but how?
overlaymoduleafter1.png

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: +VDC
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Status: Reviewed & tested by the community » Needs work

The passing patch is the one that doesn't include test coverage, unfortunately.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

Possible duplicate: #1116326: Support admin overlay in exposed forms

We might want to look at the patch there as well?

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented
Status: Needs work » Needs review

Well, it still looks like a JavaScript bug to me, so no tests, unless we change the modules form to turn it into a PHP bug (but like I said I didn't see an easy way to do that, and it seems a bit out of scope).

I think the other issue is about method=get forms only?

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented

Since the fix is on the PHP end, though, I guess it would be possible to write some kind of test that ensures this particular function retains its memory after caches are flushed, something like that.

attiks’s picture

attiks CreditAttribution: attiks commented

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

malcomio’s picture

malcomio CreditAttribution: malcomio commented

the patch http://drupal.org/files/overlay-modules-form-submit-1788888-14.patch works for me - tested in Chrome & Firefox on Windows

evanmwillhite’s picture

evanmwillhite CreditAttribution: evanmwillhite commented

Confirmed - same patch works for me - tested in Chrome/Firefox on Mac

mariagwyn’s picture

mariagwyn CreditAttribution: mariagwyn commented
Status: Needs review » Needs work

In attempting to validate this per officehours task (http://core.drupalofficehours.org/task/2046), we confirmed that the patch above (overlay-modules-form-submit-1788888-14.patch) DOES NOT fix admin views with exposed filters in overlay (http://drupal.org/node/1116326)

Assisted by: pixlkat

Steps Taken:

  1. Applied patch to create admin/people/list view. Note: we had to apply the patch BEFORE running install.php on local environment.
  2. Confirmed that overlay disappears on 8.0-alpha1. When module is selected and submitted, or filter run on admin/people/list, '#overlay=' disappears from url.
  3. Applied patch above. Confirmed that:
    • Patch fixes overlay problem on: /#overlay=admin/modules.
    • Patch does not fix overlay problem on: /#overlay=admin/people/list or /#overlay=admin/people/whatever (made new page to test).
David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented
Status: Needs work » Needs review

Thanks for testing!

we confirmed that the patch above (overlay-modules-form-submit-1788888-14.patch) DOES NOT fix admin views with exposed filters in overlay (http://drupal.org/node/1116326)

The goal of this patch isn't to fix that :) That's what the other issue is for. They are separate bugs with separate causes.

Patch fixes overlay problem on: /#overlay=admin/modules.

Excellent. Moving back to "needs review".

mariagwyn’s picture

mariagwyn CreditAttribution: mariagwyn commented

Duh. Believe it or not I woke up in the middle of last night realizing that we posted the review on the wrong issue.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented

Ah, I see, that makes sense. It does look like the two issues have been cross-linked a lot, so it's good to finally be sure (thanks to this testing) that they have different causes. Thanks and sorry for your lost sleep :)

sjbassett’s picture

sjbassett CreditAttribution: sjbassett commented
FileSize
1788888-test-screenshot.png477.72 KB

Screenshot of the admin interface module screen being tested.

Success. Tested the overlay-modules-form-submit-1788888-14.patch. Seems good to me to commit.

sjbassett’s picture

sjbassett CreditAttribution: sjbassett commented

Just wanted to confirm. I am following the directions from [Drupal Mentoring](http://drupalmentoring.org/task/2046).

Steps taken:
* Set up fresh install of latest version of drupal-8.x
* Tested the error described in:
* http://drupal.org/node/1116326
Confirmed that the patch: overlay-modules-form-submit-1788888-14.patch from Comment #14 resolved both issues.

pplantinga’s picture

pplantinga CreditAttribution: pplantinga commented
Status: Needs review » Reviewed & tested by the community

Confirm that this issue exists and that the patch fixes it.

Looks good to me!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

I don't think we should be testing php language features... ie. the use of static to preserve multiple calls... so until we have js testing this untestable.

Committed f50aa0f and pushed to 8.x. Thanks!

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

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

Adding info on cause and potential solutions