system_update_7061() moves old files to {file_managed} table from upload.module and doing so it uses db_merge and preserves the fid from the old {files} table without regard to already existing records in {file_managed} with the same fid (which happens from over updates, e.g. user_update_7012. That overwrites some of the entries for the files if other modules make updates and migrate files to {file_managed} before system_update_7061 and causes DATA LOSS!

CommentFileSizeAuthor
#7 system-update-7061-user-module-data-loss-1882774-7.patch915 bytesDavid_Rothstein

Comments

chx’s picture

chx commented

Reading the code, I can tentatively confirm this although I have no idea really how the order gets set up but system 7061 depends on node 7001 and so I guess that causes some mess. A quick read of 7061 makes me think we could perhaps safely (?) renumber the files using db_next_id but I am not too confident in that.

Git archeology finds this was added back at #685892: Upload -> File module upgrade path is broken. The initial commit was on July 6, 2010. The first upgrade tests went in Jun 28, 2010. I guess it was not yet solidified that upgrade bugs needs tests but also this wasn't caught later with more upgrade tests because I guess we do not have uploads + user picture tests. That's just sad. I do not know a way out -- of course we can and will fix this but I do not think databases that suffered a data loss are salvageable? Just tell people to restore and restart once the fix is in?

catch’s picture

catch
he/him
Primary language English
commented
Issue tags: +D7 upgrade path

I do not think databases that suffered a data loss are salvageable? Just tell people to restore and restart once the fix is in?

Yes that's all we can do.

Just cross-linking the list of issues related to system_update_7061(), there's at least two further bugs related to this one. http://drupal.org/project/issues/drupal?text=system_update_7061

catch’s picture

catch
he/him
Primary language English
commented
Issue tags: -D7 upgrade path

I do not think databases that suffered a data loss are salvageable? Just tell people to restore and restart once the fix is in?

Yes that's all we can do.

Just cross-linking the list of issues related to system_update_7061(), there's at least two further bugs related to this one. http://drupal.org/project/issues/drupal?text=system_update_7061

chx’s picture

chx commented
Issue tags: +D7 upgrade path

I think catch crossposted himself and wanted this tag.

iva2k’s picture

iva2k commented

The actual problem I caught is when user pictures were replaced by some arbitrary zip files.

I grepped over a bunch of hook_update_N implementations in my sandbox for file_managed, and found some troubling list of files. Half of them assign new fids, and another half of them carries the old fid from {files} table into {file_managed}, and the list includes contrib modules as well as core:

1. modules that carry fid from {files} table:
system_update_7061
imce_update_7001
uc_catalog_update_7001
media_update_7006 (it renames the {files} table to {file_managed} if latter does not exist.)

2. modules that create new fid in {file_managed} table:
user_update_7012
avatar_selection_7003
comment_upload_7002

Needless to say that none of those have a dependency declared in hook_update_dependencies().

For my migration I added this code into one of hook_update_dependencies():

function mymodule_update_dependencies() {
  // system_update_7061() copies old upload files preserving the fid to {file_managed}
  // table without regard to already existing records with same fid. That may overwrite
  // some of other module's files if they update before system 7061.
  $dependencies['user'][7012] = array(
    'system' => 7061,
    'imce' => 7001,
//    'uc_catalog' => 7001,
//    'media' => 7006,
  );
  $dependencies['avatar_selection'][7012] = $dependencies['user'][7012];
  $dependencies['comment_upload'][7012] = $dependencies['user'][7012];
  $dependencies['imce'][7001] = array('system' => 7061);

  return $dependencies;
}

While doing so I encountered another problem - update process blocks if dependency has a module that was not installed in D6 (uc_catalog and media modules for me, so I commented these lines out). It means hook_update_dependencies() should never contain a module that is not in dependencies[] list in its .info file. I think update process should ignore such modules (use the dependency as informative) if the dependent module is not a required dependency in .info file. I don't know if I should open an issue on this separately, anyone?

Running update.php with this addition preserved all the data.

I know that this is not a solution as 1) the above problem with dependencies and missing modules will prevent having one hook_update_dependencies() and 2) my list of modules is incomplete, as it covered only my sandbox with a small set of contrib modules. There is probably a handful of other contrib modules that will make this list and they will have to be addressed somehow as updates are run alphabetically without explicit dependency and 'system' will land after many of them.

A solution could be adding proper dependencies to each (and every) module that assigns new fids. Of course changing all existing modules (including contrib) to not carry over fixed fid is another solution.

jaredsmith’s picture

jaredsmith commented
Issue tags: +Needs issue summary update

This issue could use an updated issue summary based the standard issue summary templates at http://drupal.org/issue-summaries.

David_Rothstein’s picture

David_Rothstein commented
Status: Active » Needs review
Issue tags: -Needs issue summary update
StatusFileSize
new system-update-7061-user-module-data-loss-1882774-7.patch915 bytes

Here's a completely untested patch which simply makes user_update_7012() depend on system_update_7061().

As for contrib modules:

1. modules that carry fid from {files} table:
.....
imce_update_7001
uc_catalog_update_7001
media_update_7006 (it renames the {files} table to {file_managed} if latter does not exist.)

I have no idea what to do about those, but I don't see how those contrib modules can assume that the {file_managed} table is empty when they run? The site could have already been live on Drupal 7 core before those modules are even upgraded.

2. modules that create new fid in {file_managed} table:
...
avatar_selection_7003
comment_upload_7002

These modules could add a similar dependency as in the attached patch if they want to, but strictly speaking it shouldn't be necessary. If you follow the instructions in UPGRADE.txt correctly, there's no way these update functions could run before the core ones because they won't even be in the codebase yet when the core upgrade runs.

So if you are hitting issues with these modules, you should read UPGRADE.txt and try again. (Although some sites can get away without following those instructions to the letter, it's definitely not expected that all can.)

David_Rothstein’s picture

David_Rothstein commented
Issue tags: +Needs issue summary update

Did not mean to remove that tag.

chx’s picture

chx commented
Status: Needs review » Needs work

This is not enough -- contrib might run before it -- heaven knows what runs there -- it really needs to be bumped to pre-update for this reason.

David_Rothstein’s picture

David_Rothstein commented
Status: Needs work » Needs review

See my comment above - how would that happen unless you upgraded without following the instructions in UPGRADE.txt?

catch’s picture

catch
he/him
Primary language English
commented

Even if you didn't, contrib modules could add an update dependency if they need to.

fabianx’s picture

fabianx commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

RTBC, this fix is better than no fix.

David_Rothstein’s picture

David_Rothstein commented
Status: Reviewed & tested by the community » Fixed
Issue tags: +7.36 release notes

I did some testing and was able to reproduce this on a D6 site that had one file uploaded to a node and one picture attached to a user. The D6-to-D7 upgrade path removed the user picture and essentially replaced it with the file from the node, but with this patch it didn't and all went well.

Hopefully the reordered update functions don't break anything else in the upgrade path.... but they don't seem to and the tests pass, so let's go ahead with it.

Committed to 7.x - thanks!

  • David_Rothstein committed 2ace190 on 7.x 
    Issue #1882774 by David_Rothstein, iva2k: User pictures are lost when...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.