I have two patches for the update system. I'll provide a changelog below for each patch. The patches have been tested by loading the database.mysql and database.pgsql files from 4.5 then running the update process. Works like a charm.

Changelog for update.php.177.diff
* In update_data(), if $ret isn't array, don't call array_merge().
* In update_finished_page(), if $queries isn't array, don't try to foreach it.
* Move drupal_bootstrap(DRUPAL_BOOTSTRAP_DATABASE) call to a more obvious location.
* Add update_get_db_server_version() and $GLOBALS['db_server_version'].

Changelog for updates.inc.207.diff
* Add docblock explaining how to keep file orderly.
* Add mysqli to locations where checks existed for only mysql (because users could change or import data to mysqli based system before upgrading).
* Place all DBMS checks in standardized switch statement to improve consistency.
* Allow PostgreSQL to drop columns if version allows it.
* Remove database level UTF-8 check from _system_update_utf8() (because some tables and/or columns could have been created before the default character set was changed to UTF-8).
* Make nesting and wrapping consistent.
* Add comment about potential query failures regarding locales_target indexes.

CommentFileSizeAuthor
#16 updates.inc.219-220.diff37.34 KBdanielc
#15 updates.inc.218-219.diff1.58 KBdanielc
#5 update.php.177.v3.diff3.22 KBdanielc
#4 update.php.177.v2.diff3.25 KBdanielc
#1 updates.inc.207.diff66.26 KBdanielc
update.php.177.diff3.17 KBdanielc
preview29.56 KBAmazon
thumbnail6.6 KBAmazon
_original58.67 KBAmazon

Comments

danielc’s picture

danielc commented
StatusFileSize
new updates.inc.207.diff66.26 KB
Cvbge’s picture

Cvbge commented

Hallo,

wouldn't it be better to allways upgrade 4.5->4.6 and then 4.6->4.7? I think we should improve 4.5->4.6 upgrade process in 4.6 branch, and 4.6->4.7 in 4.7 branch.

I don't think you should set status to RTBC without having it reviewed, especially with 66K patch...

Cvbge’s picture

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

Hallo,

wouldn't it be better to allways upgrade 4.5->4.6 and then 4.6->4.7? I think we should improve 4.5->4.6 upgrade process in 4.6 branch, and 4.6->4.7 in 4.7 branch.

I don't think you should set status to RTBC without having it reviewed, especially with 66K patch...

danielc’s picture

danielc commented
StatusFileSize
new update.php.177.v2.diff3.25 KB

The existing upgrade system allows users to go straight from 4.5 to 4.7. This is a good thing and should be maintained. There's no reason to force users to jump through hoops.

As mentioned in my initial comment, the concepts put forth in the patch are indended to be able to be back ported to the 4.6 branch.

Wasn't trying to be presumptuous about the marking it "ready to be committed." Just misunderstood what the categorizations meant.

That aside, attached is a revised version of update.php that strips out unwanted strings from the MySQL version number.

danielc’s picture

danielc commented
StatusFileSize
new update.php.177.v3.diff3.22 KB

Version 3 of the update.php patch: minor optimization of removing the "break" from my switch statement because it is unnecessary due to their being a "return" right above it.

dries’s picture

dries commented

I don't understand the purpose of this patch. If $ret is not an array, something is wrong with the update functions. Ditto for $query. Also, $GLOBALS['db_server_version'] isn't used...

Tempted to mark this "won't fix". What is the critical problem you are fixing?

dries’s picture

dries commented

It is extremely hard to review this patch; the diff is kinda large. Any way to trim it down? I'd like to understand the core problem/fix. We can leave cosmetic changes for a second patch.

Steven’s picture

Steven commented
Priority: Critical » Normal

* Remove database level UTF-8 check from _system_update_utf8() (because some tables and/or columns could have been created before the default character set was changed to UTF-8).

I don't understand what you mean with this. Before 4.7, we created tables with the default character set, i.e. that of the database. So, if the database character set was not UTF-8, we need to do table conversions. Otherwise, they are already UTF-8.

Cvbge’s picture

Cvbge commented

The existing upgrade system allows users to go straight from 4.5 to 4.7. This is a good thing and should be maintained. There's no reason to force users to jump through hoops.

Well, it's better for users, but worse for us - more code/cases to maintaint and test => it's worse for users ;)

Anyway, you don't need the whole database version stuff. Drupal 4.7 requires PostgreSQL 7.3 which supports columns dropping.

danielc’s picture

danielc commented

is_array stuff for $res and $query
--------------------------------
I added these checks because PHP warnings were cropping up. If their not being arrays indicates an underlying problem, then we should add debug backtrace code inside the if is_array checks to aid developers finding the problems.

db_server_version
-----------------
This is used in the new updates.inc file.

separating formatting changes from functionality changes
-------------------------------------------------------
I completely agree about separating functional commits from formatting commits. This is an unusual patch. The formatting and functionality changes are interrelated. This patch adds "mysqli" to the many places where only "mysql" was referenced. To accomplish this, I put all DBMS specific items inside switch statements. Putting items inside switch statements necessitates indenting by two spaces. I took advantage of the nesting changes to make the nesting consistent.

