Provide upgrade path to file

subscribe

OH YEAH BABY!

Subscribe!

so needs to:

if the upload module is active:
* create field_attachments (or field_attachments_n if there is already such a field)
* attach that field to all existing content types
* migrate attachment files to that field

is that it in a nutshell?

for that matter, upload.module won't even exist after this is all done. :D

Detail : please keep the 'field_' namespace for UI-created fields.
Do we really want to add an 'attachments' field to all existing content types ? Or only to the ones for which variable_get('upload_[type]', FALSE) == TRUE ?

Also, we'll probably need to mirror existing upload settings ? (filesize, user quota, file extensions)

@yched - thanks; didn't realize that non-UI-created fields could have other names. Makes the naming task easier; we'll be able to use 'attachments'. (Related question: will this field then be able to be modified or deleted by users down the road? We don't really want this field to be maintained by a module.)

And yes, we'll want to limit the types to only the specified content types, and mirror existing upload settings.

"will this field then be able to be modified or deleted by users down the road?"
Yes, modules exposing fields that shouldn't be removed through the UI should use $field['locked'] = TRUE (although I'm not sure field UI actually supports it right now).
All instance properties (weights, label, widgets, display formatters) are always freely changeable through the UI, whether field is 'locked' or not.

subscribing

totally a work in progress. lots of TODOs in there.

okay a lot closer but still some TODOs. need some feedback and advise though.

• If we're really killing off upload.module we should move the updates done in upload_update_7000() into this update.
• Translatable? I marked the fields as non-translatable since I don't think the current upload are translated... not really sure which makes more sense.
• Uninstalling via drupal_uninstall_modules() doesn't seem to quite do the trick. It doesn't change the module's system.status value to 0 so upload_file_load() still gets called resulting in fatal errors.

I should mention I stole a big chunk of this from node_update_7006() so look there if you have questions.

SPAM here.. please clean it..
edit: thanks..

Looks like upload_update_7000() was duplicating system_update_7035(). So I deleted upload_update_7000() spent some time batch enabling system_update_7035().

Sub

Huh, just now found this issue. I think removing upload.module is clearly allowed. It's already "obsolete" and there is a 100% replacement for it. There is no reason why upload.module should stick around.

I was focusing on other image features, but I'll help push this along as necessary post-freeze.

I'm working on this upgrade.

Slowly getting back on track after 4 weeks mainly afk. Just a few notes on the last patch in #17, even though quicksketch is working on his version.

+++ modules/system/system.install
@@ -2431,28 +2431,74 @@ function system_update_7034() {
+      $result = $query
+        ->fields('f', array('fid', 'uid', 'filename', 'filepath', 'filemime', 'filesize', 'status', 'timestamp'))
+        ->range($context['last'], $batch_size)
+        ->orderBy('fid', 'ASC')
+        ->execute();
...
...
+        $context['last'] = $file->fid;

I don't think range($last_fid, 100) will work. It's one of :
a) track the number of files and use that in the query range
b) track the fid of the last file handled, and use a 'fid > $last_fid' condition + order by fid in the query.
That's often the most puzzling bit in multipass ops.

