In a theme I was debugging, a routine called "l" with a NULL for the value of $options (third argument).

This generates an error in line1396 when the code tries to add an array to $options (since it isn't an array).

My simple fix is to replace:

 $options += array(
      'attributes' => array(),
      'html' => FALSE,
    );

with:

  $def = array( 'attributes' => array(), 'html' => FALSE, );
  if(is_array($options)) $options += $def;
  else $options = $def;

Comments

rszrama’s picture

rszrama commented

Haven't tested the bug/fix, but you'd need to rewrite this per Drupal's coding standards and make it a patch for it to be committed:

  $default = array(
        'attributes' => array(),
        'html' => FALSE,
      );

  if (is_array($options)) {
    $options += $default;
  }
  else {
    $options = $default;
  }

I dunno if it would be acceptable or not, but this could be even trimmer with an inline if (though I seem to remember being told to avoid those in the past).

jody lynn’s picture

jody lynn
she/her
Primary language English
commented
Status: Active » Closed (works as designed)

In my opinion this is the correct behavior and l('foo', 'node/x', NULL) is incorrect syntax. The third argument, if specified, needs to be an array.

dman’s picture

dman commented

Still, many of the internal APIs should be a lot more bullet-proof and paranoid than they are.
They are public APIs - that means they are required to be more robust code than average, even if that means fixing a few input problems.

Hundreds of errors are possible when an array-that-is-not-an-array gets iterated or something. Sometimes the fatal error doesn't pop until a few levels deeper, like with a bad SQL call.

Have you never passed a wrong parameter to the form API and seen it go down in flames? Try passing an #options value as null or integer.

It is - I think - much better coding practice to assert is_array where it matters, and where it can be caught, rather than pass bad values further in.

For the code, I'd usually go:

  if (! is_array($options) ) {
    $options = array();
  }
  $options += array(
    'attributes' => array(),
    'html' => FALSE,
  );
...

Or to be concise ...

  $options = (is_array($options)) ? $options : array();

.dan.

sumgai’s picture

sumgai commented

I just bumped into the same issue with the url() function.

I guess I just have some places in my code where the $options parameter winds up being null and I don't check it before it gets passed to the Drupal API.

It seems that it would be more efficient if the Drupal functions checked rather than have all of the calling routines always check?.

m

rszrama’s picture

rszrama commented

Perhaps... but I've never actually run into this issue in module development. I guess I'd just keep api.drupal.org handy and check to make sure you're passing the proper empty parameters.

sumgai’s picture

sumgai commented

What utility do you guys use to generate patches?

I've been using WinMerge lately, but it doesn't generate the same format patch as your tool seems to.

I'd like to offer a few patches that I apply to all Drupal releases.

Thanks,

m

dman’s picture

dman commented

More efficient .. some folk will debate that, but I think it's better, defensive coding.

Having a public API call crash the system because it got unexpected input is like having an application crash because someone pressed a button they weren't supposed to.

_private functions have the privilege of assuming their input is clean
Module functions should be slightly forgiving, but it's not a problem.

Documented API functions intended to be called by everyone from everywhere should either trigger_error, fail gracefully, or - in this case - massage the input into the logical default.