Clean-up the upgrade path: node

Marking "needs review" for testbot.

I'm not sure I agree that these are critical unless they're known to cause an actual bug in the wild.

#7: 898558-upgrade-cleanup-node.patch queued for re-testing.

I don't think this is critical (see @webchick's comment above). Mostly looks like a good cleanup, though.

           // This is a core update and no contrib modules are enabled yet, so
           // we can assume default field storage for a faster update.
-          field_sql_storage_field_storage_write('node', $node, FIELD_STORAGE_INSERT, array($body_field_id));
+          _update_field_sql_storage_write('node', $node->type, $node->nid, $node->vid, 'body', $node->body);

The above gives me pause, though. The existing code was already something of an update-specific hack, and now we are replacing it with a whole duplicate function that is also update-specific? As far as I know, there is no core code that would break if field_sql_storage_field_storage_write() is called during the update - is that correct? And our UPGRADE.txt instructions already tell people to disable all contrib modules on their site before running the D6->D7 upgrade.... so contrib modules shouldn't have a chance to respond to these hooks anyway, and thus, nothing will break there either.

I think we should tread carefully before adding too many of these update-specific duplicate functions to core. They are sometimes necessary, but perhaps we should make sure that each one definitely is necessary before adding it.

+++ modules/field/field.install
@@ -274,6 +274,63 @@ function _update_7000_field_create_instance($field, &$instance) {
+ * This function is valid for a database schema version 7000.

What does this mean? Can we clarify that comment?

Powered by Dreditor.

the #898520: Clean-up the upgrade path: comment goes in.

@17, yes

+++ modules/field/field.install	2010-09-13 09:44:59 +0000
@@ -171,7 +171,7 @@ function field_schema() {
  * Utility function: create a field by writing directly to the database.
  *
- * This function is valid for a database schema version 7000.
+ * This function is valid for field module database schema version 7000.

+++ modules/field/modules/field_sql_storage/field_sql_storage.install	2010-09-13 09:46:52 +0000
@@ -44,3 +44,80 @@ function field_sql_storage_schema() {
+ * Utility function: write field data directly to the SQL storage.
+ *
+ * This function is valid for field_sql_storage database schema version 7000.

Sorry, but I'm still uncertain about what these comments are effectively trying to teach me.

- It looks like the functions can be "invalid" (probably rather their usage, not the function itself), but it's not entirely clear when it's valid and when it's invalid.

- I guess I get that I should not be using those helpers prior to reaching the stated schema version. Does my code have to implement hook_update_dependencies() to actually enforce and ensure this?

- It's not clear to me whether the helper functions are still valid after the stated schema versions. The current commment makes it sound as if they would be only valid for 7000 and only valid for exactly that schema version, not more, not less. I guess that isn't the intended meaning, but honestly, I don't know, because I didn't work on these issues and didn't come to the conclusion that we require those helpers.

Powered by Dreditor.

this is the last beta blocker..

int wrote:
> this is the last beta blocker..

Not yet. I was also following the beta blocker tag, but I think the beta blocker list at Drupal core improvements has precedence over the tag, and some critical upgrade issues were added to the list a few days ago.

Even more beta blockers would be added according to a strict interpretation of the criteria mentioned on the list. Details could be discussed at Criteria for Drupal 7 beta?

The update function changes and the new helpers look good. My only comments are about the comments. I'm quite convinced we need these new helpers, but they're a totally new idea, and they need as clear an explanation as we can come up with.

+++ modules/field/field.install	2010-09-13 09:44:59 +0000
@@ -171,7 +171,7 @@ function field_schema() {
- * This function is valid for a database schema version 7000.
+ * This function is valid for field module database schema version 7000.

I agree with sun this could be clearer. Maybe something like "This function can be used for databases whose schema matches field module version 7000."

Another possibility is ..."is at field module version 7000 or higher." The "or higher" part is a bit dangerous, because we'd have to revisit it if we change the schema before we dump the update system in D8. But I bet that doesn't happen.

+++ modules/field/modules/field_sql_storage/field_sql_storage.install	2010-09-13 09:46:52 +0000
@@ -44,3 +44,80 @@ function field_sql_storage_schema() {
+  // field_update_7000(). So this function needs to exist but as
+  // field_sql_storage is a new module in Drupal 7, it does not need to do
+  // anything.

Instead of the sentence starting "So this function needs to exist..." I might say "This function exists solely to set the schema version to 7000." But if anyone disagrees I won't insist - it's probably fine as is.

At any rate, these are small nitpicks, and I'll RTBC a reroll.

Powered by Dreditor.

Oh, and yes, ideally you would implement hook_update_dependencies to make sure field_sql_storage is at 7000 before using these helpers. But since the field system is new, and will be in place before your updates run, it doesn't really matter.

"7000 or higher" works for me. It's like saying Windows 3.1 or later, you know eventually it's going to be invalid.

Okay, I rerolled it myself, with changes only to the comments (which you can see in the attached interdiff). Hopefully they clarify what's going on with the update helpers.

I'm going to be a pain in the rear and RTBC this even though I did the last reroll. I didn't touch the code, which is the important part here, and it is ready to go.

Committed to CVS HEAD. Thanks.