Make update.php work with modules

I've tested this patch:

- I created an update for my mail patch
- I increased the number in system_version to 151.
- I ran the upgrade: everything worked. :)

big +1... this is great drumm... i'll report back when i get a chance to test

is it possible that the update does not work without JS? I am debugging the revisions update and did switch off JS to avoid being directed away from my error message and now the update does to happen at all.

Regarding HTTPPost, the original first two parameters are the only essentials. I don't know if the patched way is better, as I don't like passing nulls or false to functions for no reason.

I'm curious about update.js... what happens when Javascript is enabled, but in an old browser (i.e. isJSEnabled() returns false). It will still try to do the progress thing.

Is there a reason you didn't do the usual combination like this?

if (isJsEnabled()) {
  addLoadEvent(...)
}

I'm working on testing this now, so far applied cleanly and I see the system.module selection in update.php, so looks good.

I'm wondering if you have a sample module,install file that I could test? I'm trying to work out how to structure it so it's picked up by this patch.

+1! Seems to work great. Here was my test case:

1. Installed a fresh copy of Drupal 4.6.4
2. Used the generate-content.php (rev. 1.8) and generate-taxonomy.php (rev 1.2) scripts located @ http://cvs.drupal.org/viewcvs/drupal/contributions/modules/devel/generate/ to generate 500 notes, 1000 comments, 50 vocabularies and 75 terms.
3. Performed a checkout of Drupal HEAD and "uploaded" the files over on top of the ones I already had (simulating a user's upgrading).
4. Went to update.php and chose the default selection for which update to run. All queries passed flawlessly.

Ok, disregard previous patch review. :P I have never done a Drupal upgrade before and got confused and didn't do it the right way.

HERE is the real review of this patch. ;) Still +1, everything is working.

1. In the Drupal Database update screen, the upgrades are no longer listed by date nor have a 'friendly' description about them (as in, "2004-10-31: first update since Drupal 4.5.0 release" => "update_110"). They are just "129." The other way is a bit friendlier.

2. The progress bar is slick!!

3. I really like that instead of reading 500 lines of "OK" messages, it instead tells me if you DON'T see anything below, everything is ok.

4. Clicking the administration link takes me to ?q=admin which is a white screen of death. However, this is unrelated to your patch as a "standard" update.php from CVS does the same thing.

Nope, you're right. After selecting a theme I returned to ?q=admin and it showed up fine. Thanks!

webchick: which browser did you use?

1. The upgrade worked fine. I did get some SQL errors because I didn't start from a clean 4.6.4 install; it tried to fix up some tables while that was no longer necessary. I used a CVS checkout from a couple weeks ago. As a result, the version numbers in the selection menu were somewhat confusing; I had to guess a number because there are no dates. Given the fact such numbers don't communicate or mean anything, I wonder why are offering the user a choice?
2. When I clicked 'Back' after an upgrade (eg. to read the instructions once more), the upgrade process was re-activated. It was smart enough not to run any queries, I think. I think everything is OK, though I panicked for a second or two.
3. Can contributed modules use db_add_column() and db_change_column<code> too? Looking at the code, it's no problem though it might make sense to move these functions out of <code>updates.inc to update.php, so
   

   updates.inc<code> makes for a good/clean example for contrib module authors.  Not sure if it's possible, but it looks like it is.</li>
    <li>The PHPdoc code in <code>updates.inc

   instructs people to use update_N() functions. Shouldn't that be system_update_N()?

4. In documentation write SQL (not sql), MySQL (not mysql) and PostgreSQL (not pgsql).
5. Could you document SCHEMA and SCHEMA_MIN? Maybe consider renaming them? Based on their names, I couldn't guess what they are used for, which is usually a bad sign. After reading the code, I figured it out though. I'm think we should PHPdoc system_version() at the top of the file, just like we document update_N()?
6. It might be a good idea to provide some basic pointers as how to provide an upgrade path for a contributed module? It could also be part of the documentation of the top of updates.inc.
7. Can update_fix_schema_version() be removed later on? If so, it might be worth documenting this too just like you did for update_fix_sessions.
8. Sometimes you write index.php?q=, sometimes ?q=.
9. Code looks good, but haven't looked at it too closely.

+1

We tested this with a csl.org dump and experienced only minor timeout issues.

On a personal note, I highly agree with Dries point #1. It is confusing.

Robin

killes: I had originally tried it on Firefox 1.5 WIn32. I have since tried it on IE 6 XP SP2 (worked fine) and Opera 8.5 Win32. Unfortunately, on Opera the progress bar just sits there forever in the 0 state (with the fancy stripes working away). It doesn't do any updates at all. The good news though is that if I use a different browser to run through the update.php again it works fine (so no harm done).

