Regression: can't get query results on install/uninstall [#190899]

while updating one of my modules to drupal 6, i noticed a slight regression due to the schema API -- i have no way to get feedback about the queries being run on install/uninstall. in drupal 5 i could just $q1 = db_query('blah') to get that info.

i use this info to print a 'success' or 'failed' message related to the module install or uninstall.

what do you think about returning $ret from drupal_install_schema() and drupal_uninstall_schema()? should be able to inspect that to get the necessary info.

Comment	File	Size	Author
#5	schema-install-query-results.patch	1.62 KB	hunmonk
#3	schema-install-query-results.patch	1.74 KB	hunmonk
#1	schema-install-query-results-190899-1.patch	1.53 KB	bjaspan

Comments

Comment #1

bjaspan commented 9 November 2007 at 23:16

Status	File	Size
new	schema-install-query-results-190899-1.patch	1.53 KB

I think you meant $ret = update_sql($query) not $ret = db_query($query). db_query never returned the 'success' or 'failed' thing.

All Schema API functions generate the update_sql() output array and, in fact, drupal_install_schema() and drupal_uninstall_schema() actually collect them (they have to since the API functions require the argument). They do not yet return anything, so it is easy enough to return them.

Patch attached.

Comment #2

bjaspan commented 9 November 2007 at 23:16

Status:

Active

» Needs review

Comment #3

hunmonk commented 10 November 2007 at 18:14

Title:

Regression: can't get query results on install/uninstall

» Regression: install/uninstall deficiencies in Schema API

Status	File	Size
new	schema-install-query-results.patch	1.74 KB

couple of corrections:

$ret is actually an array of arrays, so i corrected the doxygen appropriately
we really should be testing for the existence of a table before we drop it on uninstall, so i've added that. normally i'd roll another patch for that, but it's a trivial change which affects the very same code. tested it and it works perfectly.

FYI, here's how i'm using this:

function asterisk_install() {
  $ret = drupal_install_schema('asterisk');

  $failed = array();
  foreach ($ret as $query) {
  	if (!$query['success']) {
  	  $failed[] = $query['query'];
  	}
  }
  if (empty($failed)) {
    drupal_set_message(t('Asterisk module installed successfully.'));
  }
  else {
    drupal_set_message(t('Table installation for the Asterisk module was unsuccessful. The following queries failed: !queries', array('!queries' => theme('item_list', $failed))), 'error');
  }
}

this allows me to give the user a bit of intelligent output about the module installation.

actually, in the old days, i would just do $q1 = db_query('blah'), as i indicated above, which would at least give me a TRUE/FALSE on the query. AFAICT, we always used db_query(), not update_sql(), for initial install queries, even in core.

Comment #4

bjaspan commented 10 November 2007 at 18:48

You say "We really should be testing for the existence of a table before we drop it on uninstall" but I'm not convinced. We don't check if a table exists before creating it, and we don't check if the schema matches our expectations before we alter it. Our entire schema maintenance system is built on assuming things are the way we left them. I don't see much harm in checking if a table exists, and I'm not concerned about the extra query. On the other hand, avoiding a "DROP TABLE failed because table does not exist" error is also not that important and, I'd even say, getting such an error when it should not occur provides useful information to the administrator. So, I'd vote for not checking before db_drop_table().

Note that "assuming things are the way we left them" is absolutely on the table for discussion for D7. Schema.module already knows how to inspect the database and compare it to hook_schema results. This can lead us towards automatic schema updates instead of hand-written hook_update_n() functions, at least for many common cases. However, I'd still say that we can/should assume that any table in hook_schema() exists and if it doesn't report an error.

Incidentally, making hook_install() unnecessary for modules that do nothing but drupal_install_schema() is also on the table for D7. The central hook_install replacement should probably report on failed table creation as you are doing in asterisk.module; there is no reason for that code to be duplicated among many modules.