Upload -> File module upgrade path is broken

.

I need to do something similar in my module, so I definitely need this update to work.

+++ modules/system/system.install	2010-02-22 15:08:48 +0000
@@ -2224,28 +2224,46 @@
+  $result = db_query_range('SELECT f.fid, uid, filename, filepath AS uri, filemime, filesize, status, timestamp FROM {files} f INNER JOIN {upload} u ON u.fid = f.fid WHERE f.fid > :current GROUP BY f.fid ORDER BY f.fid', 0, $limit, array(':current' => $sandbox['current_fid']), array('fetch' => PDO::FETCH_ASSOC));

GROUP BY fid ? why ?

+++ modules/system/system.install	2010-02-22 15:08:48 +0000
@@ -2224,28 +2224,46 @@
+  else {
+    $sandbox['#finished'] = 1;
+  }

No biggie, but this part is not needed. #finished is assumed to be 1 if not explicitly set to a different value.

Powered by Dreditor.

Setting to 'needs work' based on yched's review.

The GROUP BY is to prevent the duplicate results that Damien talked about. We only care that there is a row in {upload} for each fid in {files}, not what's actually in {upload}.

And that makes me realize that these files aren't being attached to nodes anywhere. Unless there's something handling that elsewhere, then this update needs to make a filefield on nodes and put this data in it. That would make the GROUP BY problematic. Leaving as "needs work" until this gets resolved.

Patch updated from yched's comments, plus increased the limit to 500, since that's closer to what other batch updates have.

If this is an upload thing, why isn't it handled by upload.module?

It doesn't exist in D7.

needs bot review?

Well, even though it passed, the patch still needs work because a field for the legacy {upload} data needs to be made and that field attached to nodes.

Ugh.

Beginning brain dump because I can't finish the patch tonight:

Create file field called "uploads" or "legacy_upload" or something.
Make instances on every node type, unless I'm wrong about upload.module.
Loop through data in {upload}, joined to {files}.
Use field_sql_storage_field_storage_write() the way node.install migrated the body field.

Be careful how the batch is constructed, because you don't want to overwrite the deltas, considering that you have to make them up as you go along.

Alright. Now the node data is copied from {upload}. I tried to use the settings from upload.module where I could, but I don't think there's any equivalent to the role-specific settings.

+++ modules/system/system.install	2010-02-26 14:21:36 +0000
@@ -2224,28 +2224,121 @@
+        'object_types' => array('node'),

'object_types' is optional. Do we really want to make this field restricted to nodes ? Maybe we do, just asking. In this case, this might deserve a comment.

+++ modules/system/system.install	2010-02-26 14:21:36 +0000
@@ -2224,28 +2224,121 @@
+        'cardinality' => 0,

0 is not a standard value - should be FIELD_CARDINALITY_UNLIMITED ?

+++ modules/system/system.install	2010-02-26 14:21:36 +0000
@@ -2224,28 +2224,121 @@
+    $node_types = node_type_get_types();

Does this include node types defined in hook_node_info() by modules that are currently disabled (as is supposed to be the case for contrib modules when running a major core upgrade) ?

+++ modules/system/system.install	2010-02-26 14:21:36 +0000
@@ -2224,28 +2224,121 @@
+      if (variable_get('upload_' . $type->type, TRUE) && empty($instance)) {
+        ...
+
+        $instance = array(
+           ...
+        );
+
+        field_create_instance($instance);
+      }

Problem is, you might find files attached to nodes where upload was once enabled but got disabled at some point.
In the current logic, no instance would be created and the existing data would be ditched ?

+++ modules/system/system.install	2010-02-26 14:21:36 +0000
@@ -2224,28 +2224,121 @@
+    // We will convert filepaths to uri using the default schmeme

Typo: schmeme (already present in current HEAD)

Powered by Dreditor.

I don't see a real reason to restrict the field to just nodes. I was just trying to make it as similar to the way upload.module worked before.

It looks like _node_types_build() adds a "disabled" flag to those node types, but it still puts them in the return value. And since the upload_* variables default to TRUE, they should all have instances created.

Rerolled with fixes.

Oh. Just realized what you meant about enabling and disabling uploads. I don't know what ought to be done. I don't think it's a good idea to check if the instance should exist for every run through the batch. Maybe an instance should be made for every node type, and then disable some of them afterwards?