The Opera test was also done with the access check bypassed, but that shouldn't make a difference I don't think?

What are 'minor timout issues'?

I don't quite understand why we have this timing stuff built-in; isn't easier (less code, less complications) to execute each update in its own request?

Ok ran into another thing. This time trying an module.install file.

I managed to find a module that would benefit perfectly from this functionality: Amazon associate tools. Between 4.6 and HEAD all of the database fields were renamed to be all lowercase.

I was going bonkers because I could not figure out why the update script was not picking up my module.install file. I echoed $module in update_include_install_files and noticed it was only returning a few core modules. I read up @ DrupalDocs.org, and now see that the reason for this is it only picks up modules which are enabled.

For whatever reason, even though the amazon module *was* enabled in 4.6.4, it did not seem to see this in update.php. I had to apply the system update first, then go to admin/modules and click "Save configuration" (the enabled checkbox was already checked), and *then* I was able to go back to update.php and update the module. The only problem is, system.module defaults to 110 again, and if I don't go and manually change it to "no updates available", it will run through the system updates again which will of course fail miserably. ;)

Now, there's still something wrong with my amazon.install file (the $ret array is empty) so I've not got this totally tested yet, but I thought I'd post back to say that this was happening.

Here's my amazon.install, per drumm's request.

Ok, getting somewhere now. :P

The problem with my module.install file is that $_GLOBALS['db_type'] is NULL. Trying to figure out how I can get access to that variable.

So yeah. Typos are hilarious.

It's $GLOBALS, not $_GLOBALS. :P~~~~~~~~~

Here's an updated amazon.install file if you want to use it for testing.

One other minor, nit-picky thing.

Should these files be called module.update rather than module.install? I could see it being confusing for users thinking they had to do something with them in order to install a module.

I fail to see any stopgaps so... here we go?

No, something is wrong... :\

I checked out a fresh copy of HEAD, applied Drumm's latest patch (btw, both system and amazon modules appeared this time, which was cool), and applied it against my 464 backup again. Now every page no matter what gives me "Page not found" although on pages like admin/logs and such I can see the intro text, but no table with any information.

The database looks fine. Everything is where it should be, and the alterations seemed to take place without a hitch (node_revisions is there, all the field names in my Amazon tables are lowercase, etc.).

Is there something in here that expects clean URLs perhaps?

Ok. Without the amazon.install file, it does work properly. So it must be something with that. Still looking...

No, I'm wrong again.

It works fine for *anonymous* users and any user I create subsequently, but uid 1 shows "page not found" on every single page.

sorry forgot to update status.

trying now with completely fresh 4.6.4 install in case I butchered something along the way with my testing.

Nope, something's definitely wrong.

I downloaded a brand new Drupal 4.6.4, blew away my old database completely, and just installed 4.6.4 with absolutely no changes except I created a user #1 and changed the password.

I deleted the Drupal 4.6.4 files, replaced them with a fresh checkout of Drupal HEAD with drumm's most recent patch applied, and ran the update.php script.

The update seemed to go fine without any errors, but upon clicking the link to go back to the 'main page' I just get "Page not found" and that's it. Regardless of what Drupal URL I enter, it's "Page not found," although a few of the admin pages show the initial intro paragraph and that's it. I get the same even if I blow away all my private data (I'm using Firefox 1.5 on Win32) and view as an anonymous user, "Page not found."

Can someone else test this and confirm they're getting the same results? :( I know there are many chomping at the bit for this to get included in 4.7.

I tested w/ a fresh HEAD and Angie's database.

it's not drumm's fine work that's wrong, but the update process itself: menu_rebuild (because we moved a lot of pages around, including the one that handles login...) and system_themes() must be called (this one is because of regions and stuff). These two calls are missing from current HEAD as well.

As soon as this gets in, I'll add the two missing calls somewhere.

Per Amazon's request I added a system_update_156. Instead of menu_rebuild I decided for a cache table deletion, much cleaner and reaches the same goal.

I think update_data() clears the cache. What is the point of update _156?

I looked into update_data and it does not, and as update_data is called for module updates too, there is little point in that. Also. this delete from cache is a recent need, at least it seems so from this http://drupal.org/node/39819 bug report.

Another point in 156 is calling system_themes which makes theme stuff (like blocks...) appear.

to mkae it more precise the truncate {cache} is badly needed after jonbob's recent patch which broke HEAD all over.

