Hi,

There are a couple of issues in the CCK queue that are related to multigroups:

- #196421: Deleting unwanted multiple values / multiple values delta issues
- #119102: Combo field - group different fields into one

Let me quote myself on a comment I posted in comment #93 of the first issue:

The only issue with this patch I know of is that possible conflict with filefield/imagefield, but I think both 'remove' buttons can co-exist perfectly well because they both serve a different purpose which seems to be clear enough for the user, as far as I can tell from the input I got here in the office, and having got no negative comment from here either. hmm... yes, look. The 'remove' button provided here allows the user to remove an item, it desapears from the UI. The 'remove' button provided by filefield/imagefield can be used to clear the value of the item, which is not the same exact thing. Anyway... I would love to hear suggestions here, so there may be a better way to do it.

That being said. fielfield/imagefield don't work with multigroups. Well, more or less. What fails is the AHAH stuff related to the 'remove' button provided by fielfield/imagefield. I started to look at that, and I feel it can be solved with a patch to filefield, but I'm not yet sure. Unfortunately, I've been very busy this week with other things, so I had to leave this temporarily. I hope to get back to it next week.

PS: I think the problem with filefield in multigroups is related to the value callback, which is a piece of code that's executed before after_build, which is where the multigroup code uses to move fields back to their original positions in the form. Well, I said, I'm not sure... the function that's aparently broken is filefield_js(), but the real cause may be somewhere else...

Please, let me know if there's anything else that needs to be done in regards to the 'remove' button, so that I can provide a different patch to CCK, or maybe work a patch here for filefield.

When looking at the compatibility issue with multigroups, I noticed that filefield 'compacts deltas' in several places. Is there any reason for that? This is a problem because that will cause delta items to shift within multigroups. For example, if we have the following:

delta  myfile   mytext   myselect
0      foo.txt  foofoo   Option A
1      bar.txt  barbar   Option B
2      abc.txt  abcabc   Option C

If the second file (bar.txt) is emptied, we need to still keep the other fields in sync, so we get this:

delta  myfile   mytext   myselect
0      foo.txt  foofoo   Option A
1      -        barbar   Option B
2      abc.txt  abcabc   Option C

That's why we cannot compact deltas anymore.

However, fielfield compacts deltas in several places, so get the following, which is not correct.

delta  myfile   mytext   myselect
0      foo.txt  foofoo   Option A
1      abc.txt  barbar   Option B
2      -        abcabc   Option C

Note that the 3rd file has been moved to the second row (because its delta has been changed by compacting deltas code in filefield), related to other fields, which is wrong.

Well, I'm wondering why filefield is compacting deltas. ¿? ...what would happen if we remove that. Compacting deltas should be something that is resolved within CCK when needed, see content_set_empty(), so maybe it is not really needed here?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quicksketch’s picture

I'm totally down with compacting deltas, it's legacy code left over from the Drupal 5 version. Are there any other changes we need to implement? I'm 100% behind CCK multigroup so that we can remove all the crazy extra fields from FileField (like description) which don't really relate directly to the file/image.

I'm not saying that we'll actually remove description, but FileField is set up to handle multiple extra fields in its widget, but no other widgets have yet been created to implement this feature (nor will they be once multifield is done). I'm going to try to get my copy of CCK working with multifield then start working on fixing FileField. Any insight or patches you can provide to fix FileField would be very welcome. Thanks!

quicksketch’s picture

Status: Active » Needs work
FileSize
1.13 KB

The 'remove' button provided here allows the user to remove an item, it desapears from the UI. The 'remove' button provided by filefield/imagefield can be used to clear the value of the item, which is not the same exact thing.

I've also tidied this up quite a bit by drastically simplifying the FileField form in #387194: FileField UI Refactoring. We still have per-field remove buttons, but at least we've removed "replace", since you can just click "remove" then upload a new image in the same place.

The first stab at this, I can't quite get this working. Uploading a file results in the an empty upload field and a bunch of notices. But at the same time it seems to work just fine with the delta compacting removed.

markus_petrux’s picture

Thanks a lot for chiming in. :-)

It's good to know there's no particular reason to keep the compacting deltas snippets in filefield.

The other thing that I believe it's causing conflicts with multigroup is a bit more tricky... The content multigroup module changes the position of the fields in the form at hook_form_alter() time, which is invoked through a fieldgroup hook (from group/field/delta to group/delta/field). To prevent from breaking the code of CCK fields which their validate handlers depend on the field position in the form, the content multigroup module injects an after_build handler where all fields are moved back from group/delta/field to group/field/delta.

