Do not fire bootstrap hooks during update

This is a bug with the database update system (update.php), not the update notification system (update.module). sorry for the confusion (update.module was not my choice for what this should be called in core). moving this issue to the right place...

I experienced the same (maybe just similar?) error and have included the error information below just in case it helps with troubleshooting this issue.

Oddly, just after the error appeared the browser automatically (and quickly) redirected to the Drupal update page where all updates appeared to proceed correctly (http://www.example.com/update.php?op=results
Not sure why that is happening...

Updating

    * user warning: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ') ORDER BY fit DESC LIMIT 0, 1' at line 1 query: SELECT * FROM menu_router WHERE path IN () ORDER BY fit DESC LIMIT 0, 1 in /home/example.com/public_html/includes/menu.inc on line 286.
    * user warning: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ') ORDER BY fit DESC LIMIT 0, 1' at line 1 query: SELECT * FROM menu_router WHERE path IN () ORDER BY fit DESC LIMIT 0, 1 in /home/example.com/public_html/includes/menu.inc on line 286.

An unrecoverable error has occurred. You can find the error message below. It is advised to copy it to the clipboard for reference.
Please continue to the error page
An HTTP error 200 occured. http://www.example.com/update.php?id=1&op=do

and for the record:

MySQL database	4.1.11
PHP	4.4.4
Unicode library	PHP Mbstring Extension
Web server	Apache/1.3.36 (Unix) mod_fastcgi/2.4.2 mod_ssl/2.8.27 OpenSSL/0.9.7a PHP/4.4.4 mod_perl/1.29 FrontPage/5.0.2.2510

ested for PostgreSQL 8.1.9 on Debian etch 4.0r1 with PHP 5.2.0-8+etch7, flash drupal-5.3 install + active all drupal-5.3 modules + drupal-6.x-dev CVS HEAD update. Access /update.php and face error message as following:

    * warning: pg_query() [function.pg-query]: Query failed: ERROR: syntax error at or near "AS" at character 14 in /home/hswong3i/project/drupal/drupal-6.x-dev/includes/database.pgsql.inc on line 174.
    * user warning: ERROR: syntax error at or near "AS" at character 14 query: UPDATE files AS f SET uid = n.uid FROM node n WHERE f.uid=n.nid in /home/hswong3i/project/drupal/drupal-6.x-dev/modules/system/system.install on line 4233.
    * warning: pg_query() [function.pg-query]: Query failed: ERROR: syntax error at or near "AS" at character 15 in /home/hswong3i/project/drupal/drupal-6.x-dev/includes/database.pgsql.inc on line 174.
    * user warning: ERROR: syntax error at or near "AS" at character 15 query: UPDATE upload AS u SET nid = r.nid FROM node_revisions r WHERE u.vid = r.vid in /home/hswong3i/project/drupal/drupal-6.x-dev/modules/system/system.install on line 4235.
    * warning: pg_query() [function.pg-query]: Query failed: ERROR: syntax error at or near "s" at character 8 in /home/hswong3i/project/drupal/drupal-6.x-dev/includes/database.pgsql.inc on line 174.
    * user warning: query: DELETE s FROM locales_source s LEFT JOIN locales_target t ON s.lid = t.lid WHERE t.lid IS NULL in /home/hswong3i/project/drupal/drupal-6.x-dev/modules/locale/locale.install on line 113.

Does this patch fix it? I don't have pgsql to test so I'm just taking a stab in the dark.

http://drupal.org/node/157682 is the issue that changed 6022 for PostgreSQL compatibility, btw.

And I think this patch will fix those menu warnings.

This supresses the error not fixes it. Ancestors is empty because the menu_masks variable is not yet. menu_masks is set at the end of _menu_router_build. That's called from menu_rebuild. system_update_6021 calls menu_rebuild pretty early. So. What calls menu_get_ancestors and why does it call? Is there a menu_get_item call maybe too early?

I just tried and I am unable to reproduce the menu problem. I checked the code flow and indeed, unless you visited first the site w/o running update it's unlikely you get that because ancestors are only called from the links rebuild which is called after the router rebuild and router rebuild does save the menu masks.

On 6022, I have unified the postgresql and mysql updates into standard query tested on both. viva subselects.

The patch.

First, patch #8 fails.

Parse error: syntax error, unexpected ';' in /Library/WebServer/htdocs/drupal-up/modules/system/system.install on line 4224

The subquery parenthesis wasn't closed properly. Should be:

  update_sql('UPDATE {files} SET uid = (SELECT n.uid FROM {node} n WHERE {files}.uid = n.nid)');
  update_sql('UPDATE {upload} SET nid = (SELECT r.nid FROM {node_revisions} r WHERE {files}.vid = r.vid)');

But the query still errors:

    * warning: pg_query() [function.pg-query]: Query failed: ERROR: missing FROM-clause entry in subquery for table "files" at character 67 in /Library/WebServer/htdocs/drupal-up/includes/database.pgsql.inc on line 155.
    * user warning: ERROR: missing FROM-clause entry in subquery for table "files" at character 67 query: UPDATE upload SET nid = (SELECT r.nid FROM node_revisions r WHERE files.vid = r.vid) in /Library/WebServer/htdocs/drupal-up/modules/system/system.install on line 4225.

Still testing. Will try to attach a proper patch when finished.

The error is caused by trying to invoke {files}.vid within the subquery. Not sure if this is intentional, but the error goes away if {files}.vid is replaced by {upload}.vid.

Attached patch fixes the SQL errors, but I am unsure if it sets the data as desired.

Checked with chx in IRC, {upload} is correct.

Thanks, committed this. I can still reproduce the menu issue.

I need a database dump with which to test the menu issue. Are you guys sure that you went straight to update.php and have not tried to load the page at index.php ?

FWIW, I did not get an error on update.php

I get the two warnings quoted at #2 on every 5->6 upgrade. Going straight to update.php of course.

The dump is a bit of problem: I tried to extract the menu table (whole db dump is quite large, and contains too sensitive data to be attached here), but with the table planted into a new, fresh 5.x install, update seems to stop working entirely. I haven't time to research on that now :-(

So attaching the table alone, in hope that it say something. If not, then ignore it, please.

FYI - the site started on 4.7.3, attached state is 5.1

JirkaRybka, thanks for your dump. I took a Drupal 5 database, added your menu table and got Relocated 264 existing items to the new menu system no errors. If it's too sensitive to be shared, maybe you could contact me in person? As member (and former leader) of the security team, I have quite some experience how to handle sensitive data.

I will look into this, as time permits (perhaps tomorrow, right now just a few minutes available, unfortunately). In the first place, I must figure out why the update.php stopped working for me entirely, before I can confirm whether the problem still occurs. I hope to be able of providing some *real* data/steps to reproduce then, guessing that we're rather wasting time without of that.

OK, I'm digging inside... For now, some more solid info about the issue:

Steps to reproduce:
- Fresh 5.3 install
- Enable Statistics module, and enable all logging
- Replace code to 6.x-dev and run update.php

what happens: debug_backtrace() revealed that...
- update.php uses batch processing, which uses drupal_goto(), which in turn calls module_invoke_all('exit')
- Statistics module's hook_exit() attempts to log the visited page, and having no entry for update.php(+query string) yet, calls drupal_get_title() to get page title.
- So menu_active_title() gets called, proceeding to menu_get_item(), which fails and throws warnings (although $_GET['q'] is set to 'node' from bootstrap), because the 'menu_masks' variable is not set and default is empty.
- I tried to provide a sensible default value usable for the 'node' path, which is array(1), but then it fails with just a bit different warning, because the menu_router table doesn't even exist on that point.

I think the solution here should be to move the menu_router table creation to update_fix_d6_requirements(); and provide fake entry for 'node', to make the new menu system happy, in case that it gets called early (of which the Statistics module is just an example).

Now proceeding to create a patch. Hold on for a while...

Uhhh... This one is quite too big for me :-/

I managed to throw in some fake menu_router and menu_masks, but now there are warnings on menu_links. Attaching the currently existing code, and leaving. Sorry, I've no more time left now.

We probably need to either include a fake menu_links too, or throw this whole patch to recycle-bin, and make update.php to just temporarily disable statistics module. But the question is, whether statistics is *for sure* the only module bringing the new menu system to life before relevant updates are performed.

Oh, and also #5 have some kind of hack, which I overlooked and so didn't test.
Also marking http://drupal.org/node/185131 as duplicate.

This explains why I couldn't replicate the error. I never turned on Statistics module. Excellent report!

JirkaRybka, your analysis rocks. But this is much broader than menu, statistics_exit or even than core updates: in general, you do not want anything to fire while you are updating.

That patch is needlessly complicated... and does not kill hook_boot which can cause problems too. This is a simpler and yet much richer approach.

Plus exrta doxygen.

Looks great, although - as usual - my mornings are not time-permitting more of a review. I guess there's no chance to make update.php running independently of bootstrap, so then this looks like really a solution, hackish only a very little bit. Great.

Only bit I noticed right now are two instances of code comments, which might be corrected to: ... during update we do not _want_ any hook ...

This needs more testing though. I'll be back as time permits.

Comment fixed, thanks. Hackish? Might be. But it's not a big hack and it's quite necessary.

Agreed. :) I'm not setting RTBC right now only because I didn't have a chance to really run the code yet.

