[support] PHP 5.3 upgrade problems

[andy baxter]

Larry Garfield wrote:
> On Wednesday 14 October 2009 12:02:28 am andy baxter wrote:
>
>   
>> What I really want to know is, if I find the (relatively few) places in
>> the code where php5.3 is causing an issue that actually breaks site
>> functions, and apply the fix above (i.e. &$variable -> $variable) or
>> something similar, is this a good way of solving the problem for the
>> time being, given that the site goes public on thursday, or is it just
>> going to break something else, or cause other problems like security
>> holes or whatever?
>>
>> It seems to have stopped the error in the couple of places I've already
>> tried it, but it worries me to just convert a pass-by-reference into a
>> straight parameter pass like this. I like to understand what I am doing
>> rather than just apply a patch, which is why I was asking if anyone can
>> explain a bit more about what's going on here?
>>
>> cheers,
>>
>> andy
>>     
>
> What functions are we talking about?  The answer to that will vary per the 
> exact function being called.
>
> Vis, show us da codez!
>
>   
OK, as an example, this is the log entry for one of the errors that is 
coming up:

Type 	php
Date 	Tuesday, 13 October, 2009 - 18:10
User 	experiment <http://www.lancs.ac.uk/fass/drupal/expt/users/experiment>
Location 
http://www.lancs.ac.uk/fass/drupal/expt/profile/experimentalist/edit
Referrer 	http://www.lancs.ac.uk/fass/drupal/expt/profile/experimentalist
Message 	/Parameter 1 to theme_imagefield_widget() expected to be a 
reference, value given/ in 
//export/depts/fass/drupal/expt/includes/theme.inc/ on line /617/.
Severity 	error
Hostname 	83.216.128.236
Operations 	


This is one of the errors that actually breaks site functions - the 
upload image box doesn't come up on the edit profile page.

I have grepped for the function name and found this:
andy at anthill:/local/www/experiment$ grep -R 
'function.*theme_imagefield_widget' *
sites/all/modules/imagefield/imagefield.module:function 
theme_imagefield_widget_preview($item = NULL) {
sites/all/modules/imagefield/imagefield.module:function 
theme_imagefield_widget_item($element) {
sites/all/modules/imagefield/imagefield_widget.inc:function 
theme_imagefield_widget(&$element) {

I assume that the last declaration is the relevant one because of the 
'&$element'. This function looks like this:

/**
 * FormAPI theme function. Theme the output of an image field.
 */
function theme_imagefield_widget(&$element) {
  drupal_add_css(drupal_get_path('module', 'imagefield') 
.'/imagefield.css');
  return theme('form_element', $element, $element['#children']);
}


Here is another one, which doesn't break anything serious as far as I 
can tell:

Type 	php
Date 	Tuesday, 13 October, 2009 - 23:28
User 	experiment <http://www.lancs.ac.uk/fass/drupal/expt/users/experiment>
Location 	http://www.lancs.ac.uk/fass/drupal/expt/blog/previously-test/edit
Referrer 	http://www.lancs.ac.uk/fass/drupal/expt/blog/previously-test/edit
Message 	/Parameter 1 to content_diff() expected to be a reference, 
value given/ in //export/depts/fass/drupal/expt/includes/module.inc/ on 
line /471/.
Severity 	error
Hostname 	83.216.128.236
Operations 	


Same procedure:
andy at anthill:/local/www/experiment$ grep -R 'function.*content_diff' *
sites/all/modules/cck/includes/content.diff.inc:function 
content_diff(&$old_node, &$new_node) {
sites/all/modules/cck/includes/content.diff.inc:      $function = 
$field_types[$field['type']]['module'] . '_content_diff_values';
sites/all/modules/cck/includes/content.diff.inc:      $function = 
function_exists($function) ? $function : 'content_content_diff_values';
sites/all/modules/cck/includes/content.diff.inc:function 
content_content_diff_values($node, $field, $items) {
sites/all/modules/cck/includes/content.diff.inc:  function 
userreference_content_diff_values($node, $field, $items) {
sites/all/modules/cck/includes/content.diff.inc:  function 
nodereference_content_diff_values($node, $field, $items) {
sites/all/modules/cck/content.module:  if (module_exists('diff') && 
!function_exists('content_diff')) {

Here it looks like the first line above; the function looks like this:
/**
 * Implementation of hook_diff()
 */
function content_diff(&$old_node, &$new_node) {
  $result = array();
  // Prevent against invalid 'nodes' built by broken 3rd party code.
  if (isset($new_node->type)) {
    $type = content_types($new_node->type);
    $field_types = _content_field_types();
    foreach ($type['fields'] as $field) {
      // Ignore fields the current user is not allowed to view.
      if (!content_access('view', $field, NULL, $new_node)) {
        continue;
      }
      $function = $field_types[$field['type']]['module'] . 
'_content_diff_values
';
      $function = function_exists($function) ? $function : 
'content_content_diff
_values';
      $old_values = array();
      $new_values = array();
      if (isset($old_node->$field['field_name'])) {
        $old_values = $function($old_node, $field, 
$old_node->$field['field_name
']);
      }
      if (isset($new_node->$field['field_name'])) {
        $new_values = $function($new_node, $field, 
$new_node->$field['field_name']);
      }
      if ($old_values || $new_values) {
        $result[$field['field_name']] = array(
          '#name' => $field['widget']['label'],
          '#old' => $old_values,
          '#new' => $new_values,
          '#weight' => $field['widget']['weight'],
          '#format' => array(
            'show_header' => FALSE,
          ),
        );
      }
    }
  }
  return $result;
}

I could just remove the & in front of the parameter name in both cases, 
but doing that without understanding what I am doing makes me nervous. 
Why would another function be passing a variable by value when the 
function expects a reference? Is this just bad coding which PHP <5.3 
used to cover for, or some older feature which has been removed?

You could tell me in each case whether you think this hack is safe to 
use, or suggest another one, but what I really need is some better 
understanding of what's happening and why this situation has come about 
so I can do it myself - there are over 400 files in my installation 
which include functions with variable declarations like this, so 
presumably I can't just change all of them from &$ to $.

andy