I believe the mutligroup module doesn't work with filefield because it has a value callback that's executed by FormAPI before the after_build handler of the content_multigroup takes action.

I hope that helps. I'll also try to find a hole to look into this, which might be easier now that there seem to be no particular reason to compact deltas in filefield.

markus_petrux’s picture

FileSize
2.06 KB

Here's a new patch that takes #2 a bit further.

Besides removing the array_keys() stuff, it also replaces unset($item[$delta]) with $item[$delta] = NULL. Otherwise we still get items shifted from the array, and that breaks the multigroup.

The filefield 'upload' and 'remove' buttons do not work yet. However, the preview button of the node form worked for me with this setup:

1) Installed latest CCK2 dev, with the multigroup related patches.
2) Created a content type with a multigroup with 2 fields: filefield and text.

Steps taken:

1) Create a new node of that content type.
2) Enter some stuff in the text field.
3) Press 'browse' button of file field and select a plain txt file.
4) Press the 'preview' button of the node form.

Result:

The file was uploaded to correct place, and a record was created to the {file} table (temporary status). The node preview output was rendered correctly, and the node edit form was also rendered correctly.

Now, if we use the 'Add more values' button, it works. If we reorder deltas, then sometimes work, and sometimes don't. But I believe it is getting close...

markus_petrux’s picture

BTW, that fact that the preview button of the node edit form worked makes me think there's no problem with the value callback of filefield. But still, I'm not sure...

Maybe the problem with the filefield buttons 'upload' and 'remove' within a multigroup happens because the AHAH handlers need more love.

markus_petrux’s picture

FileSize
2.51 KB

Here's a new patch that seems to work in all cases I tested. Adding more items, reordering, letting fields empty, etc.

The AHAH buttons don't work yet, so I'm uploading, deleting or forcing node form updates using the preview button.

The AHAH stuff is something I don't completely understand, so if someone does, please help.

markus_petrux’s picture

Status: Needs work » Needs review

The patch in #6 should not affect the functionality of filefield without multigroup, so maybe this is CNR.

markus_petrux’s picture

Here's an updated version of the multigroup module:

http://drupal.org/node/119102#comment-1313226 (comment #378)

quicksketch’s picture

Wow, thanks markus_petrux! I spent several hours last night trying to figure this out also. It looks like in the filefield_js() function, the $form_state correctly has the uploaded file (the uploaded file is also saved to disk) when using the AHAH upload button. However, something doesn't work right when filefield calls content_field_form().

  $field_element = content_field_form($built_form, $built_form_state, $field);

The returned $field_element doesn't contain the newly uploaded file, even though $built_form_state does. Like you mention though, the preview and save buttons work fine, as does uploading with JavaScript disabled, so I'm also suspecting something in the filefield_js() function, even though I can't find anything wrong.

markus_petrux’s picture

Since the multigroup alters the position of the fields from group/field/delta to group/delta/field, maybe filefield_js() would have to use content_multigroup_group_form() to build the element when the field is part of a multigroup?

You can get if a field is part of a multigroup by checking for ($group['group_type'] == 'multigroup').

filefield_js() knows the delta, so it shouldn't be complex to extract the part of the form that would build content_multigroup_group_form() ?

markus_petrux’s picture

FileSize
5.58 KB

Hi, another patch that includes modifications to filefield_js() to add support for the multigroup.

It doesn't fully work, but I think it's getting close. The 'remove' button seems to work, but the 'upload' button seems to return the field as if it was when starting the edit session. Not sure why...

markus_petrux’s picture

There's something that I don't quite understand with these AHAH functions. We have 3 different functions here:

- cck/includes/content.node_form.inc, function content_add_more_js()
- cck/modules/content_multigroup/content_multigroup.module, function content_multigroup_add_more_js()
- filefield/filefield.module, function filefield_js()

The last one doesn't work the same way as the other two, part of cck. Aside from the code that manipulates the form element, which is necessary different, the function in filefield does not cache the form back after it's changed, or it manipulates js settings in a different way. Maybe the problem is somewhere around here, but I don't fully understand this FAPI operations, so I cannot tell. :(

quicksketch’s picture

FileSize
8.75 KB

After some grueling debugging I finally found the problem. What's happening here is that we're checking the $form_state['clicked_button'] value in our #process callback. However, the buttons themselves are added in that same #process callback later. Since the buttons don't exist yet, $form_state['clicked_button'] isn't populated.

