Posted by vjordan on November 30, 2007 at 4:25pm
6 followers
| Project: | Drupal core |
| Version: | 6.x-dev |
| Component: | update system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
When upgrading a site to 6.0-beta3 a warning message appeared on the screen which started out something like:
"Saving an old version of the welcome..."
I only got to read the first few words of the message before it disappeared from the screen as other update progress information appeared. The message wasn't available after the upgrade completed and I couldn't find them logged anywhere (watchdog/DBLog). These messages should be left on screen or made available somehow for viewing after the upgrade is complete.
Issue arose using firefox 2.0.0.10.
Comments
#1
I've tried and tried but so far I've not been able to reproduce the problem where this warning message disappears before getting a chance to read it completely.
I'm marking this closed for now.
#2
error.
#3
#4
OK - I have reproduced the error.
The first time a site gets upgraded from d5 to d6 the warning messages that are shown get overwritten. During my upgrade process (d5.3 -> 6.0-beta4) two warning messages are shown. One is the message about the notification email which flashes up after update 29/49. The second is a warning message I didn't see (disappeared too quickly) after update 19/49.
This behaviour does not manifest during subsequent 'upgrades' of a given site (ie, if I revert to a v5 install, then upgrade to v6 again).
After searching around drupal.org I didn't see a related issue. Might it relate somehow to the use of javascript? Problem experienced on both FireFox 2.0.0.10 & .11.
I haven't had an opportunity to test this behaviour on Internet Explorer yet.
As originally noted, it might be worth posting the warning messages to watchdog so they can be reviewed after leaving the update page.
#5
Internet Explorer (v6) has precisely the same behaviour documented above: first time through an upgrade the messages disappear as the upgrade progresses.
Interestingly, if I revert to a d5 site previously test-upgraded to v6 using FireFox, then even if I go through the upgrade process on IE the messages only appear at the end of the upgrade process, and they persist.
Note: browser seems to do full refresh during upgrade - possibly related to batching of upgrade? cf. http://drupal.org/node/176003#comment-651631
#6
I believe the full message is:
are you using beta4 alone, or did you apply any patch on it? this patch you mention above concerns only the install process.
#7
From what I read, batching the install process is something 'borrowed' from the batch approach to updates previously implemented. This behaviour (in upgrading) isn't influenced by the patch I mentioned above. The comment from yched is noted as it might hint towards origin of the behaviour. Then again, it might not.
FWIW I have observed the problem documented here in upgrades from d5 (5.3, 5.4, 5.5) to 6-beta3, 6-beta4, with and without various versions of the 'broken menu items' patch http://drupal.org/node/196043
#8
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...
#9
Also, this bug should only happen when JS is disabled. Can you confirm this, vjordan ?
#10
@#9 - Actually JS was enabled on my clients (both Firefox and IE6) when this error occurred.
I'll test this patch as soon as I get a chance. That's probably going to be a day or two away.
#11
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.
#12
I don't want to divert this issue, but this might be relevant.
I get the collapsible field-set right enough. Works fine on FireFox but on IEv6 the theme doesn't seem to work so well. See screenshot...
As I said, as soon as I have tried out the patch I'll come back here.
#13
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.
#14
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.#15
Applied patch against clean 6.0-beta4. On upgrade the no JS mode fires as described by JirkaRybka. No warning messages during update process. Unfortunately, no warning messages after the update completes either, only the list of executed DB updates.
As far as I know, there should at least be the warning about "Saving an old value of the welcome message..." (as on previous updates documented above). Anything else I can do to help profile this problem some more?
Javascript issue has been logged here: http://drupal.org/node/199946
That issue needs some work to flesh out the problem from #14 above ("OT"?)
#16
A bit more information:
a) When the update runs with Javascript detected, the warning messages appear at the end of the update process, and stay on the screen (both with and without this patch).
b) Using a clean 6-beta4 distribution:- when the update runs in non-JS mode the error messages are written to the screen, but get overwritten as the update progresses and are not shown at the end of the update process.
c) Using a 6-beta4 distribution with the above patch:- when update runs in non-JS mode no messages are shown during the update process nor after the update is complete.
Questions I can't answer yet are: why does update fail to detect JS during the first time through an update of www.example.com? why does it then correctly detect JS when I repeat the update process for site www.example.com, even though I clear cache, clear cookies, start with a clean 6-beta4 distribution?
#17
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.
#18
I'm not sure it's a sessions problem - my uid=1 session persists from d5 through the upgrade into the post-update state (d6).
Summary of this issue so far...
I suggest the primary focus here be on displaying the warning notices after the non-Javascript mode update completes. The JS-presence test issue is being handled at 199946 (possibly stale .js .css).
#19
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...?
#20
What I mean is as in #16 c.
No warning messages are displayed as the batch is running.
No warning messages shown after the batch completes.
#21
AAMOF, I reproduce this as well :
with the patch and js disabled, no messages during the update, no messages at the end.
Investigating...
#22
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.
#23
Sounds great.
My dev machine died last night and it might be some days before its repaired. I'll come back as soon as I've been able to test this patch.
#24
This applies cleanly, and works quite fine for me.
#25
Applies cleanly. Works fine with Firefox with no JS - I get the expected warning messages.
BUT with IEv6 and no JS (JS disabled so
update.php?id=1&op=do_nojsfired) I'm getting no warning messages at the end of the update. (JirkaRybka - I dont suppose you tried IE6?) This IE6 behaviour is of no importance to me, but might be relevant for other users out there.So I'm not sure RTBC is appropriate as I guess IE6 is going to used by the odd person or two (undoubtedly there are some odd IE6 users out there) and may need to be catered for.
Reluctantly marking as CNW solely because of this.
#26
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 ?
#27
Regular IE6 install. Haven't the patience, perseverance or processing power to contemplate IE7, let alone a multiple IE distro [shuddering at just the thought].
I don't think there's anything special about my set-up that might be leading to this - but if somebody with an IE6 install gets this patch to work that's great and could be something on my config. (I can't see that the stale .css issue in http://drupal.org/node/199946 could be implicated here either - can you?)
#28
FWIW: getting a watchdog "file not found" entry for "modules/system/fix-ie.css" with referrer "update.php?op=results"
No sign of this css file in the 6.0-beta-4 distro. Hmmm... still don't think this is the problem.
edit: actually, the 'missing' .css file is in the distro at themes/garland/ - wonder why the path isn't correct when it's being loaded (or more properly, not loaded)?
#29
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...)
#30
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...
#31
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.
#32
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 ?
#33
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...
#34
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.
#35
#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?
#36
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.
#37
BTW - #12 seems like it might be caused by this as well, not getting IE fix properly.
#38
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?
#39
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.
#40
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.
#41
I can't get this patch to apply cleanly - could be a problem with my dev environment (eclipse) which rejects the first hunk (can't match for some opaque reason) but applies the second hunk ok.
But after forcing the patch through with a hand edit the problem in #25 (no warnings after update completes on IE6, no JS) is resolved with this code. And the fix to the original problem has already been committed as noted in #36.
#42
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.
#43
Committed, thanks.
#44
Automatically closed -- issue fixed for two weeks with no activity.