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?
Comment | File | Size | Author |
---|---|---|---|
#20 | filefield-367267-20.patch | 2.23 KB | markus_petrux |
#16 | filefield_group_delta.patch | 736 bytes | quicksketch |
#14 | filefield_multigroup.patch | 9.08 KB | quicksketch |
#13 | filefield_multigroup.patch | 8.75 KB | quicksketch |
#11 | filefield-367267-11.patch | 5.58 KB | markus_petrux |
Comments
Comment #1
quicksketchI'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!
Comment #2
quicksketchI'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.
Comment #3
markus_petrux CreditAttribution: markus_petrux commentedThanks 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.
Comment #4
markus_petrux CreditAttribution: markus_petrux commentedHere'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...
Comment #5
markus_petrux CreditAttribution: markus_petrux commentedBTW, 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.
Comment #6
markus_petrux CreditAttribution: markus_petrux commentedHere'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.
Comment #7
markus_petrux CreditAttribution: markus_petrux commentedThe patch in #6 should not affect the functionality of filefield without multigroup, so maybe this is CNR.
Comment #8
markus_petrux CreditAttribution: markus_petrux commentedHere's an updated version of the multigroup module:
http://drupal.org/node/119102#comment-1313226 (comment #378)
Comment #9
quicksketchWow, 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().
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.
Comment #10
markus_petrux CreditAttribution: markus_petrux commentedSince 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() ?
Comment #11
markus_petrux CreditAttribution: markus_petrux commentedHi, 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...
Comment #12
markus_petrux CreditAttribution: markus_petrux commentedThere'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. :(
Comment #13
quicksketchAfter 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. :)
Comment #14
quicksketchReroll 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.
Comment #15
quicksketchI 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.
Comment #16
quicksketchLooks 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.
Comment #17
markus_petrux CreditAttribution: markus_petrux commentedThanks 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.
Comment #18
markus_petrux CreditAttribution: markus_petrux commentedReopening 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():
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. :)
Comment #19
quicksketchThanks 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.
Comment #20
markus_petrux CreditAttribution: markus_petrux commentedSorry 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 thanunset($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.
Comment #21
quicksketchIs setting
$items[$delta] = NULL
the correct value? Normally for "empty" rows, a FileField row will bearray('fid' => 0)
. At least, I think this is what FileFields default to when the form is initially loaded and no files have been uploaded.Comment #22
markus_petrux CreditAttribution: markus_petrux commentedI 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:
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.
Comment #23
quicksketchYeah 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.