Is this tackling #563000: Provide upgrade path to file now? If so can that other issue be marked as dup (since there's no patch)?

Actually, there is a patch in #563000-47: Provide upgrade path to file. It just looks like everyone forgot about it, or they're just waiting on #211182: Updates run in unpredictable order.

dww's concerns (#64) about the filename munging still need to be addressed, however. I haven't heard of any way for fields to have role-specific settings, much less in core.

You can put whatever you like in the 'settings' key of a field definition, but it's very likely the UI doesn't support it at all.

tried #13 and got:
* Failed: SQLSTATE[23000]: Integrity constraint violation: 1052 Column 'uid' in field list is ambiguous

added explicit f. to all columns from the files tables. then got errors when trying to create a field of type file without the file module enabled, fixed that.

Now getting:

    * Failed: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'machine_name' in 'field list'

due to $field = field_info_field('files'); which I was not able to fix.

EDIT: the last error above might be coming from a messed up taxonomy update function. Also, worth noting, my upload and files tables still contain records, and my file table is empty :(
EDIT2: this error goes away after deleting the taxonomy module, and the file table does contain some records!

I guess that means needs work.

{file} was renamed {file_managed}
object_type was renamed entity_type

scor when did that get changed?

let's let the test bot run.

subscribe

@drewish: #741578: "File" is a reserved word that should not be used

Marked #563000: Provide upgrade path to file as duplicate. Note there's a working, if slightly stale patch at http://drupal.org/node/563000#comment-2243028

Hmm, weird. WTF with this duplicate issue re-doing all the work from #563000: Provide upgrade path to file? Should I just get out of the way and work off of comment #20 instead?

quicksketch uploaded a patch at http://drupal.org/node/563000#comment-2869832 both attempt to do the same thing, I'm not sure which is closest to working, but linking here for reference.

uploading quicksketch's patch here which is more advanced and working. rerolled with 'file' renamed to 'file_managed'. still need works as it gives me " * Failed: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '1' for key 'PRIMARY'" due to upload.fid not being unique.

use db_merge() instead of drupal_write_record() to ensure we only store each file once in file_managed.

+  // To make sure we process an entire revision all at once, toss the last node
+  // revision (which might be partial) unless it's the last one.

huuuh?

@chx: Since files are upgraded a bulk number of revisions at a time (say 500), it's possible that the group of 500 revisions may cause a cut-off half-way through a node. Say if there are 40 revisions of a node but 490 revisions have been processed by the time we get to that node in the loop. If we only had 10 revisions of the total 40, the node would not upgrade properly. So we toss off the last revision when doing the upgrades to ensure that we do the entire node all at once. This means that only way the upgrade would fail would be if some node had more than 500 revisions.

To make sure we process an entire revision all at once
s/revision/node/ then?
Also if we discard the last node in a batch, again, we're tossing the node, not the revision, from that batch.

At least those two clarifications would help, I think it'd be worth putting most of #30 into that comment to flesh it out a bit though.

Oh, I think I said the wrong thing as I was speaking from memory. We need to process all the "files within a revision" at once, not all the "revisions within a node". That's why we throw away the last revision on every batch of revisions, so that we're sure we have all the files uploaded from that revision all at once.

system_update_7053() and system_update_7054() taken, now moving to system_update_7055()

@Damien Tournoud
huh? I could not find a line like that in the patch... are you saying we need code to handle the case where it is == 0 .... and that in that case we *should not* drop the table, but currently the patch does drop it?

I think this might be needs work? but I'm not sure if others can tell what work is needed from Damien's review.

@YesCT, he means the condition is:

 if (count($context['types']) > 0)

So if you end up with exactly 0, then none of that code will execute last time around, and the table will be left in (for example if it was 1 previously).

+++ modules/system/system.install
@@ -2386,6 +2366,224 @@ function system_update_7054() {
+    if (count($context['types']) > 0) {

ah, so this should be >=
?

Powered by Dreditor.

+++ modules/system/system.install
@@ -2386,6 +2366,224 @@ function system_update_7054() {
+      // We're done: return without specifying a #progress.

In this case the upload module is enabled, there are no uploads and no node types that have uploads enabled. I guess we can safely remove the upload table then.

Powered by Dreditor.

Here's a new patch that addresses the comments from #34. Changes described in #38

+++ modules/system/system.install	21 Jun 2010 22:10:49 -0000
@@ -2404,6 +2384,227 @@ function system_update_7054() {
+  $query = db_select('upload', 'u');
+  $query->innerJoin('node_revision', 'nr', 'u.vid = nr.vid');
+  $query->innerJoin('node', 'n', 'n.nid = nr.nid');

Usually, db_select() should only be used if there is a reason to.

Reasons include:
- The query is dynamic
- It uses a extender (pager, tablesort, ...)
- It needs hook_query_alter(), for example, to check node access.

The reason is pretty simple: db_select() is quite a bit slower than a simple db_query() (multiple objects generated, lots of string handling, alter hooks, ..).

+++ modules/system/system.install	21 Jun 2010 22:10:49 -0000
@@ -2404,6 +2384,227 @@ function system_update_7054() {
+      ->fetch(PDO::FETCH_ASSOC);

->fetchAssoc() is a tiny bit simpler :)

+++ modules/system/system.install	21 Jun 2010 22:10:49 -0000
@@ -2404,6 +2384,227 @@ function system_update_7054() {
+    db_delete('system', array('type' => 'module', 'name' => 'upload'));

I don't know what syntax this is, but it's not valid :)

Instead, something like this should be used:

db_delete('system')
  ->condition('type', 'module')
  ->condition('name', 'upload')
  ->execute();

Also, it is probably not necessary at all since Drupal 7 cleans up the table automatically if a module doesn't exist.

Powered by Dreditor.

Reroll addressing comments from #40.

1) Thanks for the tip, didn't know that. Looks a bit readability vs performance.
2) Changed.
3) Can't believe I missed that syntax (I blame copy/paste).
4) Removed the delete from system queries. This is indeed done automatically, so we don't need to duplicate that here.

+++ modules/system/system.install	22 Jun 2010 09:22:38 -0000
@@ -2162,27 +2162,7 @@ function system_update_7034() {
+  // Update merged into system_update_7053().

I guess that should read 7055. Sorry, didn't see that in the previous review.

Powered by Dreditor.

Certainly. Btw, that number is the only change between these patches :)

Reroll because the patch didn't apply any more as a system_update_7055 was committed. Now concurring for getting system_update_7056.

Also left out the changes to node.install because the extra dependency isn't needed.

system_update_7056 just got committed, so going for 7057 now.

Reroll because system_update_7057 exists in HEAD now.

Did a quick test yesterday and this patch did get my D6-D7 update to complete. Haven't looked at the code carefully.

Took me forever to find this issue. Let's make it more sensible to normal humans who don't have the contents of each individual update function memorized, hm? ;)

