Make Drupal PHP 5 compliant

One of the problems is the change of behaviour of array_merge() in PHP5. In PHP4, you can pass non-array arguments to array_merge, which is the same as wrapping the parameter in array:

array_merge('a', 'b') is the same as array_merge(array('a'), array('b'))

This patch breaks this behaviour and simply assumes that the arguments are always arrays. This is not always the case.

For example in menu_get_active_help():

  $return = module_invoke_all('help', $path);
  foreach ($return as $item) {
    if (!empty($item)) {
      $output .= $item ."\n";
    }
  }

I'm going to look for more instances and see if it's worth it to include an array_merge wrapper. Something like this:

function drupal_array_merge(&$a, &$b) {
  if (!is_array($a)) {
    if (!is_array($b)) {
      return array_merge(array($a), array($b));
    }
    else {
      return array_merge(array($a), $b);
    }
  }
  else {
    return array_merge($a, $b);
  }
}

(note: the code is a bit ugly for speed reasons and optimal usage of references)

Sorry, I used regular PHP tags rather than square bracketed ones. I got rid of the funky code as well:

function drupal_array_merge(&$a, &$b) {
  $aa = is_array($a) ? &$a : array($a);
  $bb = is_array($b) ? &$b : array($b);
  return array_merge($aa, $bb);
}

I don't think that's a good idea. Module_invoke_all is really an odd case: it simply throws together all arrays returned by the hooks without regard for duplicate elements. This is handy in several cases (_xmlrpc, _link) but annoying for others that do not return arrays.

Having the _help hook return single-element arrays doesn't make much sense. Having all hooks return arrays is madness.

The wrapper would only be used in those cases where we know we can get non-array arguments, not everywhere.

I could go either way on this. It clearly is crazy to require an array return value for every hook. We currently have a few possible situations:

1) Hooks that are only called via module_invoke(), and so array_merge() never comes into the picture.
2) Hooks that already return arrays, so module_invoke_all() has no problem.
3) Hooks that return no value, called via module_invoke_all().
4) Hooks that could return arrays or scalars, and which rely on the PHP 4 array_merge() semantics to end up with an array.

A quick scan through the hook API reference seems to indicate that only hook_help() falls into that last category, so that would be the biggest change to make to be able to go along with killes' suggestion. I would object to any resolution that required a change to hooks in category 1 or 3; we'd need to account for no-return-value hooks somehow if module_invoke_all were to presume array return values.

On the other hand, I don't think explicitly checking for scalars and making them arrays is that bad of a solution. My gut feeling is that it would be better to do this directly inside module_invoke_all() rather than in an array_merge() wrapper though, because it would force us to really examine each case of array_merge() and come up with the best solution rather than adding unnecessary checks everywhere.

I'm fine with Jonbob's suggestion then. The wrapper I suggested was just a way to reduce code duplication (it wouldn't be replacing array_merge everywhere).

Did I make a suggestion? I'd be okay with either changing hook_help() to return arrays, or changing module_invoke_all() to convert return values into arrays.

I was referring to your 'gut feeling':
"My gut feeling is that it would be better to do this directly inside module_invoke_all() rather than in an array_merge() wrapper though, because it would force us to really examine each case of array_merge() and come up with the best solution rather than adding unnecessary checks everywhere."

;).

Apologies for a dumb beginners question here but it seems the main developers list is rather hostile towards anything to do with PHP5 mods. I downloaded a recent CVS snapshot tarball and there is no sign of any of the patches on this page. Exactly where are these patches being applied ?

They have not been applied, which is why this issue is still marked "patch" and not "fixed."

It would be great if someone could (i) update and (ii) combine these patches in a single patch.

> It would be great if someone could (i) update and
> (ii) combine these patches in a single patch.

Here is an attempt at such a unified patch. It's probably all backwards compatible except for the "var" to "public" transform in xtemplate.inc, so it's not suitable to be applied to HEAD. I'm not sure what the PHP5 policy is as there are two issues, a) making drupal forwardly compatible with PHP5 so that, ideally, it passes running under E_ALL | E_STRICT and, b) having a separate branch specifically to take advantage of PHP5 semantics, especially exceptions.

A new patch for includes/common.inc. This one corrects the error_handler function to be able to handle the new STRICT type of errors.

