(Needs tests) Reordering multivalue file and image field uploads is broken

I can confirm that this is an issue.

Steps to repeat the problem.

1. Edit the default article content type and change the image field to allow multiple images.
2. Create a new article node and upload multiple images.
3. After saving the node, return to the edit screen and use the drag/drop functionality to re-order the attached images and click save.

Expected outcome, images should be displayed on the node in the order you just set. Actual outcome. They retain the ordering from when the node was originally created (the order the files are uploaded.)

Moving this to the dev queue

Very possibly related (if not plain duplicate) : #817668: Removing image from Article not working for me

Are you sure that this is still the case with latest HEAD? There was a image handing commit recently which afaik fixed this.

Yeah, I guess I was wrong, what I meant/remembered was broken *deletion* of images, that should have been fixed with #736298: Refactor file.module to use proper button click detection, enabling FAPI to improve button click detection security

Reordering files and images doesn't work because field_submit_default doesn't sort custom "multiple values" implementations.

This patch:

• Adds file_field_insert() to sort file items on field insert
• Adds image_field_insert() to call file_field_insert()
• Updates file_field_update() to sort items

This patch is related to both file.module and image.module, but essentially is a file.module issue.

#12: 819816-image-file-field-sort-order.patch queued for re-testing.

1) if something needs to be done in both 'submit' and 'insert' ops, then 'presave' op might be more relevant.

But:
2) hook_field_[op]() hooks are run for a given field type, regardless of the widget being used. Even though there is only one widget available in core for File and Image field types, there might be alternate widgets in contrib at some point. Having the generic field-type hooks run code relevant only for a specific widget type (that happens to implement its own 'reorder' feature, I don't exactly remember why) is not too great.
Ideally, it should be the widget that needs it that triggers the extra submit steps, for instance in a FAPI #value_callback value.

yched, thanks for the review.
I'm not sure the best way to use a #value_callback. The children need to be sorted based on their relative weights, and the parent element's #value_callback never gets called.

In this patch I moved the logic in the the #process callback, but it doesn't feel quite right.
How can make sure the #value_callback from the parent element gets triggered?

+++ modules/file/file.field.inc	12 Jul 2010 17:37:26 -0000
@@ -666,9 +666,26 @@ function file_field_widget_process($elem
- * wrapper around the entire group so it can be replaced all at once.
+ * wrapper around the entire group so it can be replaced all at once. On form ¶
+ * submit, sorts the file fields based on assigned weight.
  */

trailing white space

+++ modules/file/file.field.inc	12 Jul 2010 17:37:26 -0000
@@ -666,9 +666,26 @@ function file_field_widget_process($elem
+  }
+  ¶
   $element_children = element_children($element, TRUE);

trailing white space

51 critical left. Go review some!

Right, my bad, #value_callback is only called for #input elements, and cannot be used on a simple wrapper element.

To tell you the truth, I'm not sure what the best solution is... Would be good to have quicksketch chime in here.
I'm not too fond of hitting $form_state['input'] directly.

While digging around, I stumbled upon #853926: Wrong order of multi-value file fields when form rebuilt (preview, failed validation), which looks related but is actually a different bug with a separate fix.

I have worked with the latest (passed) patch above, and it causes unusual behavior. You can not reorder images, yet you can 'swap' them (example: 4 for 10 OR 5 for 7). However, if you delete a 'swapped' image, all images added after the 'swapped' image will be deleted.

Attached is a much more elegant solution, imo, starting from scratch.

• Added #element_validate attribute, file_field_element_validate(), to wrap the set of file upload fields in hook_field_widget_form()
  file_field_element_validate() sorts the fields' form_state['values'].

This solution only requires changing one file, since image field's form builds on file_field_widget_form().

I'm not sure that #element_validate is the most intuitive place to stuff this logic, but there's no such thing as #element_submit, so it's really the only option. As noted above, #value_callback applies to single elements only, which precludes sorting a group of elements.

Now gimme some love, testbot!

#22 works - but you should use form_set_value() instead of writing to $form_state['values'] directly.

I've been wondering about a simpler fix - just let Field API sort items in all cases, instead of just the ones for which it handles multiple values itself (just removing the if around _field_sort_items() in field_default_submit()).

If the widget implements its own reordering (like file widget), it can take care of providing the _weight in the expected place;
if the widget is inherently not reorderable (multi selects, checkboxes), _weight is just not there.

The _field_sort_items_helper() sort function already handles the case where no _weight information is provided in the submitted form values. Problem is: in this case, usort() with a sort callback that always returns 0 ends up with an array in the reversed order :

function cmp($a, $b) {return 0;}
$a = array(3, 2, 5, 6, 1);
usort($a, "cmp");
dsm($a); // --> array(1, 6, 5, 2, 3)

So unless we have a workaround, #22 (having widgets do the extra mile) is the way to go. A widget re-implementing its own drag-drop should be enough of an edge case that it's not too big a deal.

If we can afford removing the NULL values (I guess that's the only case where an item can be something that isn't an array?), we can easily do something like that, and simplify the logic in _field_sort_items_helper().

function _field_sort_items($field, $items) {
  if (($field['cardinality'] > 1 || $field['cardinality'] == FIELD_CARDINALITY_UNLIMITED) && isset($items[0]['_weight'])) {
    $count = 0;
    foreach ($items as $key => $item) {
      if (!is_array($item)) {
        unset($items[$key]);
      }
      else {
        $items[$key]['_weight'] = (isset($items[$key]['_weight']) ? $items[$key]['_weight'] : 0) + $count / count($items);
      }
      $count++;
    }
    usort($items, '_field_sort_items_helper');
    foreach ($items as $delta => $item) {
      unset($items[$delta]['_weight']);
    }
  }
  return $items;
}

Hm, removing empty values is the job of _field_filter_items() , I'd rather keep them separate if possible.

However, I'm a jerk - _field_sort_items() does check that the first element has a _weight, and does nothing if not.
So the approach in #23 should work. We just ask widgets that handle multiple values on their own to be consistent : either they provide a _weight for all values, or for none - which is only a fair assumption IMO.

Patch.

Sounds good to me.

What about:

1) Removing items before sorting (because (a) there is no point in sorting items we will remove and (b) what happens if you remove the first element and sort the other? the check at the beginning of _field_sort_items() might very well fail)
2) Simplify the logic of _field_sort_items_helper() so that it explicitly fails if a '_weight' key is missing

