drupal_get_schema_unprocessed() called in update functions

Tagging.

So this means the schema of the cache table needs to be duplicated to both image_update_7000 and update_fix_d7_requirements.

1) My first idea was to add a function that returns the cache schema to reduce code duplication. The cache schema would still be defined twice: once the "original" version, and once as the "current" version in system.install. The original version should then never be changed.

2) If we don't add the separate function the cache schema is defined three times, but there is a function call less and there is no chance of accidentally changing the "original" schema when the cache table definition is updated.

3) A completely different approach might be useful because the cache tables are a bit special in the sense that they always have the same schema:
- store a list of the cache tables in a variable 'cache_tables'
- use a function drupal_cache_bin_add($bin) which adds the cache table to 'cache_tables' and returns the schema, and a function drupal_cache_bin_delete($bin) for modules that would like to remove their cache table.
- when updating the cache schema (for example in system_update_7054), automatically update the tables in 'cache_tables'.
Using this approach the core/contrib modules that define an extra cache bin don't have to worry about changes to the cache schema.

Let's define system_schema_cache_7000() in system.install, following the pattern introduced by Earl in his modules. I think that's what you mean in 1)?

Lets get this started then. The patch creates a function system_schema_cache() and a function system_schema_cache_7054(), the described option 1. Thanks to merlinofchaos for the pattern.

The number is 7054 for consistency: system_update_7054 introduces the first schema change for the cache tables, and thus it is the first schema version in D7. As there hasn't been a D7 release yet, we don't need to worry about modules creating a cache bin based on an a version before system_update_7054.

+function system_schema_cache($caller_function = FALSE) {
[... a big pile of code ...]
+}

Any reason not to call this function system_schema_cache_latest() and just have it:

function system_schema_cache_latest() {
  return system_schema_cache_7054();
}

I don't see any compelling reason not to manage this stuff manually.

hmm, not really no. I was just following the pattern from the views module, but on a second look it does look a bit overkill. And the chances of the cache tables changing during the D7 release would be minimal probably.

Patch looks fine except this hunk is inconsistent with the rest of core, I think we should keep this change just to update functions, at least for now. RTBC with a re-roll though.

-  $schema['cache_image'] = drupal_get_schema_unprocessed('system', 'cache');
+  $schema['cache_image'] = system_schema_cache_latest();

Reroll with comments from catch.

Looks great.

+++ modules/system/system.install	24 Jun 2010 04:10:29 -0000
@@ -1580,6 +1539,78 @@ function system_schema() {
+ *
+ * @param $caller_function
+ *   The name of the function that called us.
+ *   Used internally, if requesting a specific schema version.

That parameter doesn't exist anymore.

+++ modules/system/system.install	24 Jun 2010 04:10:29 -0000
@@ -1580,6 +1539,78 @@ function system_schema() {
+ *
+ * Updates to the cache schema must be provided as system_schema_cache_7xxx() functions,
+ * and system_schema_cache_latest() shoud then be updated with the new function.
+ * See views.install for examples on how to write such functions in a short way.

The first line doesn't wrap at 80 chars. Also I don't think we should be referencing views.install from core.

Powered by Dreditor.

Otherwise looks good!

Reroll with correct comment wrapping and removed the stale parameter comment.

I don't really see a problem to reference views.install, it's merely providing credit for the idea :)

guess this should be set to needs review again; Only comments are wrapped up, and the stale parameter is removed.

I don't understand why you are doing this. All over core, we have the pattern that the first time a new table is introduced, the schema is put into the hook_schema() and into the hook_update_N().

I realize it causes duplication, but I think this method of having hook_schema() call system_schema_cache_latest() is NOT a good pattern to start, especially the bit where system_schema_cache_latest() is currently calling system_schema_cache_7054().

This is not the pattern of the rest of core. Just use the common pattern.

If you do want to start this pattern, then you should modify all the other core modules to use this pattern as well.

@jhodgon: Could you explain, why you think that's not a good pattern? I think i's quite useful and it makes the problem in the OP less likely.

a) You haven't supplied a patch that changes the pattern for all the other Drupal modules. Consistency is good.

b) It is very useful to be able to go to coremodulename_schema() on api.drupal.org and be able to actually see the schema(s) of all the tables defined by that module. I do that at least once every week or two. All core modules and contrib modules currently follow this pattern, and have for many versions of Drupal. Again, consistency is good.

