views_update_6000 causes errors

Adding error messages for searchability:

Update 6003:
user warning: Duplicate key name 'name' query: ALTER TABLE views_view ADD UNIQUE KEY name (name) in [...]/includes/database.mysql-common.inc on line 403.

Update 6006:
warning: Table 'cache_views_data' already exists query: CREATE TABLE cache_views_data ( `cid` VARCHAR(255) NOT NULL DEFAULT '', `data` LONGBLOB DEFAULT NULL, `expire` INT NOT NULL DEFAULT 0, `created` INT NOT NULL DEFAULT 0, `headers` TEXT DEFAULT NULL, `serialized` SMALLINT NOT NULL DEFAULT 1, PRIMARY KEY (cid), INDEX expire (expire) ) /*!40100 DEFAULT CHARACTER SET UTF8 */ in [...]/includes/database.inc on line 517.

I think this views.install file needs a separate views_schema_6000() which defines the schema as it was originally, so that all of the subsequent updates can apply cleanly. This then would be the one used by views_update_6000().

Actually, looking at the comment for views_schema_1(), this may be precisely what this function was intended for: "Views 2's initial schema; separated for the purposes of updates."

If I'm reading this rightly, the problem is that views_schema_1() was subsequently modified when it should never have been.

views_schema() should possibly have started out as a replica of views_schema_1(), rather than a function which returns it?

Revision 1.37 was where this split originated, so I've made a patch (against 6.x-2.6) which uses that version of views_schema_1() and uses the 6.x-2.6 version of views_schema_1() as views_schema()

Modified views_schema_1() as per revision 1.44 (see http://drupal.org/node/357368 )

The differences between views_schema_1() and views_schema() are now exactly what the hook_update function have changed, so I think this patch is good.

That seems kinda verbose (and possibly error-prone), to be honest. To my mind that's what CVS is for (if the changes in question aren't clear from the hook_update functions).

OTOH I don't spend any time with this code base, so I won't pretend my opinion counts for much here :)

Let me know if you'd like a new patch adding the additional schemas.

Oh I see. Yes, that makes sense to me now :)

Patch attached.

Possible improvement: Use something along the lines of install.inc's drupal_get_schema_versions() to automatically determine the latest views_schema_(n) in hook_schema()

/**
 * Implementation of hook_schema().
 */
function views_schema() {
  $views_schema = views_current_schema();
  return $views_schema();
}

/**
 * Find the current version of the database schema.
 * @return
 *   The name of the views_schema_<n> function, for the highest available <n>.
 */
function views_current_schema() {
  $schemas = array();
  $functions = get_defined_functions();
  foreach ($functions['user'] as $function) {
    if (strpos($function, 'views_schema_') === 0) {
      $version = substr($function, strlen('views_schema_'));
      if (is_numeric($version)) {
        $schemas[] = $version;
      }
    }
  }
  sort($schemas, SORT_NUMERIC);
  return 'views_schema_'. array_pop($schemas);
}

Here's the patch with this integrated, if it's considered useful.

It occurred to me that I could also remove the requirement for each schema update function to explicitly name the previous one as a starting point, if I simply statically cached the sorted array of updates. Each call would then pop the correct function name off the stack.

I'm attaching a couple of new patches. The first is the simpler version, which works just fine if all we want is the latest hook_schema() or the original hook_schema_6000(), but which breaks if any other specific version is asked for.

I'm not really sure if that's ever going to happen, but I implemented another variant which resolves the problem by passing __FUNCTION__ to hook_schema(), so that it knows where to begin.

I've updated my last patch to account for the changes added in 6.x-2.8.

O_o and bricestacey: Please test this patch, and set to RTBC if it looks good to you! I don't think this is likely to get committed otherwise.

In general, the module maintainer(s) will commit a patch if they personally agree that it is a good patch, and it has been reviewed and tested by the community. A maintainer is far more likely to look at a patch in the first place if it is already RTBC, but they still need to find the time to do so in amongst all the other issues on their plate.

So hopefully merlinofchaos finds some time to have a look at this before the next release. I doubt that there's anything more to do in the meantime.

I reverted views_schema_6000() (previously views_schema_1()) back to its original state (including any errors) and made all the subsequently-committed fixes and changes to the schema in the views_schema_6xxx() functions.

views_schema_6003() (and views_update_6003()) fixed this particular error:

function views_schema_6003() {
  $schema = views_schema(__FUNCTION__);
  $schema['views_view']['unique keys'] = array('name' => array('name'));
  unset($schema['views_view']['unique key']);
  return $schema;
}
function views_update_6003() {
   $ret = array();
   db_add_unique_key($ret, 'views_view', 'name', array('name'));
   return $ret;
}

This prevents the following error when updating from Drupal 5 (which applies all views_update_N()s to views_schema_6000 specifically)

Update 6003:
user warning: Duplicate key name 'name' query: ALTER TABLE views_view ADD UNIQUE KEY name (name) in [...]/includes/database.mysql-common.inc on line 403.

For fresh installations, calling views_schema() returns the final version of the schema, with all views_schema_6xxx() fixes applied in sequence (as per comments #5 and #7).

Additional comments added :)

New version because I missed a word in one of the comments, and I can't edit the previous attachment now.

Excellent.

I'll try to have a look at the 7.x branch some time. I've not spent much time with 7.x, but it sounds like it should be straightforward.

It looks like there's an issue with the 6.x-3.x branch, however, as that contains a new views_update_6008() (committed to 3.x in #593668: Ability to change display ids) which needs to be dealt with.

Here's a patch for that.

I'm not sure whether you mean a new issue for this 6.x-3.x patch (which will also be relevant to the 7.x-3.x version), or a new issue for the 7.x port?

A single issue made sense to me, but I don't mind if it's split, so long as they're clearly marked as related. I'll leave it to someone else to create the new issue, though.

Which patch was ported? (Did you miss part of your reply?). I think I may be losing something in the translation, sorry :/

I'm pretty sure you're saying that the new issue should be for the new 6.x-3.x patch, but FWIW I did have a reason for putting that here.

Given the extra hook_update_N() in 3.x, my preference would have been to commit the patch from #23 only to the 6.x-2.x branch, then port it to 6.x-3.x, and then port that to 7.x-3.x. That's why I reverted the version back to 6.x-3.x in #25/26, so that we could follow that sequence, and it seemed sensible to do it all in the one issue.

If you're thinking of fixing 7.x-3.x first and then back-porting to 6.x-3.x, then I can see why what I did looks a bit backwards, but given that 6.x-2.x and 6.x-3.x are virtually identical as far as this file is concerned, it seemed easier to port the patch forwards.

I am also wondering whether the numbering scheme is going to cause issues between the 2.x and 3.x branches? Currently we have a views_update_6008() which is only in 3.x, so what happens the next time there's an update for 2.x? Will that also be numbered 6008? Or if it applies to both branches, will it be 6008 in 2.x and 6009 in 3.x? Or do we skip 6008 in 2.x, and try to keep the numbers synchronised across both branches?

The hook_update_N docs indicate that updates for 6.x-3.x should be numbered 63xx. If updates that need to be added to both branches could end up out of sync anyhow, should we look at using the more explicit 62xx and 63xx numbering?

Well I'm back to being unsure about precisely what you are saying. Please go ahead and create the new issue according to your requirements, and I'll be happy to follow it up. Make sure you link to it in this issue so I can find it, though.

Thanks for the clarification, and here's a patch for 7.x-3.x

I haven't tested this. I compared the 7.x-3.x views.install with the 6.x-3.x version in both the pre-patch and post-patch states, though. The only code changes between the two were changing $GLOBALS['db_type'] to db_driver(), and while ($block = db_fetch_object($result)) to foreach ($result as $block)

I also changed the "Implementation of hook_" wording to "Implements hook_", as that appeared to be the standard in Views 7.x (although I couldn't see any mention of this elsewhere in Drupal land... it that just a local convention?)

Once this is committed, a 7.x-3.x patch for #466250: Enlarge views_display.display_options can be made.

Patch updated

I'm not too familiar with Drupal 7, so hopefully I'm not missing anything here, but it looks as if upgrading from previous versions of Drupal is not yet a concern in this version (the schema versions are still synchronised between 6.x and 7.x), so I think the main question is: Does views_schema() return the same array that it did pre-patch?

This could be compared and verified manually without very much effort (the output should be equivalent, although some of the children may end up in a different order, so you could use a recursive ksort before diffing the output). Alternatively, rename the functions, load both files, and check that old_views_schema() == new_views_schema() -- PHP doesn't care which order the elements are in unless you use ===.