+++ modules/system/system.install
@@ -2431,28 +2431,74 @@ function system_update_7034() {
+      // We will convert filepath to uri using the default schmeme and stripping

typo 'schmeme' (already there, the patch just moves it around)

+++ modules/system/system.install
@@ -2493,6 +2539,222 @@ function system_update_7037() {
+    // The {upload} table will be deleted when this update is complete so we
+    // want to be careful to migrate all the data, even for node types that
+    // may have had attachments disabled after files were uploaded. Look for
+    // any other node types referenced by the upload records and add those to
+    // the list. The admin can always remove the field later.

I agree with the reasoning, but re-displaying file attachments on node types where they had been 'disabled' might come as a surprise. Maybe this could warrant a message in the upgrade results ? Something like 'Attachments were found in the following node types where the upload feature was disabled. The field_upload field has been added to those types, you might want to remove it' ?

+++ modules/system/system.install
@@ -2493,6 +2539,222 @@ function system_update_7037() {
+    // If there are any any node types then create the file field and an

any any

+++ modules/system/system.install
@@ -2493,6 +2539,222 @@ function system_update_7037() {
+      $instance = array(
...
+        'widget' => array(
+          'weight' => '1',
+          'module' => 'file',
+          'active' => '1',

'module' and 'active' are read-only and should not be specified (same thing in display settings)

+++ modules/system/system.install
@@ -2493,6 +2539,222 @@ function system_update_7037() {
+        'bundle' => NULL,

Looks odd. Maybe a just comment that bundle will be specified later ?
Also, after #470242: Namespacing for bundle names we now need to specify 'object_type' => 'node'

+++ modules/system/system.install
@@ -2493,6 +2539,222 @@ function system_update_7037() {
+        'display' => array(
+          'full' => array(
...
+            'settings' => array(),
+            'weight' => 0,

We should probably assign the same display weight than current file uploads, to ensure they get displayed below the body (which has weight 0).

Also, there's no need to specify a 'settings' element if it's empty.

+++ modules/system/system.install
@@ -2493,6 +2539,222 @@ function system_update_7037() {
+      $vids = db_select('upload', 'u')
+        ->distinct()
+        ->fields('u', array('vid'))
+        ->range($context['last'], $batch_size)

range($last_vid) : Same remark as above.

I'm on crack. Are you, too?

Additionally, I'm not sure that saving the field data with a simple

+        // 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());

will work, since this skips calls to hook_field_[insert|update](), which filefield uses for reference tracking.
That code was taken from the 'body as field' upgrade, and is OK there, since text fields have no specific post processing.
Well, maybe that's not actually an issue in this specific update process, dunno for sure. Quicksketch should probably know better than me.

what this moment the upgrade path is blocked with the upload module.

An error occurred.
Path: http://xxxx/update.php?id=2&op=do
Message: Recoverable fatal error: Argument 4 passed to db_query_range() must be an array, integer given, called in /persistent/home/int/sites/drupal7/modules/upload/upload.install on line 101 and defined in db_query_range() (line 1897 of /persistent/home/int/sites/drupal7/includes/database/database.inc).

int, when we finish this patch, we'll remove upload entirely (including the upload.install file), so it doesn't really matter what's in upload at the moment. The upgrade will be done via system.install.

subscribe - I'm going to use this issue to learn more about Drupal 7 - I'm currently serving a kind of one-man Drupal apprenticeship at the D7 issue queue coal face. If any more experienced Drupal developers can provide some mentoring I'd really appreciate it.

Regarding file references, please see #353458: hook_file_references() was not designed for a highly flexible field storage

Please note that this issue/patch is still in the loop for D7. The patch seems to be half-way done and the overall task doesn't look to be very challenging.

Though I'm not sure whether #353458: hook_file_references() was not designed for a highly flexible field storage is a prerequisite for this issue? (It removes all the nightmare around file references in File module)

The current strategy, as expressed by webchick, is: "it's too late now, let's just remove the filefield UI" (reference: #608894: Resolve the conflict between the fieldable users UI and the profile module).

Er - what ?

I don't see how this relates to the discussion in #608894: Resolve the conflict between the fieldable users UI and the profile module.
Shipping with profile is one thing.
Shipping with upload + filefield is a major WTF.

yeah not seeing the relation at all. i think damien might have edited the wrong issue.

yes, I was channeling webchick above. So, any takers?

Shipping with profile is one thing.
Shipping with upload + filefield is a major WTF.

I don't see any difference.

Shipping with Profile + User Fields is a major WTF
Shipping with Upload + FileField is a major WTF

I don't understand why the core committers opinion on the first one could be "it's too late now, let's remove User Fields and ship with the 4.6-ish Profile module", while the opinion on the second one could be "let's take the time required to properly migrate Upload to FileField and then remove Upload".

- "Shipping with Profile + Fieldable users" is still being discussed in #608894: Resolve the conflict between the fieldable users UI and the profile module,
- and no, I don't see how this compares to this issue which is just a migration path and file rm. Nothing comparable to the 'profile as field' quagmire for which there is no clear architecture.

The core version of FileField was specifically designed to replace Upload module, hence the reason why we have things like the "Display" checkbox, the imitation of how Upload module currently works, and the "Table of files" Field formatter. Right now we have two things that provide *identical* functionality in Drupal core. Profile module is a different matter, since we're lacking both the APIs and the actual implementation to actually have a full replacement for Profile module.

Literally the only thing remaining in killing upload module is this upgrade path. We're not even close to having a replacement for profile, which is why this is "Clean up" and profile module is still "New features".

Just because the continued existence of Profile module is a frustrating doesn't mean that we should make another mistake and keep Upload module too when there is absolutely no reason to keep it.

I've been taking a look at this over the weekend, but it looks like we have numerous updates failing (node, user, dblog, taxonomy... it's pretty bad) which makes it impossible to do a straight upgrade even without this update function. This might take a while fixing all the other upgrades. :-/

scors' #24 patch in #563106: Cannot upgrade from Drupal 6 to Drupal 7 - meta issue unblocks my upgrade issues, hurray! I'll continue working on the upgrade while that set of patches gets committed.

Yes, to clarify, the difference between this and Profile is we actually have a replacement for Upload in core and we do not have a replacement for Profile in core. In both cases, we're removing the 'less done' option of the two.

This is still on the table for D7, and is in fact necessary in order to provide the upgrade path.

Here's a fully working upgrade patch. Unfortunately we're blocked because of #563106: Cannot upgrade from Drupal 6 to Drupal 7 - meta issue, but at least this gets the data moved over if the patches are applied in the correct order.

Here's how to test this patch:
- Install a Drupal 6 site.
- Create some nodes that contain uploads.
- Point an unpatched Drupal 7 site at the D6 database.
- Apply #24 from #563106: Cannot upgrade from Drupal 6 to Drupal 7 - meta issue.
- Delete the modules/upload directory.
- Run update.php. Most upgrades will complete but you'll get an error in the node.module upgrade converting the body into field.
- Apply this patch.
- Run update.php again. Now the node body conversion patch and this upload module patch will migrate over data into the new fields.

I'm not sure how to fix the needing to run update.php twice. There used to be an "#abort" property we could use to officially bail out, now I'm not sure how to reproduce that functionality in the new update system.

This patch still needs work on making it so that it can be run in a single pass through update.php, but node.module has this same problem. The solution we find can be applied in both places hopefully.

I tested this patch with 15,000 nodes and about 35,000 uploaded files, so I know it should work with large numbers and the batch processing is working properly. I think this patch also addresses yched's comments in #24, but another in-progress review would certainly be appreciated.

There used to be an "#abort" property we could use to officially bail out, now I'm not sure how to reproduce that functionality in the new update system.

Just raise an exception. update_do_one() catches it and treats it like the previous #abort.

As part of a potential fix, what do you guys think about moving this update function to node.install? System.install is a bit of a tricky situation, since it's forced to run first, before any other modules. This is problematic in the case of this field conversion, since other modules need to run updates before their field systems are working properly. Taxonomy needs a new database column, and node module renames the entire 'node_revisions' table to 'node_revision'. These seem like potential problems in trying to execute this upgrade path.

Fixed minor stuff:
- fixed a 80 line wrap error
- removed the empty 'settings' => array() in the $instance definition
- replaced cardinality '-1' with FIELD_CARDINALITY_UNLIMITED
- removed the try { } catch {}, since an exception will simply cause the update to #abort - although I'm not sure what kind of exceptions was expected ?
- $sandbox['#finished'] = 1; is not needed.

Other than that:
- don't we want to remove the various upload_* variables from {variable} table ?
- I'm still trying to wrap my head around that part:

  // 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.
  if ((count($node_revisions) > 1) && (count($result) == $limit)) {
    array_pop($node_revisions);
  }
  else {
    $finished = TRUE;
  }

I understand the point, and this might very well be the right way, but so far I can't fully convince myself ;-)

Crosspost with #43 - moving this to node.install seems good.

- don't we want to remove the various upload_* variables from {variable} table ?

I think we do that already with this line:

      variable_del('upload_' . $node_type);

- I'm still trying to wrap my head around that part:

Yes that's a highly confusing segment. Basically what we don't want to have happen is for a revision to be updated across two requests. If half a revision were updated, then the second half of the revision would end up with duplicate delta keys when we did the inserts into the field tables. Say a revision had 6 uploads, the first three would be inserted as deltas 1-3. Then the second request would find the last three uploads and insert them as deltas 1-3 again, wiping out the previous values.

Here's a fully working patch that should kill Upload module when combined with an updated patch in #563106: Cannot upgrade from Drupal 6 to Drupal 7 - meta issue.

Instructions for use:
- Install a Drupal 6 site.
- Enable upload module. Create some nodes with uploads.
- Point your Drupal 7 site at the Drupal 6 database.
- Apply #32 from #563106: Cannot upgrade from Drupal 6 to Drupal 7 - meta issue.
- Trash the "modules/upload" directory.
- Apply this patch.
- Run update.php (just once now, yay!).

I moved the upgrade to node.install, which runs after all other modules now, rather than system.install which had the caveat that it would always run first before other modules had made the necessary changes to make the Field API fully functional.

variable_del('upload_' . $node_type);
There are more : upload_extensions_default, upload_uploadsize_default, ... A full-text-serach on variable_get('upload_* brings quite a few.

About the 'full revisions':
I thhink the logic in the current patch will break if a range query returns 500 rows for the same revision. OK, that means a revision with 500+ files, not really likely.
Other than that, the logic sounds good.

Yup - a couple days ago intecepted a 'overhaul help text for upload.module' patch, but after someone wasted 2 hours on it :-/.

Removing upload.module now sounds like a good idea to me also.

Just a note that this is still ready to go afaik, but this is not where the hold-up is. This patch is dependent on #563106: Cannot upgrade from Drupal 6 to Drupal 7 - meta issue, which is dependent on #211182: Updates run in unpredictable order. #211182 is pretty much the bane of the upgrade path right now.

RTBC for patch 53.

Committed #53 to CVS. Now back to providing an upgrade path.

Hm, was #53 really committed ? I don't see it in the CVS log and still have an upload module in my updated local copy.

Oh! OH! ME! I GET TO KILL IT! MWAHAHAHAHAHAAAA!

*ahem*

Really committed to HEAD now. ;)

I can confirm that the patch was indeed committed.

Keeping this open. The patch that was commited was an intermidiate step, the upgrade path still needs to be committed. See #52 for the prerequisites.

Upload module offered out-of-the-box "podcasting", i.e. RSS enclosure element. grep doesn't turn up any mention of "enclosure" in the current D7 codebase, with the exception of CHANGELOG entry for Drupal 4.6.0 when the feature was added. Are there any plans for restoring this feature e.g. via hook_node_view implementation?

mfb: I'd suggest a separate issue for that. Though I never saw (or would ever recommend) a podcast site based on Upload module, it doesn't seem like something that would be difficult to implement as a Field Formatter for the File field. Though at the same time, I'm not sure this would be significant loss since the RSS feed wouldn't actually work in iTunes, meaning that you'd need an additional module if you had any plans for widespread use of your podcast anyway.

@quicksketch Sounds good, see #666412: Regression: RSS feed enclosure support lost from core. BTW, plain RSS 2.0 feeds with enclosures worked fine with iTunes last I checked; the iTunes tags are recommended not required.

webchick submitted #693084: Regression: file_munge_filename() extension handling broken by move to File Field originally under the title "Can't upload .tar.gz files in update manager". In comment #9 I pointed out that removing upload.module in favor of file field completely broke file_save_upload() because we're not handling the upload_extension_* variables anymore. Not sure if we want to fold all that into this issue and mark that one duplicate, or split out the whole question of file upload extension whitelist handling and the admin UI to populate the per-role whitelists to #693084 and mark this postponed until that's resolved (since you can't really have a full upgrade path from upload.module to file fields until you know what those variables are called and how they work). I leave it to the luminous file API folks in this issue to decide. ;)

Thanks,
-Derek

.

#47 does not apply anymore. Maybe it could be merged with #685892-13: Upload -> File module upgrade path is broken so this issue can be closed?

@scor: #53 was applied. the issue is open now to provide an upgrade path to file (which still needs to happen).

uggh. reread; this issue is so old. sorry, @scor, you're right of course.

This is a blocker for beta 1. It would be ever so lovely to have this fixed by the end of the week, so I don't need to roll another alpha. :(

This is a trivial re-roll. I've got it working and giving it a few rounds of testing and I'll get up a new patch.

Well... maybe not trivial since apparently a bunch of APIs have changed that were used in the update. This patch sort of works, it gets the field_upload created but doesn't migrate files yet. I think some changes to BatchAPI must need to be factored in.

Okay, well never mind... looks like my patch got forgotten about and replaced by #685892: Upload -> File module upgrade path is broken. Annoyed. Oh well.

Renaming non-canonical duplicate tag. See #2426171: Multiple tags similar to 'API clean-up' for background.