Something like this?

Re-titling so I stop being confused when I see this in the critical bug queue. :)

Also, sounds like we need tests for this.

#26/#27 should be OK.
Fixed the syntax error (missing closing parenthesis).

a and b were reversed in the usort helper functions.

LOL. RTBC, then. Thks !

sub

Tests? Or is that impossible because it's JS functionality?

I'm going to go ahead and assume that's the reason. If it is possible to write tests for this, let's please do.

In the meantime, committed to HEAD to close out another critical. w00t.

It's not strictly JS related, reordering works without JS too (plain old 'weight' form inputs). Should thus be testable.

Downgrading

I am pretty sure there are already tests for the weight fields.
These are the tests that were failing in #30, no?

re #39 : no, the tests failing in #30 are testing generic reordering for widgets where field API takes care of handling multiple values - i.e not the tests that would have detected the bug in this thread with filefields.

This is a problem in D6 as well.

See #1083904: Drag 'n Drop reordering broken (within fieldgroups?)

my last comment was inaccurate.

This was fixed in 9.1.x. and left open for a backport to D7. Marking fixed since D7 is no longer supported.

@kim.pepper, thanks for cleaning up issues.

Updating credit and version to the time of the commit.

commit a1ce6bac55287fa815921561d747b5fc2e88fde6
Author: Angie Byron <webchick@24967.no-reply.drupal.org>
Date:   Wed Aug 4 06:55:35 2010 +0000

    #819816 by aaronbauman, yched, Damien Tournoud, tsi: Fixed Reordering multivalue file and image field uploads is broken.