I tried looking for any possible way of adding the buttons earlier in the process, but I really couldn't find anything. So this patch takes a different approach of adding the buttons first then manually checking them. It's not pretty, but it cleans up our AHAH callback quite a bit and makes it more consistent to form_alter(). Plus, it works with multigroup, since that's what we're after here. :)

quicksketch’s picture

FileSize
9.08 KB

Reroll that removes the "_weight" field from the AHAH callback. Now that we're running through the normal form building mechanisms, content.module is automatically adding it in there.

quicksketch’s picture

Status: Needs review » Fixed

I went ahead and committed this patch, let me know if you need anything else to get FileField working with Multigroup. I merged ImageField back into FileField for field handling (#397578: Uncouple ImageField from FileField Custom Hooks), so ImageField now automatically gets this compatibility too.

quicksketch’s picture

FileSize
736 bytes

Looks like I didn't test *without* the multigroup patch applied. This small patch corrects fields within groups but will need to be unapplied when the Multigroup patch goes in.

markus_petrux’s picture

Thanks a lot, quicksketch, for keep this moving. I'm sorry I had not had the time to test your latest patches.

Based on a quick review of the last patch, I believe one could be rolled that checks the type of group. If it is 'standard' get the field from group/field/delta, if it is 'multigroup' get the field from group/delta/field.

markus_petrux’s picture

Status: Fixed » Needs work

Reopening because there's still something that could be done to support multigroups with no impact to current user base.

For example, we could do the following to filefield_js():

-    $form_element = $form[$group_name][$field_name][$delta];
+    if (isset($form['#multigroups']) && isset($form['#multigroups'][$group_name][$field_name])) {
+      $form_element = $form[$group_name][$delta][$field_name];
+    }
+    else {
+      $form_element = $form[$group_name][$field_name][$delta];
+    }

And then it works with multigroups as well as standard fieldgroups.

Off topic: I was trying to work on this, but got a weird issue when using filefield_paths #398754: Wrong file name when transliteration is applied to file paths / new files deleted.. I hope to get into this tomorrow, and maybe we can get a filefield fully compatible with CCK multigroups. :)

quicksketch’s picture

Status: Needs work » Fixed

Thanks again markus_petrux, committed this fix. I had thought that the multigroup patch would actually change the structure of all groups, I didn't realize that the old structure would stay the same for traditional field groups.

markus_petrux’s picture

Status: Fixed » Needs review
FileSize
2.23 KB

Sorry for re-opening this issue again, but I believe there's something more than needs to be fixed.

I've been playing with filefields with and without fieldgroups, multigroups, with one single value and multiple values, reordering them adding, removing and so on... sometimes filefields loose sync with other fields in multigroups, and here's a patch that I think fixes this.

Basically, what it does is $items[$delta] = NULL; rather than unset($items[$delta]);. CCK will remove items when necessary from content_set_empty().

One thing I should note about my tests is that my CCK is latest 6.x-2.x-dev with the latest patch in #196421 applied, which is the one that changes the way items in CCK are removed (with the 'remove' button). It changes the CCK function content_set_empty() among other things. So maybe the patch attached here works only well with that. I cannot tell with a plain CCK installation.

quicksketch’s picture

Is setting $items[$delta] = NULL the correct value? Normally for "empty" rows, a FileField row will be array('fid' => 0). At least, I think this is what FileFields default to when the form is initially loaded and no files have been uploaded.

markus_petrux’s picture

I would say using NULL is not completely incorrect because content_multiple_value_form() in cck/includes/content.node_form.inc may invoke hook_widget() for items that do not really exist in the field values array.

For example, in filefield_widget() we have code that's aware of NULL items:

  $item = array('fid' => 0, 'list' => $field['list_default'], 'data' => array('description' => ''));
  if (isset($items[$delta])) {
    $item = array_merge($item, $items[$delta]);
  }

On the other hand, if unset() is used, then the array of field items is altered by _content_sort_items() in content.module, which may cause loss of sync with other fields in multigroups.

quicksketch’s picture

Status: Needs review » Fixed

Yeah now thinking about it some more, NULL will be equivalent to unset() in every situation where FileField checks, but maintain the current indexes on the list of items. Thanks for your thorough explanation. So... committed and marking fixed again. Let me know if further changes are needed.

Status: Fixed » Closed (fixed)

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