Replace function_exists() with is_callable() in form.inc as part of #1002080: [Policy, no patch] Normalize on usage of is_callable() instead of function_exists().

CommentFileSizeAuthor
#3 drupal_2040559_3.patch2 KBXano
#3 interdiff.txt3.48 KBXano
#1 drupal_2040559_1.patch3.44 KBXano
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Status: Active » Needs review
FileSize
drupal_2040559_1.patch3.44 KB
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Needs review » Needs work 
+++ b/core/includes/form.inc
@@ -1115,12 +1115,12 @@ function drupal_prepare_form($form_id, &$form, &$form_state) {
+    elseif (is_callable($form_id . '_validate')) {
       $form['#validate'][] = $form_id . '_validate';
...
     // handler for the shared $form_id.
-    elseif (isset($form_state['build_info']['base_form_id']) && function_exists($form_state['build_info']['base_form_id'] . '_validate')) {
+    elseif (isset($form_state['build_info']['base_form_id']) && is_callable($form_state['build_info']['base_form_id'] . '_validate')) {
       $form['#validate'][] = $form_state['build_info']['base_form_id'] . '_validate';

@@ -1132,12 +1132,12 @@ function drupal_prepare_form($form_id, &$form, &$form_state) {
-    elseif (function_exists($form_id . '_submit')) {
+    elseif (is_callable($form_id . '_submit')) {
       $form['#submit'][] = $form_id . '_submit';
...
-    elseif (isset($form_state['build_info']['base_form_id']) && function_exists($form_state['build_info']['base_form_id'] . '_submit')) {
+    elseif (isset($form_state['build_info']['base_form_id']) && is_callable($form_state['build_info']['base_form_id'] . '_submit')) {
       $form['#submit'][] = $form_state['build_info']['base_form_id'] . '_submit';

I think these should stay as function_exists() because that's exactly what's being checked here: "Is there a function called $foo_submit() that I can auto-magically call as a submit handler?" There's no chance that this is going to be an object, because you can hardly append '_submit' to an object.

+++ b/core/includes/form.inc
@@ -2107,7 +2107,7 @@ function _form_builder_handle_input_element($form_id, &$element, &$form_state) {
-        if (function_exists($value_callback)) {
+        if (is_callable($value_callback)) {
           $element['#value'] = $value_callback($element, $input, $form_state);

@@ -2122,7 +2122,7 @@ function _form_builder_handle_input_element($form_id, &$element, &$form_state) {
-      if (function_exists($value_callback)) {
+      if (is_callable($value_callback)) {
         $element['#value'] = $value_callback($element, FALSE, $form_state);

This change looks great, but the direct function call should be replaced by call_user_func() in order to get object support. This would need a test, as well.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Status: Needs work » Needs review
FileSize
interdiff.txt3.48 KB
drupal_2040559_3.patch2 KB

Thanks for the review :) Find and replace wasn't such a good idea after all.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Needs review » Needs work
Issue tags: +Needs tests

From the interdiff: 

+++ b/core/includes/form.inc
@@ -5159,7 +5159,7 @@ function batch_process($redirect = NULL, $url = 'batch', $redirect_callback = NU
-        call_user_func($batch['url'], $options);
+        call_user_func($function, $batch['url'], $options);

Ohh, nice. I didn't catch that.

Patch looks great now, but I think we need some tests.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented

How do you suggest we test this? The only way is to create a custom element in system_test.module that uses class methods, and then the class with the methods, but that may be a little overkill.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

I don't think that is overkill. Look at the existing form_test.module for how this could look.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented

The #value_callback method callbacks are tested in #2041367: Element values retrieved from top to bottom instead of bottom to top.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Category: feature » bug

We apparently already have a change record that says this should be possible, so this issue technically fixes a bug.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

I think you need call_user_func_array for the ones taken by reference.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented

I think you need call_user_func_array for the ones taken by reference.

Are you referring to the batch API redirect_callback?

I don't think that is overkill.

I'm still not sure. It's an extra 150 lines of test code just to make sure that two calls also support classes.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Status: Needs work » Needs review
Issue tags: -Needs tests

#3: drupal_2040559_3.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal_2040559_3.patch, failed testing.

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

Added parent issue.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Title: Replace function_exists() with is_callable() in form.inc » Replace function_exists() with is_callable() in FormBuilder
Issue summary: View changes

form.inc is now \Drupal\Core\Form\FormBuilder

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Closed (duplicate)

Was done in #2072995: Move FAPI callbacks for file/image widgets in classes, the only remaining function_exists in FormBuilder are when concatenating a string:
elseif (function_exists($form_id . '_submit')) {