Destroy remnants of update_sql()
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | update system |
| Category: | task |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed |
This is a follow up to #394268: DIE update_sql() DIE!
Now that update hooks have a new API (see previous issue), we have some follow up work to do. webchick already said this is safe to do during code slush as it's a continuation of an active issue, but we should still do it quickly.
1) Update all core update hooks to the new API. (Do nothing on success, return translated message on success, use $sandbox parameter for batch operations, throw an exception in case of error.)
2) Update Schema API to remove the stupid $ret array that serves no useful purpose anymore. This is an API change to the DB classes and to the wrapping functions. It also means that the schema classes can just call $this->connection->query() directly and do not need to use update_sql() anymore.
3) Delete update_sql() entirely.
I'll look into this after I get back from vacation if someone doesn't beat me to it. :-) webchick said she wanted it all in one patch, but I am also open to a multi-step process if it makes things easier.
#1
Oops, just created #570910: Return meaningful messages from update hooks for 1)
#2
OK, I hope to God this works. :-) This is the all-in-one patch. $ret is gone. update_sql() is gone. All update hooks in core should be updated. While I was at it I also converted a lot of single-inserts to multi-inserts, since we didn't have to use update_sql() anymore.
I'm not removing the BC parts of the update system yet. Convert stuff first. This patch is huge enough as is. It's also probably very fragile. Please review soon so that it doesn't break and force me to redo it. :-)
#3
The last submitted patch failed testing.
#4
Let's try to get them all this time...
#5
The last submitted patch failed testing.
#6
OK, $ret, you and me, behind the gym after school. You're goin' down, boy...
#7
The last submitted patch failed testing.
#8
OK, I give, I cannot replicate these exceptions. Those tests run perfectly on my system. Anyone want to help here?
#9
The update field tests are attempting to pass array() to db_drop_table().
field_sql_storage_field_storage_update_field() still has a couple of db_ function calls with $ret.
#10
Huh? Where? I don't even see such a function. Got line numbers?
#11
field_sql_storage_field_storage_update_field() was recently added in this commit: http://drupal.org/cvs?commit=267832 to modules/field/modules/field_sql_storage/field_sql_storage.module
Can't update the patch from here (firewall restriction), but the changes for field_sql_storage_field_storage_update_field() are:
@@ -231,17 +231,15 @@ function field_sql_storage_field_update_forbid($field, $prior_field, $has_data)* Implement hook_field_storage_update_field().
*/
function field_sql_storage_field_storage_update_field($field, $prior_field, $has_data) {
- $ret = array();
-
if (! $has_data) {
// There is no data. Re-create the tables completely.
$prior_schema = _field_sql_storage_schema($prior_field);
foreach ($prior_schema as $name => $table) {
- db_drop_table($ret, $name, $table);
+ db_drop_table($name, $table);
}
$schema = _field_sql_storage_schema($field);
foreach ($schema as $name => $table) {
- db_create_table($ret, $name, $table);
+ db_create_table($name, $table);
}
}
else {
@@ -253,8 +251,8 @@ function field_sql_storage_field_storage_update_field($field, $prior_field, $has
foreach ($prior_field['indexes'] as $name => $columns) {
if (!isset($field['indexes'][$name]) || $columns != $field['indexes'][$name]) {
$real_name = _field_sql_storage_indexname($field['field_name'], $name);
- db_drop_index($ret, $table, $real_name);
- db_drop_index($ret, $revision_table, $real_name);
+ db_drop_index($table, $real_name);
+ db_drop_index($revision_table, $real_name);
}
}
$table = _field_sql_storage_tablename($field);
@@ -266,8 +264,8 @@ function field_sql_storage_field_storage_update_field($field, $prior_field, $has
foreach ($columns as $column_name) {
$real_columns[] = _field_sql_storage_columnname($field['field_name'], $column_name);
}
- db_add_index($ret, $table, $real_name, $real_columns);
- db_add_index($ret, $revision_table, $real_name, $real_columns);
+ db_add_index($table, $real_name, $real_columns);
+ db_add_index($revision_table, $real_name, $real_columns);
}
}
}
#12
Figures, that patch was committed *as I was working on this one*. Fail!
Let's try this one now...
#13
quick, someone commit this before it breaks :D
#14
I really want to commit this, but unfortunately the system.install hunk no longer applies. :( I can haz reroll?
#15
Would you people please stop committing things while I am writing gigantic patches?
I have no idea what was wrong with the last patch, but here it is again. Probably whitespace somewhere the broke stuff. Yar.
Bot, you like?
#16
Looks good -- less is more. Committed to CVS HEAD. Thanks!
#17
Automatically closed -- issue fixed for 2 weeks with no activity.
#18
This has been documented. Just sayin'.
#19
In past we haven't used
t()in hook_update for the simple reason that translatable strings are not yet imported while running the update hooks and therefore never shown translated. Anything changed about this behaviour?#20
Is there a reason why system_update_N() all still have '$ret's all over the place ?
#21
Hm. That's a really good question. It looks like they're all in 60xx update hooks. I know I did a find for "$ret =" in all of Drupal when rolling the patch above and killed them all, even those not in update hooks just to be sure. There's even some update_sql() calls in there! Did someone add back in old update hooks or something? I know I'm not THAT oblivious...
Looks like we've still got some cleanup to do. *sigh*
#22
It looks like what happened is someone forward-ported the updates added to Drupal 6 post-6.0's release, and didn't update them for the changes to the update API. This should take care of it.
It also removes the BC code from update.inc that we don't need anymore.
#23
The last submitted patch failed testing.
#24
Grumble grumble grumble.
#25
Changes look good. Besides deleting the strict-BC code in update_do_one, we could also rework the rest of the function that is still globally designed around the old format.
$ret['results']['query'] = $function($context['sandbox']);$ret['results']['success'] = TRUE;
Not critical, though.
#26
Committed to CVS HEAD. Thanks!
#27
Automatically closed -- issue fixed for 2 weeks with no activity.