Contrib.social

drupal_static() edge case bug and inconsistent comments

Open on Drupal.org →
Created on 13 May 2020, over 4 years ago
Updated 5 April 2023, over 1 year ago

drupal_static() is currently as follows (excluding some of the header comments). The code hasn't changed in a decade.

/**
 * @param $name
 *   Globally unique name for the variable. For a function with only one static,
 *   variable, the function name (e.g. via the PHP magic __FUNCTION__ constant)
 *   is recommended. For a function with multiple static variables add a
 *   distinguishing suffix to the function name for each one.
 * @param $default_value
 *   Optional default value.
 * @param $reset
 *   TRUE to reset one or all variables(s). This parameter is only used
 *   internally and should not be passed in; use drupal_static_reset() instead.
 *   (This function's return value should not be used when TRUE is passed in.)
 *
 * @return
 *   Returns a variable by reference.
 *
 * @see drupal_static_reset()
 */
function &drupal_static($name, $default_value = NULL, $reset = FALSE) {
  static $data = array(), $default = array();
  // First check if dealing with a previously defined static variable.
  if (isset($data[$name]) || array_key_exists($name, $data)) {
    // Non-NULL $name and both $data[$name] and $default[$name] statics exist.
    if ($reset) {
      // Reset pre-existing static variable to its default value.
      $data[$name] = $default[$name];
    }
    return $data[$name];
  }
  // Neither $data[$name] nor $default[$name] static variables exist.
  if (isset($name)) {
    if ($reset) {
      // Reset was called before a default is set and yet a variable must be
      // returned.
      return $data;
    }
    // First call with new non-NULL $name. Initialize a new static variable.
    $default[$name] = $data[$name] = $default_value;
    return $data[$name];
  }
  // Reset all: ($name == NULL). This needs to be done one at a time so that
  // references returned by earlier invocations of drupal_static() also get
  // reset.
  foreach ($default as $name => $value) {
    $data[$name] = $value;
  }
  // As the function returns a reference, the return should always be a
  // variable.
  return $data;
}

Right at the start, this immediately looks wrong to me:

  if (isset($data[$name]) || array_key_exists($name, $data)) {
    // Non-NULL $name and both $data[$name] and $default[$name] statics exist.
    ...
  }

(a) That logic is buggy, and certainly does not align with what the comments claim is happening.

(b) If we're going to fall back to testing array_key_exists($name, $data), then testing isset($data[$name]) beforehand is redundant.

(c) isset($data[$name]) can be TRUE when $name = NULL

(d) $default[$name] is not even looked at.

I can't actually tell for certain what the intended logic was, as there are multiple problems. That isset($data[$name]) is definitely wrong, though. We can get away without testing array_key_exists($name, $default) if we have established that array_key_exists($name, $data) (examining the rest of the code confirms that the one follows from the other), but we definitely need to check isset($name) as otherwise we will get this:

>>> drupal_static('', 'foo')
=> "foo"
>>> drupal_static(NULL)
=> "foo"

Which means Drupal can no longer perform a full reset of the static variables, because the interface for doing that is passing $name = NULL.

This happens because $data[NULL] = 'foo' actually gives us ['' => 'foo'] and also array_key_exists(NULL, $data) => TRUE.

I think this code should actually be:

  if (isset($name) && array_key_exists($name, $data)) {
    // Non-NULL $name, and both $data[$name] and (implicitly)
    // $default[$name] statics exist.
    ...
  }

However the following block is also conditional upon isset($name), so we should refactor it all to:

  if (isset($name)) {
    if (array_key_exists($name, $data)) {
      // Non-NULL $name, and both $data[$name] and (implicitly)
      // $default[$name] statics exist.
      ...
    }
    else {
      // Neither $data[$name] nor $default[$name] static variables exist.
      ...
    }
  }

and personally I am in favour of also putting the remainder of the function -- the "Reset all: ($name == NULL)" code -- into an else {...} clause for that initial if (isset($name)) statement.

I also think $default should be $defaults (plural) to match with $data (plural).

In all, I propose that the function be refactored like so:

function &drupal_static($name, $default_value = NULL, $reset = FALSE) {
  static $data = array(), $defaults = array();
  if (isset($name)) {
    // Check if we're dealing with a previously defined static variable.
    if (array_key_exists($name, $data)) {
      // Both $data[$name] and (implicitly) $defaults[$name] statics exist.
      if ($reset) {
        // Reset pre-existing static variable to its default value.
        $data[$name] = $defaults[$name];
      }
      return $data[$name];
    }
    else {
      // Neither $data[$name] nor $defaults[$name] static variables exist.
      if ($reset) {
        // Reset was called before any value for $name was set, so we should
        // not set anything ($default_value is not reliable in this case). As
        // the function returns a reference, we must still return a variable.
        // (Code using $reset does not use the return value).
        return $data;
      }
      // First call with new non-NULL $name. Initialize a new static variable.
      $defaults[$name] = $data[$name] = $default_value;
      return $data[$name];
    }
  }
  else {
    // Reset all: ($name == NULL). This needs to be done one at a time so that
    // references returned by earlier invocations of drupal_static() also get
    // reset.
    foreach ($defaults as $name => $value) {
      $data[$name] = $value;
    }
    // As the function returns a reference, we must still return a variable.
    return $data;
  }
}
🐛 Bug report
Status

Fixed

Version

10.1 ✨

Component
Bootstrap  →

Last updated 6 days ago

No maintainer
Created by

🇳🇿New Zealand jweowu

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024