I found a bug: it would always stop updating after the first refresh. So if you had long updates, you would manually have to press 'back' until they were all done. The cause was $_SESSION['update_total'] which was used to estimate the percentage, but was never actually being set. This, percentage was always set to 100, which ended the update process prematurely.

I fixed that (you could trigger this bug easily by forcing a small delay in update_data(), for example usleep(250)).

I also tweaked the results page: the list of queries dominated the (more important) instructions at the top. The code also repeated <ul id="..."> several times on that page, which violated the "id's must be unique" rule. I added a 'no queries' item and a bit of style.

Looked over the rest of the code, and don't immediately see anything wrong, though I don't have time for a full-on review... Works as advertised though.

Screenshot of new results:
http://acko.net/dumpx/update.png

1. I'm pretty sure that the current update.php automatically flushes the cache after the upgrade process.

2.Why would we use the timer stuff? Why don't we do a request for each update? Would it add that much overhead and be less prone to timeouts? I'm just trying to understand that design decision.

• Jonbob's separate menu callbacks should have been accomplished by an update_sql which clears the cache because as I have mentioned on devel, if you update to that , you'll get Page not found all over.
• while current update.php does empty the cache for every run, this one does not because it is also for used to install modules which do not requre a cache empty. But if this blocks the committ, I myself will roll a new patch which adds back the cache clear.
• With all that said, the system_themes call is needed because of the many theme system changes and the change in 145 seems to be not enough -- but this again has nothing to do with this patch, whether 156 is required or not can be prolonged after the patch is in. On your word, 156 is out and we will see whether it's needed or not after this is in.
• Finally, the timer , Drumm has wrote a small essay about that to devel: http://lists.drupal.org/pipermail/development/2005-December/011863.html in short, there is more to come if this one is in.

Have I mentioned that it'd be very cool if this would in? :P

After quick review:

* patch changes database.mysql.inc to trigger_error(..., E_USER_WARNING), but mysqli and pgsql still use E_USER_ERROR. Is that ok?
* update_fix_schema_version() uses mysql-specific sql only, needs db_add_column($ret, 'system', 'schema_version', 'smallint', array('not null' => TRUE));
* update_fix_schema_version() changes only the live database, I see no patch to database.{mysql,mysqli,pgsql} ?
* update_fix_sessions() uses $ret that is not declared, but should be, even if not used later on
* update_fix_sessions() in mysql part uses AFTER timestamp. I believe that even if it's mysql version usage of AFTER column encourages use of not standart sql and is not necessarly (you can't depend on column order) and should be removed.

I've paid attention mainly to sql part and have not tested the patch. I'll try to do it today.

I had to fix (revert) changes to updates.inc

I've fixed some things I've mentioned earlier, but 1 and 3 are still not fixed.

The selection (page where you can select from which update you want to update) is not very usefull. How can I check that '129' is first update since 4.6? It does not say anything to me.

After clicking update I got many errors in update queries, but I didn't see the error messages. I could go to admin/logs, but
a) if the update went bad at wrong time, it might be not possibile to check admin/logs
b) it's harder to see which errors are for which queries.

'm going to see why I got the errors, attaching new patch for now.

The bugs I've mentioned in my last followup are related to postgresql, not to this patch. I need to think about how to fix them in the best way.

After change to the way update was done I got no errors...

But in the admin/logs I see some errors, e.g.
pg_query(): Query failed: ERROR: column "access" of relation "up_users" does not exist in /var/www/dt/d/includes/database.pgsql.inc on line 78.
Those were probably logged before initial updates were done
misc/update.js not found
Not sure about this one.

drumm:
Ah, didn't noticed the .js. Thought it'd be included in the patch.

I've changed in update_fix_schema_version() from int2 back to smallint and readded default. I've also changed the database.pgsql.
(This maybe should be changed again, please see the bottom of this post)

I've also noticed, in the same functions, that there are 3 dates that point to update 129, is that correct?:

'2005-04-14' => 129, '2005-05-06' => 129, '2005-05-07' => 129,
'2005-04-08: first update since Drupal 4.6.0 release' => 129,

I've also change update_fix_watchdog() to have $ret defined for pgsql.

The schema_version:
+ schema_version int2 NOT NULL CHECK (schema_version > 0),
It's unsigned in mysql, why > 0 check here? Also, there are many other unsigned columns in the db but pgsql versions of them never were checked for >=0. I think we shouldn't check now.
What should be the default for this column? You can't add NOT NULL column to a table with existing rows, if it has no DEFAULT... Also, later in database.pgsql file you insert some values into system table but do not specify the schema_version.
For mysql it sets it to 0, I think.