I'm not too pleased about killing hook_boot for this page. hook_boot is used by modules like webserver_auth and tokenauth to authenticate a user on an incoming request.

Could we stop firing hook_exit() but keep the firing of hook_boot? Seems like that would resolve the statistics_exit problem. If we have to kill hook_boot() too for this page then we'll survive but is not ideal.

One more idea, forgive me if stupid: Do we need to check the SCRIPT_NAME for hardcoded 'update.php'? What about checking some flag in globals, set by update.php, and re-usable for other external helper-scripts too? install.php comes into mind here, although I'm not sure whether it's exactly the case.

I tested #28: No errors, my update went smoothly. Patch works for me.

Then I created a bit modified patch, by implementing my #31 suggestion, including the #30 security concern, and finding a bit more in the code. So the attached patch:
-- Uses a global variable $update_mode as the flag, so it may be re-used by any helper script, such as install.php, if necessary. The tricky bit about setting it only lives in update.php, bootstrap code is more simple.
-- Allows hook_boot, per #30 concern. As far as I understand it, this fires even for cached pages and very early in the bootstrap, so I think it's not likely to carry any dangerous-for-us function calls inside.
-- Doesn't replace module_invoke_all() by bootstrap_invoke_all(), making the change to bootstrap smaller and not API-changing.
-- Disables also hook_init() at the end of _drupal_bootstrap_full(), which the previous patch seems to be missing.
-- In drupal_page_footer() disables not just the hook, also prevents the update pages from being stored into page cache. If the cache is enabled and user is not logged in (accesses update via the settings.php flag), then caching might be a problem too, if the drupal_page_footer() is ever reached (which I doubt, but still it may not hurt to exclude update from caching more explicitly.)