c) This pattern suggstion has not been discussed as a new coding standard by the community at large (I'm willing to be convinced, but it needs a broader discussion).

d) WTF -- the fact that you felt the need to add this HUGE WTF docblock should be an indication that it was not a great idea.

+/**
+ * The initial cache schema.
+ *
+ * Important: Do not edit this schema!
+ *
+ * Updates to the cache schema must be provided as system_schema_cache_7xxx()
+ * functions, and system_schema_cache_latest() shoud then be updated with the
+ * new function. See views.install for examples on how to write such functions
+ * in a short way.
+ *
+ * Please do document updates with comments in this function, however.
+ */
+function system_schema_cache_7054() {

e) You now have calls to system_schema_cache_7054 sprinkled all over the place. If the schema changes, will all these calls need to change?

First of all: the cache schema is a bit special in the sense that there are many tables which need to have the same schema, just with another table name.
The pattern is retrieved from the views module, as referenced in the quoted docblock.

a) I think this patch does change all places in core where the cache table schema was called in an update function.

b) While I agree that this is very useful, it would require duplicating the cache schema at all these places. In addition, when the schema changes, all these places have to be updated. Also, the hook_schema impl in other modules have always retrieved the schema from system_schema.

c) Agreed that this hasn't been used in core before, but it has been used in contrib. So lets discuss this :)

d) It might not be the best idea, but at least it doesn't change an API or something. The original version of the patch provided a function that dynamically found the latest version of the schema, but that might be a bit overkill.

e) No, these calls should remain the same. All modules implementing a cache table do have to add an update function when the cache schema changes. Remember that this function is only called in hook_update_N, where the version from the time of writing the update function needs to be used.

I personally would prefer going with the third option of #2, but that would introduce an API addition. (btw, this wouldn't affect current contrib implementations, as a module can still choose to manually keep up with the changes in the cache schema). This could be something for D8 perhaps then? I'm fine using any of the three methods in #2 though, or even some other method :)

I think system_schema() should define the cache tables as normal. But we should keep the system_schema_cache_7054() helper for the update functions - since it's called in so many places in core and in contrib.

That way we keep a handy helper function, but that code is only ever called by update handlers rather than also indirectly via system_schema(), and the schema is only duplicated once. Then we can change the docs to just make it clear it's a helper function for updates and skip the 'do not change this'. If we do that, it doesn't feel like an API or pattern change, this is the first time I can remember a schema update to cache tables, and we have a lot of them.

This patch defines the cache schema directly in system_schema, and gets rid of system_schema_cache_latest.

I think this patch (and the previous patches as well, for that matter) still pose some problems in case the cache schema is changed:
For example when a new column is added:
The user installs the new version of drupal, but some of the modules that created a cache bin are not updated yet. When the not-updated bin is used, an error occurs (column not found).

The same goes for the other way around, when a column is removed, and the user updates the module which defined a cache bin, causing the column to be removed from that cache bin, before updating drupal core. The cache system still tries to use that column and a similar error as above occurs.

edit: these examples are for contrib modules, as modules in core would be updated together with the cache system.

back to needs review.

OK that's what I was looking for, can we tweak the comment a bit though?

+/**
+ * The cache schema after system_update_7054.
+ *
+ * Important: Do not edit this schema. Instead, when the cache schema is
+ * updated, a new system_schema_cache_7xxx() function must be provided. See
+ * views.install for examples on how to write such functions in a short way.

How about:

+/**
+ * The cache schema corresponding to system_update_7054.
+ *
+ * Drupal 7 adds several new cache tables. Since they all have identical schema
+ * this helper function allows them to re-use the same definition, without
+ * relying on system_schema(), which may change with future updates.

I don't think we can fix the contrib module issue here, just need to clean up the core ones.

OK, so we only fix the code duplication in core here. Contrib modules can also use this helper function, but must add extra updates when the cache schema is updated. I've open a new issue for the problem on updating core and contrib modules when the cache schema changes: #837342: Simultaneous updating required for modules with a cache bin when the cache schema changes

Looks great. I think jhogdon was only worried about the pattern rather than using a helper, also the whole point of this is we never update those functions calling the versioned schema because it's supposed to stay frozen.

Drop/Create of cache tables used in #690746: Text column type doesn't reliably hold serialized variables

Committed to CVS HEAD. Thanks.