This problem is completely understood and an interim solution is suggested.

It can be considered a bug in the sense that drupal_retrieve_form() makes the "unwarranted" (i.e., not OO-friendly) assumption that a $form_id can be a function but not a static class method.

It is a "Feature request" in the sense that a request is made for changes that better admit use of object-oriented techniques with Drupal7 forms.


Problem/Motivation

Because drupal_retrieve_form() uses a function_exists($form_id) test on line 749 of form.inc it fails to detect form builder callbacks to static methods of classes.

This makes it hard to use an encapsulating FormController with a delegating static createForm($form,&form_state,$builderMethod) method; compare with the Drupal7 Page controller project.

In advance, the solution: If the $form_id is in the form "$class::$method" the test needs instead to be method_exists($class,$method).

Detailed analysis

In the following, if the $form_id is for example '\Drupal\mymodule\MyClass::createForm', drupal_retrieve_form judges via function_exists that the $form_id is not a valid function/method to be called (although it gets called anyway eventually via call_user_func_array) and then hunts within $forms, which does not necessarily exist:

function drupal_retrieve_form($form_id, &$form_state) {
  $forms = &drupal_static(__FUNCTION__);
...
  if (!function_exists($form_id)) {//Drupal-7.25: form.inc 749: FAILS when $form_id is static class method
...
    $form_definition = $forms[$form_id];//Drupal-7.25: form.inc 764: point of failure 

If $forms does not exist then this causes a Notice: Undefined index: error.

Proposed resolution

I offer initially the following verbose solution, known only to work for my cases. It is not clear to me yet whether there are any possible side-effects, other than to prevent people using a double-colon '::' within form ids for any case other than static class method signatures:

  //if (!function_exists($form_id)) {//ORIGINAL
  
  //2013-01-03 Webel: extended to handle static class methods as form ids
    $sep = '::';
    $is_static_method = strpos($form_id,$sep);
    $function_exists = false;
    if ($is_static_method) {
        $explode = explode($sep,$form_id);
        $class = $explode[0];
        $method = $explode[1];
        $function_exists = method_exists($class,$method);
    }
    else $function_exists = function_exists($form_id);
    if (!$function_exists) {

The above enables one to encapsulate form builders, along with many other aspects of a form, in a FormController class, and thus promotes and supports more object-orientation already in Drupal7.

Remaining tasks

(reviews needed, tests to be written or run, documentation to be written, etc.)

I can perform tests as I am currently developing an entire object-oriented controller system for Drupal7, including form controllers (along with other object-oriented examples that integrate with Drupal7). However, I can't be sure how other people might wish to use the $form_id and whether other usages might break my simple static class method test test above (in short, whether there are other reasons to use '::' in a form id).

TODO: Document the new capability to support static class method signatures as form ids.

API changes

None, other than to document the new capability to support static class method signatures as form ids.

Comments

webel’s picture

Version: 7.23 » 7.25
Category: Feature request » Bug report
Issue summary: View changes
Issue tags: -class, -method, -static

Moved from a feature request to a bug report.

Updated from Drupal7.23 to Drupal7.25.

webel’s picture

Priority: Normal » Major
Issue summary: View changes

Priority updated to Major in awareness of Priority categories, because I consider compatibility with object-oriented techniques (and especially with object-oriented encapsulation of callbacks in OO controllers) a crucial matter, and I require this change for support of an upcoming major project/module release, an OO mapping system for Drupal7. If this is not included in future Drupal7 core, I will have to recommend a hacked version of form.inc to support my OO mapping system.

webel’s picture

Version: 7.25 » 7.x-dev
Status: Active » Needs review

Please find attached a Git diff patch against the very latest 7.x (dev beyond just released 7.36), which incorporates an original patch by @sammarks15 (to form.inc and ajax.inc) for a closely related issue (#2166371: Drupal 7.36: form.inc 1471: form_execute_handlers($type, &$form, &$form_state): $function($form, $form_state); Does not work for static class method 'Drupal\mymodule\FormContoller::handler') , as well as my additional tweaks to form.inc,, addressing both of these very closely related issues:

- #2165895: drupal_retrieve_form() issues Undefined index: warning on line 764 when static class method used as $form_id: hinders use of object-oriented FormController

- #2166371: Drupal 7.36: form.inc 1471: form_execute_handlers($type, &$form, &$form_state): $function($form, $form_state); Does not work for static class method 'Drupal\mymodule\FormContoller::handler'

drupal-oo_static_method_support_for_form_execute_handlers_and_drupal_retrieve_form_and_ajax_form_callback-2166371-2165895.patch

Since the patch covers 2 issues they are included by number in the verbose patch name, and no comment numbers are used, since it does not relate to a single comment.

I would be most grateful for community testing of this - and for inclusion of this in the next Drupal core release 7.37 - so that other developers can more easily explore my unique UML-friendly Object Oriented Example (OOE) tutorial module with a demonstration of a pure OO bridge for Drupal7.

webel’s picture

There are now instructions on how to download and apply this core patch and how to install the related tutorial module OOE = Object Oriented Examples = One Of Each at Download: a Drupal-7.x-dev patch to support the OO form controllers of the OOE tutorial module.

RdeBoer’s picture

Looks fine.
Please create a real patch or hyperlink to the one from the issue that includes the patch.

webel’s picture

@RdeBoer thanks for your initial community review. You wrote:

Please create a real patch or hyperlink to the one from the issue that includes the patch.

I am to sure what you mean be "real patch". My comment at https://www.drupal.org/node/2165895#comment-9793163 included a direct link to this core patch which I repeat here raw for clarity:

https://www.drupal.org/files/issues/drupal-oo_static_method_support_for_...

That Git diff patch is attached to this very closely related issue, #2166371: Drupal 7.36: form.inc 1471: form_execute_handlers($type, &$form, &$form_state): $function($form, $form_state); Does not work for static class method 'Drupal\mymodule\FormContoller::handler' at comment https://www.drupal.org/node/2166371#comment-9792067, and was combined with the changes to address this issue (at the suggestion of @sammarks15, who had adapted by code change suggestions as a preliminary patch).

RdeBoer’s picture

Hi webel,

Yes it's there, when you look carefully and read everything.

Sorry - I was wrong.

What time-pressed reviewers tend to do when confronted with an issue thread that sometimes goes back years, is to scan towards the bottom looking for the last patch. And they'll be looking for a visual clue of a "standard patch output", like the one in https://www.drupal.org/node/2166371#comment-9792067 -- with the little table shown "status" and "file".

Then they'll open and investigate the patch and should it contain hundreds of lines of "differences" that on closer inspection are nothing more than spaces, then their time is wasted and they're likely to be pissed off (not in your case, I hasten to add).

Try not to be too texty. Just saying, to improve your chances. That's all.

I second this patch... low risk, high gain.

Cheers,

Rik