With more subsystems moving to OO code (speaking from my own "Field API as OO Plugins" sandbox), a lot of code gets moved from procedural functions in .module or .inc files to methods in class files.

As a typical gain from OO-ification, this helps grouping what was previously a flat list of functions (possibly grouped in .inc files if you took care of loading them properly) into logical groups, and reduces the amount of code that gets loaded on each page request.

One thing that is currently forced to stay in procedural land are FAPI and render API callbacks (#element_validate, #validate, #submit, #after_build, #pre_render, #post_render...), since the code that invokes them expects expects a func name in a flat string.
(another thing is theme hooks and (pre)processors, but I'll leave that out for another issue)

Those callbacks get orphaned in .module files while all the associated logic surrounding them has moved to a class file. Feels like a sore thumb, and mitigates the gains in terms of code organization and amount of loaded code.

No patch for now, but something like the following would allow methods to be used as callbacks :

function _form_validate(&$elements, &$form_state, $form_id = NULL) {
  [...]
       foreach ($elements['#element_validate'] as $function) {
-        $function($elements, $form_state, $form_state['complete_form']);
+        if (is_string($function)) {
+          $function($elements, $form_state, $form_state['complete_form']);
+        }
+        elseif (is_array($function)) {
+          $object = $function[0];
+          $method = $function[1];
+          if (is_object($object)) {
+            $object->$method($elements, $form_state, $form_state['complete_form']);
+          }
+          else {
+            // Static method in a class.
+            $object::$method($elements, $form_state, $form_state['complete_form']);
+          }
+        }
       }

calling code :

  $form['#element_validate'] = 'my_module_my_function';
  $form['#element_validate'] = array($this, 'myMethod');
  $form['#element_validate'] = array(get_class($this), 'myStaticMethod');

Maybe one thing to consider : the 2nd form de facto puts classed objects inside $form/$render structures. That might be something we'd rather not encourage, since $form and $render arrays tend to get serialized...
Then maybe limit to the 3rd form and static methods ? Dunno.

Also, I don't think this issue needs to be be postponed on a hypothetical "OO-ify Form API" effort.

Comments

Issue summary:View changes

formatting error

of course, the patch snippet above would need to be duplicated (factorized ?) in the places where the other #callbacks are invoked...

I've been beating my head against this in D7. I realize it's not likely back-portable but just knowing it was going to be better in D8 would be wonderful. Sorry there's not much "added value" in this comment... I just wanted to do more than silently follow and say, yes, please, can we have this. :)

Michelle

Issue summary:View changes

grammar

<?php
foreach ($elements['#element_validate'] as $callback) {
    if (
is_callable($callback)) {
       
$callback($elements, $form_state, $form_state['complete_form']);
    }
?>

Should be enough to make it work. PHP 5.3 does not have the "callback" type hint, but it has everything else, my code would work. Althought it will work with PHP 5.2 too (not sure for array(object, methodname) thought).

Another viable syntax is:

<?php
$args
= array($elements, $form_state, $form_state['complete_form']);
foreach (
$elements['#element_validate'] as $callback) {
    if (
is_callable($callback)) {
       
call_user_func_array($callback, $args);
    }
?>

But I'm quite sure the first would work, even if $callback is an array(object, methodname).

EDIT: In PHP callables can be:
- A function name
- An object implementing __invoke()
- A closure
- An array whose first value is an object, and second value is a method name
And there's probably some more. Just calling them as a function actually works by design except for array...

Actually the $callback() does not work with an array, but the call_user_func() and call_user_func_array() do.

@pounard : most of these callbacks take their args by reference. That's why the current code uses $func(...) rather than c_u_f() / c_u_f_a().

Yes, true, I just read the PHP documentation. We can still put references into the $args array thought, it forces a call time reference but with no PHP warning and it works quite flawlessly.

Allowing form builders and validate/submit handlers to be class methods is being addressed in #1499596: Introduce a basic entity form controller.

$form['#element_validate'][] = array($this, 'validate');

One potential problem/concern with this is that we'd repetitively store $this many times within $form (and thus also in $form_state).

Cross-referencing #1499596: Introduce a basic entity form controller, which hit this problem as well (but only to limited extent).

Is this really a problem? It's a simple pointer.
EDIT: It will be as soon as the form gets in cache, I guess.

@pounard:

It will be as soon as the form gets in cache, I guess.

Yes, I had some problem with this in the issue referenced above: the form controller is stored in the form state and the reference has to be updated after the form submission, otherwise changes to the form controller fields are not propagated to the form state.

Forms should probably not be cached, this would be the ultimate solution. There is not enough conceptual and pragmatic separation between the form object, the current form state, form processing handlers, and form values in Drupal.

Or we could update the references automatically if the form is fetched form cache, provided that there is a reliable way to get the new reference. In the other issue there is.

This would work, but this would be complicated and just a dirty workarround for an API that reaches its own limits. I know that FAPI is complex and has a lot of history, and probably won't radicaly change today, so I guess it's the only solution for you.

Yes, see the end of the OP. Storing classed objects within $form is problematic.
Adds a lot of overhead on the size of the cached serialized $form, and objects with closures will simply break (and we're still not too sure on how the DICs will leak into various objects)

In my "Field API as plugins" branch, I hacked form.inc a bit to allow methods as #validate callbacks, but only static methods - thus, what you put in $form['#validate'] is only array('className', 'methodName'). A bit more restrictive, but then again a FAPI callback is currently supposed to receive all its params through the $form / $form_state arguments, so forbidding FAPI "methods as callbacks" to depend on the state of an instanciated object is kind of in line with what we currently have...

Well, explictly forbidding them would be problematic for entity forms where a workaround exists.

The form cache is required for forms that are very complex/resource-intensive to construct, and also for stuff like Ajax.

However, it is somewhat true that we're slightly abusing the form cache for entity forms currently. These forms actually want to cache the $form_state only, without the therein contained 'complete_form' (i.e., $form).

Technically, for the scope of this issue, I'd think we'd want to store a pointer to a classed object in the $form_state. How about this?

  $form_state['classes']['field_instance_field_body_123'] = $plugin_instance;
...
  $form['#element_validate'][] = array($plugin_instance->getID(), 'validate');
...
...
  foreach ($element['#element_validate'] as $callback) {
    if (is_array($callback)) {
      list($class_id, $method) = $callback;
      $form_state['classes'][$class_id]->$method($element, $form_state);
    }
    else {
      $callback($element, $form_state);
    }
  }

One potential problem/concern with this is that we'd repetitively store $this many times within $form (and thus also in $form_state).

True. Serialize() is intelligent enough to handle object identities properly though, however that only works for arrays that are serialized at whole. Thus, with having the same object in $form and $form_state we would still end up with two serializations of the object with the current split form and form-state cache. If we'd combine both caches that shouldn't be a problem any more though.

Still, I dislike having all the objects referenced in $form. All form-related objects and data belongs into the form state.

list($class_id, $method) = $callback;
$form_state['classes'][$class_id]->$method($element, $form_state);

Interesting idea. What about using a parents array + drupal_array_get_nested_value() for navigating through the form state? So one could use meaningful object locations in form state instead?
E.g.

<?php
list($parents, $method) = $callback;
$object = drupal_array_get_nested_value($form, $parents);
$object->$method($element, $form_state);
?>

Going further, I really like having form controller classes for implementing forms in the first place, similar to what #1499596: Introduce a basic entity form controller is coming up with. So, the controllers methods could be the default #validate and #submit, instead of magic callback detection. If we'd enforce that and encourage class base form controllers, we could even do away with form specific custom including, what would remove a huge pain point in re-using forms... (well that belongs into another issue, so stopping here...)

Issue tags:+Entity forms

Marking as follwup of #1499596: Introduce a basic entity form controller: we need to ensure that all the FAPI callbacks can accept object methods. Better: any callable, for consistency.

Issue summary:View changes

Doh.