Patch attached, needs review. Works fine for me. More opinions welcome.

OK, hook_boot can fire. the first $update_mode in update.php executes on global scope so there is no need to global that. I am not sure I like another global but seems no better way. Opinions, please.

I did some more analysis, and simplified the patch:

-- drupal_page_footer() is only called at the end of index.php, and nowhere else. It doesn't fire on update (it would be a big mistake if it did, because then update progress pages would possibly bet cached), so we can safely forget this one here. Removed from the patch.

-- bootstrap_invoke_all() is used only twice (both in bootstrap.inc): One use is hook_boot, which we want to keep alive, and the other use is hook_exit on cached(!) pages, which never occurs during update (unless some other critical bug crashed the update already). So we can forget this one as well. Removed from the patch.

-- As reported in #33, global in update.php was pointless. Removed.

So finally it all boiled down to just hook_exit in drupal_goto() - the real issue here - and it's counterpart, hook_init in _drupal_bootstrap_full()

Attaching new patch. Works fine for me.

As reported here: http://drupal.org/node/182622 there is also a problematic state of blogapi module being installed, as it has a blogapi_init() hook, which tries to drupal_add_link() with a nice url(), which tries to look up a path alias, which fails as the path alias table is not yet updated. So this should be taken into account in development/testing of this fix.

