Warning messages disappear when update fires in non Javascript mode

I believe the full message is:

Saving an old value of the welcome message body for users that are pending administrator approval. However, you should consider modifying this text, since Drupal can now be configured to automatically notify users and send them their login information when their accounts are approved. See the User settings page for details.

are you using beta4 alone, or did you apply any patch on it? this patch you mention above concerns only the install process.

This is because we should not display messages on the progress page, but wait until the results page.
I *think* it was the case when update.php was ported to the new batch API, but the recent 'theme maintenance page' refactored a few things. Anyway - patch attached.

Bumping to critical, since this might lead to important notices be overlooked...

Also, this bug should only happen when JS is disabled. Can you confirm this, vjordan ?

Ah. Then it's strange your batch progress page uses the (slower) non-JS mode (refresh by page reload instead of AHAH). This happens sometimes when JS is enabled but not detected because the user didn't get to visit a js-enabled page earlier in his browser session, so we didn't get to set our 'has_js' cookie in drupal.js.
This should not happen in update.php (nor install.php, for that matter), though, since one of the preliminary pages uses collapsible fieldsets and thus loads drupal.js.

Well, might deserve some investigation, but this is not critical, and doesn't change the relevance of the patch above.

Thx vjordan, that could be a hint on why JS doesn't seem detected.
I do think however that this should be reported in a separate issue - let's keep this one focused on the display of messages wrt update progress pages.

Confirming the OT, yet important notes above: I'm testing updates on Linux/FF, collapsible fieldset OK as far as I remember, but it really often falls to the non-js mode. To all: The non-js mode is visible in the URL field of your browser as op=do_nojs. The tasks list in left sidebar also breaks (not very often) for me - falling back to just black bold text (visibly different from normal), no green checked items. Not sure whether recent patches changed that or not.

It could be a problem about sessions not crossing over from D5 state (pre update) to D6 state (post updte) ? I'll try to investigate.

vjordan: On JS enabled update pages, there is JSON output sent back to the client, so messages are not retrieved for display. On non-JS enabled update pages however, the theme retrieves the messages and displays them, but as soon as the page is reloaded, they are gone. So this is why you seem them go by. The primary focus here is to not display messages until a batch if finished. The patch in #8 tries to do this by telling the theme function not to retrieve messages if there is a batch running (this is similarly done elsewhere).

What do you mean by it is not working for you? Does it do the same as before the patch (ie. you see the message going by around the same screen) or you see it go by later or...?

AAMOF, I reproduce this as well :
with the patch and js disabled, no messages during the update, no messages at the end.

Investigating...

Got it. This is because in non-js mode, the batch engine renders a fallback page in case PHP should die during the processing. This is were the messages went...

Patch updated, should be ready now.

This applies cleanly, and works quite fine for me.

vjordan : are you trying this with a regular IE6 install, or do you have IE7 installed and therefore test IE6 with one of the 'multiple IE' distros that exist out there ?

OK, just checking. I'm seeing this as well, but using a 'multiple IEs' version of IE6, and I'm not too sure about those for cookie / sessions things.

This being said, I don't have a clue on why IE6 behaves differently. IE7 works ok. (Sigh...)

And no, I don't think either http://drupal.org/node/199946 or "file not found modules/system/fix-ie.css" (wtf ?) are causing this...

yched: The "file not found modules/system/fix-ie.css" might as well cause this. The 404 page is a normal non-batched page so the errors are displayed there, instead of on the real user facing page. Seems like IE6 misinterprets some relative URLs somewhere in the HTML/CSS output.

Ooh, I hadn't thought about that. True.
Is a this a good thing ? that a 404 eats your messages ? No _that_ bad I guess, since a 404 on a ressource (js, css, image...) usually happens after the HTML itself is regenerated...

Aside from fixing this misinterpreted URL, shouldn't we render 404 pages without messages ?

Hm, it is possible that we'd like to display messages on 404 pages maybe. Trying to think about use cases, but nothing obvious comes to me now...

Since this only affects IE6, only in some cases, and only hides messages rather than actually breaking something. I'm setting this back to normal.

#28 and #31 probably mean, that there's incorrect path generated for the fix-ie.css in the first place. That only IE<7 attempts to really load it (maintenance-page.tpl.php; we'll probably all see it in our HTML source on update, although not firing in other browsers), and then the 404 page cause further consequences (messages flush) is a different thing (not a bug, this part).

I see that maintenance-page.tpl.php in garland directory uses (via a helper function in template.php) the path_to_theme() function, so this is probably where we should look. How is it possible, that we get the path to system module in the end? Is http://drupal.org/node/194098 related? Should be path_to_theme() used in maintenance?

Yes, path_to_theme() might be the reason to the problem here. The fix-ie.css is added differently to the page than other files (ie. late in the theme), so there might be some bug. Also it is not loaded by IE7, so this targets quite well to where we should look.

BTW I just committed the patch in #22, which really makes this issue not critical, only leaving us with an issue or IE < 7, but otherwise issue solved for other browsers without JS.

BTW - #12 seems like it might be caused by this as well, not getting IE fix properly.

This is a bit harder to wrap a brain around... But now I see (theoreticaly - just browsing CVS from a machine, where I can't really run code):

- Basically, the problem is that theme_update_page() is purely a themable function, not a template. There's no file like update-page.tpl.php
- when _theme_build_registry() do it's thing, it process theme hooks from modules first, storing paths to modules for these hooks (all the general ones like 'page', 'maintenance_page', 'update_page' etc. are in fact defined through system.module, so we get system module path stored). This makes sense for themable functions really living inside modules.
- then theme and/engine ones (i.e. found templates) come in, overwriting the initial definitions with their paths. But for 'update-page' we have no .tpl.php file, so it's considered a purely php theme-hook (which it in fact is), and so is owned by system.module and not by Garland(Minelli). That's why we have the system module path stored.
- When it comes to theme('update_page') call, path to theme always points to the owner of that particular hook/template, so we get system module.
- inside theme_update_page() we then call theme_render_template() to "manually" render the maintenance page template, but it didn't come through the usual caller path, and so gets wrong path to theme here.

I can see two solutions:

- Exception for this case: Set the global $theme_path variable inside theme_update_page() before invoking the template (we hardcode its path there already), to reflect the fact that we're in fact switching from themable function to template workflow in a custom way.

- More general: Set the global $theme_path variable inside theme_render_template(), according to the path of supplied file. IMO this makes most sense, because the code inside every single tpl.php file (I believe) expects path_to_theme() to point to its location (= location of its possible additional files). Or are there some additional concerns with sub-themes?

I'd say it would be better to contain the fix within the update code (ie. go with the first suggestion), and not fiddle with how themes are handled in general, unless that is identified as being buggy.

OK, let's stay inside maintenance theming, then.

The attached patch is rather simple, and fixed the problem exactly as I expected it to. Although I haven't IE6 at home to test, I clearly see the problem in the HTML source of rendered pages:

Before:

        <!--[if lt IE 7]>
      <link type="text/css" rel="stylesheet" media="all" href="/6paths/modules/system/fix-ie.css" />    <![endif]-->

After:

        <!--[if lt IE 7]>
      <link type="text/css" rel="stylesheet" media="all" href="/6paths/themes/garland/fix-ie.css" />    <![endif]-->

Also install pages were affected in the same way.

I don't understand the problem mentioned at #41 - today, the patch applied with just a small offset. Otherwise vjordan confirmed that it works (if I understand correctly), so attaching re-rolled for head, and setting back to CNR. No new changes.

Committed, thanks.