syntax error in update_manager_update_form()

Can someone help me with this issue?

I think $context is not being used anywhere in this hook, so just removed unusmeter

Not able to reproduce the issue.

$context is not used at a glance. But this update_manager_update_form() is a form function and passed to drupal_get_form(). Please see update_menu() and you'll find $context is used. So the proper approach is #2, I believe. I just reroll it.

But I'm not sure this is a bug...

- https://3v4l.org/K38Xm

shows that it is an error in PHP 7.1 if just one argument is passed, but a warning in all others.

- https://3v4l.org/DH1q2

shows that the syntax is indeed valid.

So I think we could remove the "$form_state = array()", but changing $form_state to be a reference feels wrong as its not required.

@Fabianx, thank you for the review.

So I think we could remove the "$form_state = array()", but changing $form_state to be a reference feels wrong as its not required.

I see. I investigated the point to know we should keep $form_state as a value or change to a reference.

In form building process, Drupal system calls drupal_get_form() first. Then, drupal_get_form() calls drupal_build_form() and drupal_build_form() calls drupal_retrieve_form().

drupal_get_form():

function drupal_get_form($form_id) {
  $form_state = array();

  $args = func_get_args();
  // Remove $form_id from the arguments.
  array_shift($args);
  $form_state['build_info']['args'] = $args;

  return drupal_build_form($form_id, $form_state);
}

drupal_build_form():

function drupal_build_form($form_id, &$form_state) {

  ...

    $form = drupal_retrieve_form($form_id, $form_state);

  ...
}

drupal_retrieve_form():

function drupal_retrieve_form($form_id, &$form_state) {

  ...

  $form = array();
  // We need to pass $form_state by reference in order for forms to modify it,
  // since call_user_func_array() requires that referenced variables are passed
  // explicitly.
  $args = array_merge(array($form, &$form_state), $args);

  // When the passed $form_state (not using drupal_get_form()) defines a
  // 'wrapper_callback', then it requests to invoke a separate (wrapping) form
  // builder function to pre-populate the $form array with form elements, which
  // the actual form builder function ($callback) expects. This allows for
  // pre-populating a form with common elements for certain forms, such as
  // back/next/save buttons in multi-step form wizards. See drupal_build_form().
  if (isset($form_state['wrapper_callback']) && is_callable($form_state['wrapper_callback'])) {
    $form = call_user_func_array($form_state['wrapper_callback'], $args);
    // Put the prepopulated $form into $args.
    $args[0] = $form;
  }

  // If $callback was returned by a hook_forms() implementation, call it.
  // Otherwise, call the function named after the form id.
  $form = call_user_func_array(isset($callback) ? $callback : $form_id, $args);
  $form['#form_id'] = $form_id;

  return $form;
}

Then, the function update_manager_update_form() is invoked in the line $form = call_user_func_array(isset($callback) ? $callback : $form_id, $args); in drupal_retrieve_form(). Here, the second element of $args seems to be a reference &$form_state.

From this, I thought that form builder function should always take the second parameter $form_state as a reference even if it's not used.

Is my understanding right? I'm not confident and would like to get feedback. Thank you for your time.

Hah! That is quite some research.

The trick is that we _allow_ the parameter to be passed by reference, but we don't enforce it.

Since PHP 5.4 at least the called function can decide if they want to have it passed by reference or not.

In versions earlier it was always passed by reference if the caller passed a reference.

In that respect you are right:

In 5.2 $form_state is a reference, in 5.5 it is not a reference.

However:

- We already have the "wrong" behavior and don't want to change it unless absolutely needed (to keep backwards compatibility - even though a chance of conflict is < 0.000001% in this special case)
- form_state is not even used by the called function (as far as I can see), so in practice it does not matter.

IIRC we sometimes put a non-reference deliberately to show that the called code is explicitly _not_ changing the values.

Does that explanation clear things up a little?

@Fabianx, thank you so much for your kind and detailed explanation. I see. Your explanation really helps and I got clear perspective. Thank you.

I created a new patch which keeps the $form_state to be a value. The patch is so simple and I didn't create a interdiff.

Thank you! And thanks for your effort in these Drupal 7 issues! Very much appreciated!

And this is RTBC now!

Committed and pushed to 7.x, thanks!

Thank you!