- 7058 needs to be 7059 now, else you get a WSOD on update.php.
- Something still appears to be wonky with the upgrade path. My Drupal 6 site shows one file uploaded on a given node:

But the same node on my localhost Drupal 7 site shows no files uploaded. The file field gets successfully attached to the "Story" type, though, so that's a plus:

Updated patch, replacing FIELD_LANGUAGE_NONE with LANGUAGE_NONE as the former isn't defined anywhere.

This fixes the field data not being saved.

Excellent! I confirmed that on webchick.net this fixed my problems related to the upload upgrade path. YAY!!

Committed to HEAD. Although after I did, I saw this:

+        drupal_set_message('The content type <em>' . $row->type . '</em> had uploads disabled but contained uploaded file data. Uploads have been re-enabled to migrate the existing data. You may delete the "File attachments" field in the <em>' . $row->type . '</em> type if this data is not necessary.');

That should be changed to st() with placeholders, no?

Follow-up patch to change the string to use t. I've used t instead of st for consistency with other occurences of t in upgrades.

Awesome, thanks. I always forget where you use st() and where you use t(). ;P

Committed to HEAD!

Update 7055 is giving me issues in 7.x-dev (July 7) with sqlite. Please see http://drupal.org/node/847888

followup : #899582: D6 upgrade: Fix display settings for the 'file' instance replacing D6 upload.module

can anyone could post here the issue where the case of the {files} table is treated please ?
It's part of D6 system_schema() but I can't find where upgrade is handled in D7.

drzraf, It's up to each module to migrate their own files. Core handles files created by the upload module in D6 in system_update_7061().