I'm noticing a small problem with the latest patch in PHP 4.3.7 on the "default workflow" page. (and possibly elsewhere as well) Empty items are being added to the array returned by node_invoke_nodeapi, so I have changed the else to an elseif (isset(...)) in the attached patch.

I looked through the rest of the patch and doesn't look like any of the other changes need a fix of this nature. Can someone running PHP5 double-check this and make sure things still work after applying this patch?

Applied to CVS.

As far as xtemplate goes, I think the best solution is to move the xtemplate engine out of core and reimplement that bluemarine under chameleon or the upcoming phptemplate engine. Xtemplate is indeed braindead, and merits no forther attention from core developers IMO.

Implementing Bluemarine and Pushbutton in other template engines is not really a problem. The problem is that many sites out there that are using custom xtemplates. We'd need an automatic convertor for that to work.

Here is a new patch which solves an issue in node.module in a php4 compatible way.

If we are going to use that eval() hack, we should do it in a cleaner way. I came up with the slightly more elegant solution of using clone($obj) and declaring clone() as a function in PHP4. That way, code that requires clone() is kept clean, while PHP5 still recognizes it as clone ($obj).

if (version_compare(phpversion(), '5.0') < 0) {
  eval('
  function clone($object) {
    return $object;
  }
  ');
}

It works this way too, but then you should use references in the created clone function, because it doesn't matter in php4, and we do not want unnecessary copying.
Also it should compare the version to 5.0.0dev, because as I remember this issue affects those versions too.

So the code would be:

    // bugfix, issue: http://drupal.org/node/view/10490
    if (version_compare(phpversion(), "5.0.0dev", "<")) {
      eval('
        function clone(&$object) {
          return $object;
        }
      ');
    }
    $nodeclone = clone($node);

What if we change every such an usage of objects in Drupal core to arrays. I think it's not so hard to do, at least the part of replacing $var->prop to $var['prop'] is just a matter of time (or a clever regexp).

To stay compatible with function calls from contrib modules we could use an object2array($obj) conversion function.
I've made a little test to see whether this creates new problems, and it doesn't:

class SomeObj {
  public $a;
  private $b;

  function SomeObj () {
    $this->a = "A";
    $this->b = "B";
  }
}

function object2array($obj) {
  if (is_object($obj)) {
    return get_object_vars($obj);
  }
}

function change ($obj) {
  echo "from: "; var_dump($obj);
  $obj = object2array($obj);
// $obj->a = "X";
  echo "to: "; var_dump($obj);
}

$obj = new SomeObj();

echo "before: "; var_dump($obj);
change($obj);
echo "after: "; var_dump($obj);

When passing $obj to the change function, it is passed by reference in php5. One would think that after the function call it became an array, but because of the conversion php doesn't use the reference any more but a new variable.
If you used the commented line in the change function and commented the object2array call, you would see that the $obj->a property changed to "X" outside of the function too.

If you also think that this could be a good start in migrating to php5 then I could create a patch. I just didn't want to do something useless and waste my time :).

I applied the taxonomy fix. We still need to think of a good clone() solution.

LacKac: the reason I had clone() explicitly copy in PHP4 is so that we can adopt a policy of always passing objects by reference. This would make PHP4 and PHP5 behave the same.

In any case, such a change is quite big and will probably introduce more bugs. Let's avoid that for Drupal 4.5 for now.

There is another problem with changing to arrays. We cannot stay compatible with contribs because many of the core functions return object. But it's still the better way. If we go on with objects in this way it will produce more and more bugs like the node preview bug not just in the core but in contribs too.

We are not using real objects, just object-like arrays so we should get rid of them.

Applied to CVS.

Committed to HEAD. Thanks.

Two duplicates have cropped up for xtemplate's incompatibility with E_STRICT.

When I turn on display of all error messages on my PHP5 test install, I seem to get a ton of "var is deprecated" warnings from xmlrpc.inc too... maybe we need to fix them in the same way?

The problems with 'var' are the result of syntax differences between PHP4 and PHP5. I don't think we can get around them without providing two versions of the class definition (either in separate files, or with some conditional code magic, if possible).

Committed the file.inc patch to HEAD and DRUPAL-4-5.

Setting to 'active' until a new patch gets attached.

I committed a fix for the node previewing/form issue.

Now the only other problem I can think of is the xtemplate "deprecated var" warnings.

There is a new version for XTemplate, published on April 11, 2005.