3F #3: Value callbacks not executed on JS "add more"

The only modules where a value callback ('#value_callback' => 'function') is used are nodereference (for the 'nodereference_autocomplete' element) and userreference (for the 'userreference_autocomplete' value). I haven't tested those, would be interesting to see if they work with JS "add more" or if they don't. If you investigate, I'll concentrate on other filefield related tasks for the time being - thanks for your help :)

Hm, yeah, seems to work correctly with nodereference. I'll have a go at AJAX uploads/deletions first, and when that works and you haven't found the issue yet then I'll probably chime in with this issue.

Debugging shows that filefield's value callback is indeed not called, but nodereference's value callback is.
...strange? hm.

Thinking about it once more, the value callback that is called for nodereference must be the one at loading time, not at submit time - because it is called 2 times only, not 1 (submit) + 2 (load) times. Thinking this further, filefield's edit widget value callback is probably not called because on deleting files, the edit widget is replaced with the upload widget, which hasn't been loaded before and therefore doesn't know what to do with the edit widget's submit values.

*debug* *try out* *debug more*

Right. Further debugging shows that a non-JS button click invokes the value callback for each existing widget at submit time (before hook_widget() is invoked) and for each existing widget + 1 empty item at loading time (after hook_widget() is invoked). For the JavaScript "add more" button, we only get the latter.

So we know what's missing. Next step: finding out how form.inc normally does it, and what needs to be added in order to get it into content_add_more_js().

> the edit widget is replaced with the upload widget

That, of course, is only a consequence of the missing function call, but not the cause. Anyways, it lead to the correct conclusion. (Btw, updating the title with the additional information.)

Here's the patch that fixes these immediate problems. It also seems to break some aspects of the tabledrag, not sure what's causing that. Form building is way advanced business, not sure if I'm able to get this working properly. Help is appreciated.

Yeah, it does, but that's not the point and doesn't solve the underlying problem. The point is that all values are dropped that are not submitted in $_POST, and that this doesn't happen when submitting without JavaScript. poll.module and all the other AHAH guys in core are able to work because they ship all information around in the $_POST.

All the data that I carry around in $form_state['values'][$field_name] is completely lost when adding a new item. I think that's a grave bug and should be fixed in CCK instead of adding an easy workaround in filefield.

Of course, fixing this problem with the above patch leads to new issues: the deltas in $_POST are being resorted by '_weight', but the $form is not adapted in the same manner in the form cache, which means those two are out of sync. In terms of filefield, this means that when I press the (non-JS) upload button after reshuffling items with JavaScript, the Forms API retrieves its state from the cache, but the cache has been written in an inconsistent state by content_add_more_js().

I've been trying to fix that too by sorting the items in $_POST and the field's form elements in the same manner - see the attached patch. (Yeah, it's a bit larger than the original one.) Works way better than initially, but still slightly buggy: newly JS-created items still don't seem to submit, and the JavaScript behaviour isn't applied after the first AHAH replacement.

This bug might affect only a few modules, but I think it should be fixed properly nevertheless. Like, rather risking a new (fixable) bug now than sticking with a broken mechanism forever. Guess I'll further investigate how to make it work perfectly *and* correctly, plus CCK is still in Beta. In the meantime, is there any chance to get the less controversial other two fixes applied?

Thanks for bearing with me, wishes,
Jakob

I learned a few things while implementing AHAH uploads/deletions in filefield - I think I can improve this patch some more, scheduled for very soon (like, in half an hour of so). If the form_clean_id() thing works, I believe I should be able to make it work pretty nicely.

I've got the same issue ('JavaScript behaviour isn't applied after the first AHAH replacement') in filefield.
The good news is that I've been able to track down *why* this happens.
The bad news is that it's a bug (or let's rather say, "missing feature") in AHAH / Drupal core itself.

The form_clean_id() call is correct, as form_builder() would otherwise build the (nearly) same form twice but with different HTML ids. And that's also the reason why the second button doesn't work: different id isn't contained in the Drupal.settings.ahah object, and therefore doesn't get its behaviour attached.

CCK's upload can be fixed this way, as there is only one "Add another item" button that stays the same throughout all form updates. For filefield though, this is fatal because either the "Upload" button or the "Delete" button don't exist when the form is initially created. With the Drupal.settings object not being updated during AHAH updates, this means that one of those two buttons won't work with AHAH until the form is fully reloaded. Guess I'll have to come up with an ugly workaround for filefield and a patch for core to get additional settings merged on AHAH updates.

Yeah, I fixed this bug for filefield, and the workaround is not that ugly. Thanks for your hint, it has been invaluable. Still no revised patch for this issue, as I wanted to make sure that filefield works properly before moving on to the harder challenge.

Tadah! The updated patch. It's a *lot* cleaner while being more correct and more readable. Makes filefield work perfectly, apart from one special case where the '_weight' doesn't seem to get recognized properly. I'm going to investigate that still, until then, have fun reviewing (and hopefully applying) this patch.

And here's a combined patch of all 3F patches by now (includes the ones in #274038: 3F #4: "Add more" for only one field and #275192: 3F #5: Attach AHAH behaviors to newly inserted widgets), for your convenience, in case you like all of them.

Mmkay, the '_weight' issue mentioned above in comment #13 was an issue with filefield not merging it in. Solved now too.

Therefore, the above can be considered the final, working patch. I hope you like it!

Hm... I don't think this will work just as well, but I'll have to test it out before confirming that.

The main reason why I've been doing it like above is in order not to affect the existing form state with form_set_cache(). If I understood correctly, only the form itself is normally stored, with $form_state and especially $form_state['values'] not being stored to the cache except if some code explicitely writes a variable outside $form_state['values']. form_builder() restores $form_state without looking at any existing state, just by rebuilding the form using the (cached) '#value', '#default_value' and '#value_callback' properties of the form elements.

If I'm wrong with that analysis then yeah, your code might be just as fine.

So, the objectives for my patch were:
1. Don't touch the $form_state that we get from form_get_cache(). In other words, take precautions not to save any additional properties like 'item_count' or 'values' that weren't in there before.
2. Don't touch the $form, except for replacing the content widget with all those new values. If I understand correctly, the form in the cache is in an unbuilt state, and should remain that way.

With those constraints applied, the second form builder can build the form from scratch, and the cache is not being altered in a way that it's not supposed for.

As for building only the form widget itself in the second form builder... yeah, I think that might work.

Reply to side note: If I can find out how to get Drupal/PHP to show notices, rest assured that not a single one will survive.

Whoo! Thanks for your help too! Contributing upstream is so much more fun if the upstream maintainers are just as involved, interested in working solutions, and picky about nice clean code :)

Believe me, I did try to make this as simple as possible too :P
Still not sure if $form_state['values'] should get written to the cache (as in the patch that you committed) or not (as in my patch in comment #13), but I guess it probably won't matter in the end.

Thanks again for the splendid teamwork!