Problem/Motivation
it is currently only possible to define a function as #ajax callback.
When you use the #ajax property "callback" to define what should be executed, it's not possible to address object methods.
This patch would make it possible to assign object methods as that callback.
Proposed solution
We should change from function to functions and object methods. Core already has a way to pass variables by reference to call_user_func_array, so that approach can be used here as well.
I added this issue as task, since field_ui module will actually use this. See #1792600: Refactor field_ui so common behavior for fields and display overview screens are extracted. That issue is not blocked but it would be very nice to use the DisplayOverview object to handle the multistep configurations.
Code
I'll add the code here as well as it is such a small patch and much easier to review.
if (!empty($callback) && is_callable($callback)) {
return call_user_func_array($callback, array(&$form, &$form_state));
}
Comment | File | Size | Author |
---|---|---|---|
#27 | 1802072-27.patch | 0 bytes | fubhy |
#28 | 1802072-27.patch | 4.52 KB | fubhy |
#20 | 1802072-20.patch | 4.55 KB | fubhy |
#19 | 1802072-19.patch | 4.58 KB | fubhy |
#17 | 1802072-17.patch | 3.15 KB | fubhy |
Comments
Comment #1
Stalski CreditAttribution: Stalski commentedThe patch
Comment #2
Stalski CreditAttribution: Stalski commented#1: 1802072-1.patch queued for re-testing.
Comment #3
fubhy CreditAttribution: fubhy commentedThat makes sense...
Comment #4
webchickHm. Let's add a test for this. I tried grepping through #1792600-29: Refactor field_ui so common behavior for fields and display overview screens are extracted and it was not immediately clear to me how this would work.
I'm a little worried here too about changing AJAX callback to any "is_callable" but not doing this consistently throughout FAPI. But I'm not sure what other similar things we have there, really. I guess submit, process, and validate callbacks? Normally these are done as an array of functions that can be re-ordered.
Comment #5
yched CreditAttribution: yched commented$form['#submit'] and #validate are already doable as "methods" (entity form controller uses that)
#element_validate, #pre_render, #after_build... are not
Comment #6
fubhy CreditAttribution: fubhy commentedThat doesn't change in any way. But yes, all callbacks in FAPI should get converted to support methods at some point. Yes, we could maybe add a simple test that exposes a method as an ajax callback. Basically we would be testing core PHP functionality with that considering that the patch doesn't change anything apart from the switch to is_callable() :). But then again, it won't hurt... So why not :).
Brb
Comment #7
Stalski CreditAttribution: Stalski commentedWell actually the reason why I did not saw the need for a test is because when field_ui OOPize patch is in, the multistep field instance create test will cover this behavior, see #1792600: Refactor field_ui so common behavior for fields and display overview screens are extracted.
Isn't that sufficient?
Comment #8
fubhy CreditAttribution: fubhy commentedThat should do. I simply converted two of the callbacks to be methods in one of the existing AJAX tests.
Comment #9
fubhy CreditAttribution: fubhy commentedComment #10
fubhy CreditAttribution: fubhy commentedWhoops, class name f*ckup :)
Comment #12
fubhy CreditAttribution: fubhy commented#10: 1802072-10.patch queued for re-testing.
Comment #13
swentel CreditAttribution: swentel commentedNote, as a follow up - #element_validate would be a nice one to convert as well - it would make some field widgets more sane, but let's get this one in first.
Comment #14
swentel CreditAttribution: swentel commentedThanks fubhy!
Comment #15
fubhy CreditAttribution: fubhy commented#1805688: Support methods as #element_validate callbacks
Comment #16
webchickSame feedback as #1805688-8: Support methods as #element_validate callbacks; we need to pull that class out to a separate PSR-0 compatible file, and document it.
Comment #17
fubhy CreditAttribution: fubhy commentedYeaah... umh :D
Comment #18
fubhy CreditAttribution: fubhy commentedComment #19
fubhy CreditAttribution: fubhy commentedForgot to add the new file.
Comment #20
fubhy CreditAttribution: fubhy commentedWow, t'is early in the morning :/
Comment #21
Stalski CreditAttribution: Stalski commentedThx for the improvement. Looking good and a good thing we have tests for it!
Comment #22
webchickAwesome, thanks!
Committed and pushed to 8.x.
We'll need a change notice for this; can likely just be added to the existing FAPI one.
Comment #23
fubhy CreditAttribution: fubhy commentedDone.
http://drupal.org/node/1734540
Comment #24
swentel CreditAttribution: swentel commentedLooks good.
Comment #25
fubhy CreditAttribution: fubhy commentedDid I miss something? I don't see the commit!
@webchick I think you might have forgot to push this one. Maybe because the two issues for the patches that needed to be commited are nearly identical :)
Comment #26
fubhy CreditAttribution: fubhy commentedComment #27
fubhy CreditAttribution: fubhy commentedThe patch doesn't apply anymore.
Here is a re-roll.Yah... Or I simply upload an empty file.Comment #28
fubhy CreditAttribution: fubhy commentedComment #29
swentel CreditAttribution: swentel commentedAlright, let's get this in! :)
Comment #30
swentel CreditAttribution: swentel commentedChange notification is done
Comment #31
andypostmaybe leave one as was before?
Comment #32
fubhy CreditAttribution: fubhy commentedWell, why not. To me it is actually not really clear why we need a test for this at all. I mean it really just is a test for is_callable() vs is_function(). We should assume that both work properly :).
Comment #33
webchickHm. No, it appears I didn't commit it at all. :P Oops. :P Clearly, I need more sleep. :)
REALLY committed and pushed to 8.x this time. :P