Tested the blogapi issue from http://drupal.org/node/182622 - it was a bit of struggle, so I'm unsure whether all the steps were necessary to reproduce, but anyway:
- Fresh installed Drupal 5.3; enabled Blog, Blogapi, Path, Locale
- Set up the site a bit: Added Czech language enabled as default, translated a string, added a story node.
- Added a translation for string 'RSD' and an alias for URL 'blogapi/rsd' - only after this the bug reproduces for me.
- Updated as usual (code replaced, straight to update.php):

    * user warning: Unknown column 'language' in 'where clause' query: SELECT dst FROM url_alias WHERE src = 'blogapi/rsd' AND language IN('en', '') ORDER BY language DESC in /home/jirux/test-webspace/xxt/includes/path.inc on line 68.
    * user warning: Unknown column 'language' in 'where clause' query: SELECT dst FROM url_alias WHERE src = 'blogapi/rsd' AND language IN('en', '') ORDER BY language DESC in /home/jirux/test-webspace/xxt/includes/path.inc on line 68.

With the #34 patch applied, update went smoothly, no warnings. This really makes sense - as Gábor reported in http://drupal.org/node/182622 , the problem comes through blogapi_init(), which is disabled by this patch.

Conclusion: This is another problem fixed nicely by this patch, and also a real use case supporting the implemented idea if hook_init() blocking.

I can't reproduce the js problem from that other issue, though, and encounter weird no-js effects in update too, so now going to check if that's related or not.

I did another "blind test" - just installed 5.3, enabled *all* core modules, and updated to 6.x-dev. I didn't find any more problems.

As for the JavaScript localization problem, I didn't find a way to reproduce that with core modules. But by analyzing the code, I found out that the _locale_update_js_files() gets called on just *every* update page, because the call is a part of drupal_get_js(), used in both theme_maintenance_page() and theme_install_page(). That seems to me like a very bad idea, as well as firing the JS preprocessing there. So I added the $update_mode restrictions into drupal_get_js() too, so that we get rid of this theoretical problem.

Since I can't reproduce the JS problem, I can't confirm whether this helps or not, but I think it should: The quoted warning in http://drupal.org/node/182622 comes from _locale_rebuild_js(), which I can't see called anywhere else (excepting administrative UI) than via drupal_get_js(). This may be coming from virtually anywhere, so I just didn't track down exact conditions to reproduce (it seems), but still this caller-path is blocked in the new patch. http://drupal.org/node/182622 is now open specifically for this, and may stay so, until someone confirms it being fixed or not fixed.

New patch attached, with the drupal_get_js() part added.

Generally I dislike the basic idea of update.php running on top of normal APIs, which probably makes it always unsure whether problems of this sort bite us again - but that's far beyond the scope of this issue and post-beta 6.x anyway.

Unless someone report more update-problems of this sort, I guess we can proceed to reviews (and hopefully further) now. Thanks in advance.

It seems that install.php is encountering a similar JS-preprocessing issue, having no 'files' directory set up yet, which makes http://drupal.org/node/190283 vaguely related. It seems to support the idea of having a global flag rather than hardcoding 'update.php', but otherwise I don't consider it a blocker here. The other issue deals with the whole batch of installer problems already.

Well, using most API functions should be disallowed in update code. We were beaten by previous API changes, which made farther reaching updates impossible (Drupal 4.6 to 5.0 direct updates are only not possible because of one node type API function change, quite funny). Sure, we are not telling people to update this far, but it could work if we follow some simple rules.

The installer probably don't want to disable anything (as far as I understand recent posts on http://drupal.org/node/190283 ), so I guess this is now *really* ready for a bit of review.

Re-rolling, just to keep things clean (nothing worse than offset).

This really needs someone to review, and perhaps hit RTBC if appropriate.

I tried to think about any other solution, but actually, I was unable to come up with anything. At least without rethinking the update process with a different bootstrap system entirely. I'd like to run this through Dries though, as maybe he has more to say.

I can't come up with a cleaner solution either. I've committed this to CVS HEAD. Thanks folks.