removing the utf-8 short-circuit
------------------------------
You're right... for 99% of the users. But there are always edge cases. Why is the short-circuit in there? To improve efficiency of the upgrade process? If so, saving a few seconds in the upgrade process, which only gets run once, isn't worth causing problems for the edge cases.

critical
------
I marked this critical because the existing upgrade system won't work for users who import their old database to a mysqli based server and then run the upgrade process. In addition, the UTF-8 edge cases will burn some users. Finally, because it seemed best to get this committed and tested before a release candidate goes out.

4.5 -> 4.7 directly
-----------------
Allowing upgrades from 4.5 to 4.7 direclty is the way the system exists now, plus it's the right thing to do.

moshe weitzman’s picture

moshe weitzman commented

danielc - i suggest you communicate privately with dries and steven and neil drumm about this patch. you will have to convince them at some point and it is better to just gather their objections up front and rebut or address them. this patch is in danger of being discarded because of perceived risk.

Cvbge’s picture

Cvbge commented

The patch has many changes which are cosmetic only... simply adding -wB switch to diff decreases size from 1742 lines to 1275.
Frequent cosmetic change is moving closing parenthesis from trailing line to last line in CREATE TABLE statesments.
Changing doxygen comments inside functions to normal comments makes the patch bigger too.
You don't really need to move the 'pgsql' block after the 'mysql' in switch statesment...

If you didn't include those changes and also added mysqli support not by using switch statement but by adding or ... inside the if, the patch would be very easy to review and I'm sure it'd be commited quickly...

I didn't reviewed the patch seriously, but after skimming it I've found this change, which seems dubious:

-      PRIMARY KEY (vid))");
+        PRIMARY KEY  (nid,vid))");

Also, you're still using $GLOBALS['db_version'] instead of db_server_version (you probably just didn't sent updated patch, but wanted to note this just in case).
And you should assume Postgresql can drop columns, so in some cases you should drop the checking completly.

I'm sorry, but I'd be against applying this patch in such form.

danielc’s picture

danielc commented

The patch has many changes which are cosmetic only... simply adding -wB switch to diff decreases size from 1742 lines to 1275.

Creating then applying such a patch would leave the nesting a complete mess. These nesting
tweaks were put in this patch because they end up changing the same lines without changing functionality. So it seemed best to do it in one patch rather than clutter the history/diffs/etc.

Frequent cosmetic change is moving closing parenthesis from trailing line to last line in CREATE TABLE statesments.

These minor changes are generally part of the nesting and case statement order changes.

Changing doxygen comments inside functions to normal comments makes the patch bigger too.

The original author didn't intend for them to be Doxygen comments, they just used a "fancy" commenting technique.

You don't really need to move the 'pgsql' block after the 'mysql' in switch statesment...

As mentioned in the initial comment, this was done to institute consistency in the file to improve ease of maintenance and readability in the long run.

I didn't reviewed the patch seriously, but after skimming it I've found this change, which seems dubious:
- PRIMARY KEY (vid))");
+ PRIMARY KEY (nid,vid))");

This is merely due to changing the the order of the mysql/mysqli and pgsql cases in the switch. The diff makes it look like an SQL change when it's not.

Also, you're still using $GLOBALS['db_version'] instead of db_server_version (you probably just didn't sent updated patch, but wanted to note this just in case).

Ugh. Good catch. Thank you. I'll revise that later.

And you should assume Postgresql can drop columns, so in some cases you should drop the checking completly.

I added the version checks for two reasons. First, this allows the patch to be backportable to earlier branches. Second, the website (http://drupal.org/node/270) says users are generally able to use 7.2. I figured it's no big deal to put them in to be safe rather than sorry.

As suggested by Moshe, I'll chat with the committers to see what's the most palatable way to get these changes in.

markus_petrux’s picture

markus_petrux commented

The incoming Drupal installer is about to add functions to get the DB engine version (see Installer for Drupal Core and requirements checking in core). I would suggest to share the same code, if possible.

danielc’s picture

danielc commented
StatusFileSize
new updates.inc.218-219.diff1.58 KB

I am in the process of splitting the patch into individual steps. The first step:

"Have pgsql always drop columns. (Dropping columns was added in pgsql 7.3, not 7.4.)"

danielc’s picture

danielc commented
StatusFileSize
new updates.inc.219-220.diff37.34 KB

Step two:

"Ensure 'mysqli' is always checked for when running DBMS specific code. (Accomplished by putting DBMS specific code inside switch statements.)"

This is necessary because even though mysqli is only supported in 4.7, users can import a 4.5 database to a mysqli based server then run the update process.

drumm’s picture

drumm
he/him
Location NY, US
commented
Status: Needs review » Needs work

Database updates earlier than 4.6 (#131 and less) should not be modified unless there is a critical bug. Adding compatibilty for a database engine we added support for later is not a critical bug since people should be running their Drupal version under the requirements for that version, not mixing and matching with future versions. If any documentation is needed to make this clear, then that should be added instead.

danielc’s picture

danielc commented
Version: x.y.z » 4.7.x-dev
Status: Needs work » Closed (won't fix)

The developers and I disagree on how to make Drupal better.