Follow-up to #2652814: Allow the use of callbacks for element value callbacks and batch API redirect callbacks

Problem/Motivation

The form API is still stuck with using global functions as callbacks:

  $function = $form['whatever callback'];
  $result = $function($form, $form_state);

This inhibits the usage of proper callbacks and makes mixing OOP with D7 a complete mess.

Proposed resolution

Change the way these callbacks are invoked.

Remaining tasks

  1. In drupal_retrieve_form change $form_state['build_info']['base_form_id'] = $callback; to $form_state['build_info']['base_form_id'] = isset($form_definition['base_form_id']) ? $form_definition['base_form_id'] : $callback;
  2. In drupal_retrieve_form change function_exists($form_state['wrapper_callback']) to is_callable($form_state['wrapper_callback'])
  3. In drupal_get_form $form_id param doxygen add a blurb about hook_forms can be used to define forms in classes
  4. In hook_forms mention that callback can be any callable so forms can be defined in classes.
  5. In hook_forms doxygen add base_form_id as an option when callable is not a function name.

User interface changes

None

API changes

Several functions can now be any callable and hook_forms gains the optional base_form_id.

Data model changes

None

CommentFileSizeAuthor
#4 2652814--manual-testing--pass-do-not-commit.patch2.49 KBFabianx
#4 2652814--manual-testing--fail.patch933 bytesFabianx
#3 allow_the_use_of-2760609-2.patch5.08 KBFabianx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Fabianx created an issue. See original summary.

Fabianx credited chx.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented
Status: Needs work » Needs review
Issue tags: -Novice
FileSize
allow_the_use_of-2760609-2.patch5.08 KB

Some credit to chx for the idea and help on the implementation.

Unfortunately hook_forms() is 100% untested in Drupal 7.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented
FileSize
2652814--manual-testing--fail.patch933 bytes
2652814--manual-testing--pass-do-not-commit.patch2.49 KB

Here is a fail and pass patch with manually changing hook_forms() in trigger, which is not for commit.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented
Status: Needs review » Needs work
Issue tags: -Drupal 7.50 target +Drupal 7.60 target, +Needs tests, +Novice

Examples show that this needs works and tests. Moving to 7.60.

The last submitted patch, 4: 2652814--manual-testing--fail.patch, failed testing.

The last submitted patch, 4: 2652814--manual-testing--pass-do-not-commit.patch, failed testing.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented
Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented
Status: Needs work » Needs review
Issue tags: -Drupal 7.60 target, -Needs tests, -Novice +Drupal 7.50 target

I was again confused as it would not pass on PHP 5.3 in the first place ...

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented
Status: Needs review » Reviewed & tested by the community

#2652814: Allow the use of callbacks for element value callbacks and batch API redirect callbacks and this issue can share a CR

  1. +++ b/modules/system/system.api.php
@@ -1807,19 +1809,22 @@ function hook_form_BASE_FORM_ID_alter(&$form, &$form_state, $form_id) {
+ *     define the base form id.
...
+ *   - base_form_id: The base form id can be specified explicitly. This is

    ID 

  2. +++ b/modules/system/system.api.php
@@ -1807,19 +1809,22 @@ function hook_form_BASE_FORM_ID_alter(&$form, &$form_state, $form_id) {
+ *     wizard-alike form buttons that are the same for a variety of forms that

    wizard-like

  • stefan.r committed 6303a15 on 7.x 
    Issue #2760609: Allow the use of callbacks instead of global functions...
stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Drupal 7.50 target

Committed and pushed to 7.x, thanks!

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented

This got committed without any issue credits (I think the mysterious drupal.org bug struck again) so I'm going to roll this back and then recommit so people get credit in the commit message.

  • David_Rothstein committed 8ec2182 on 7.x 
    Revert "Issue #2760609: Allow the use of callbacks instead of global...
  • David_Rothstein committed dc22e33 on 7.x
    Issue #2760609 by Fabianx, stefan.r, chx: Allow the use of callbacks...
David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented
Title: Allow the use of callbacks instead of global functions in the Form API » Allow the use of callbacks instead of global functions in parts of the Form API

We are down to only two of these is_callable() patches in Drupal 7.50 (this one and #2747679: Ajax form callbacks can only be global functions).... need to think about the best way to do a change notice for that.

This one is actually different in that because the existing code already did a call_user_func_array(), you can actually do this on any PHP version (not just 5.4 and higher) and it should work.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented
Issue tags: -Needs change record +7.50 release notes

I took a stab at a change record here, for this issue and the other one combined: https://www.drupal.org/node/2761169

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.