When using PHP 5.3, the above error is generated when field_file_save_file() is called.

The attached patch fixes this (patch is against 6.x-3.2, but it will probably work on latest dev too).

CommentFileSizeAuthor
#15 755558-morecomments.patch833 bytesroderik
#13 755558.NOT_.patch1.22 KBroderik
#13 755558.patch754 bytesroderik
filefield-php-5.3.patch591 bytesyhager
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented
Status: Needs review » Needs work

This seems like a very strange way to fix this problem, since it depends on a calltime pass-by-reference. I think we could fix it without causing warnings in other PHP installations just by initializing $file as $file = array(). Making something by reference with an & sign accomplishes the same thing indirectly, but it would cause a warning on most PHP installations.

yhager’s picture

yhager CreditAttribution: yhager commented

I am not sure what you suggest here. filefield_validate_associate_field() modifies the $file object. For that to work it must get a reference to the object, as the function signature shows.
However, in PHP 5.3, calling the validator using call_user_func_array() will only pass arguments by value, regardless of the function signature. The patch above does just that - sends a reference of the object.

Can you be more specific on how you suggest to solve this instead?

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

Sorry I was making the assumption that $file was undefined; I didn't actually look at the code with the patch applied.

What I'm worried about here is that putting an ampersand in front of a variable when you're calling a function results in, "Warning: Call time pass-by-reference depreciated" error, at least in every version of PHP up to 5.3. I haven't used 5.3 so I'm not sure if it occurs there also. Getting rid of a warning in 5.3 but causing one in 5.2 isn't going to be acceptable. I find it highly unusual that 5.3 would require you to explicitly cast the reference when all objects are already passed by reference in PHP 5 and higher.

yhager’s picture

yhager CreditAttribution: yhager commented

I agree that a solution should not generate errors in 5.2.

From call_user_func_array documentation:

Note: Referenced variables in param_arr are passed to the function by a reference, others are passed by a value. In other words, it does not depend on the function signature whether the parameter is passed by a value or by a reference.

I have made a short test program:


function lolcat(&$var) {
  $var = "I'm in yur' var";
}

echo "PHP-". phpversion() . "\n";
$a='lolcat';
$args = array($a);
call_user_func_array('lolcat', array($a));
echo "1. value is: $a\n";
$a='lolcat';
call_user_func_array('lolcat', array(&$a));
echo "2. value is: $a\n";

the first call is what used today by filefield calling its validators, the second is how it looks after the patch is applied. I ran it on PHP 5.2, and PHP 5.3:

PHP-5.2.13-pl0-gentoo
1. value is: lolcat
2. value is: I'm in yur' var

PHP-5.3.2

Warning: Parameter 1 to lolcat() expected to be a reference, value given in /home/yuval/lolcat.php on line 10
1. value is: lolcat
2. value is: I'm in yur' var

No error on PHP 5.2, and it works correctly.

Seeing the behavior of the first version in PHP 5.2 makes me think this is a real bug in all PHP versions, albeit does not generate an error message with 5.2.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

You should also check your php.ini file and see what this option is set to:

allow_call_time_pass_reference = Off

The default, afaik, is "Off", meaning that a call like function_name(&$variable) will throw a warning.

yhager’s picture

yhager CreditAttribution: yhager commented

True, default is off, but it seem call_user_func_array is not affected.
I have changed the value to off on PHP 5.2 (it does not exist in PHP 5.3) with the same results as above (no errors).

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

Immediately upon visiting node/add/story (with a FileField enabled on this type) I receive:

Warning: Call-time pass-by-reference has been deprecated; If you would like to pass it by reference, modify the declaration of array_unshift(). If you would like to enable call-time pass-by-reference, you can set allow_call_time_pass_reference to true in your INI file in /Users/nate/Sites/drupal6/sites/all/modules/filefield/field_file.inc on line 159

This seems to indicate exactly what I had described: This patch causes an error in PHP 5.2 and lower when allow_call_time_pass_reference is off (which is the default).

Reg’s picture

Reg CreditAttribution: Reg commented

I was looking at the patch and it looks like a similar problem as here: http://drupal.org/node/734556 .

You should probably effect a similar solution as theirs - the line:
$params[0] = &$image;

because a line like:
array_unshift($args, &$file);

as used in the above patch will throw a depreciated warning in PHP 5.3 because it uses call-time pass by reference.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

Thanks for that suggestion Reg. If using that approach, we'd first need to unshift something onto the array like an empty string, since there is already something in position 0. Then we can use your approach and it should work in all situations. Still it seems like there should be a reliable and easy way to accomplish a simple task like this.

justintime’s picture

justintime CreditAttribution: justintime commented

subscribing

imclean’s picture

imclean CreditAttribution: imclean commented

How's this coming along? I'm seeing the problem with Filefield 6.x-3.7 and Filefield Sources: #843152: warning: Parameter 1 to filefield_validate_associate_field() expected to be a reference

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

Patches appreciated, I'm pretty busy with a lot of things.

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
CreditAttribution: roderik commented
Status: Needs work » Needs review
FileSize
755558.patch754 bytes
755558.NOT_.patch1.22 KB

@ #9: I don't think there is a reliable and easy way to accomplish this. (As far as I understand, this is one of the reasons why the drupal_alter() call signature is somewhat odd. There is just no 5.2-and-5.3-generic way to call a function with a variable number of arguments, if some of these arguments are references.)

Just FYI: in other cases I have worked around call_user_func_array(), like in the first patch included here.
But that seems inconvenient & overkill here, if we know that:
- the first argument ($file) must always be passed as a reference
- no other arguments are ever passed as a reference
...because then we can keep using call_func_array().

The second attachment is the original patch, incorporating solution #8 to problem #7. As far as I know, there's no better solution than this.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

Thanks roderik. So to clarify, "755558.patch" is the suggested patch correct? It looks good to me, though the comments aren't very clear. I think explicitly stating that array_unshift() in PHP 5.3 breaks referenced variables would be good clarity.

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
CreditAttribution: roderik commented
FileSize
755558-morecomments.patch833 bytes

Yes, "755558.patch" is the suggested one. Credit to yhager - and yourself for testing.

I can never get comments right (last time I had a php5.3 patch I was told my comment was too long :p) and it depends on personal taste too. I think we should then state that any function except call_user_func* breaks referenced variables. How about this one?

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented
Version: 6.x-3.2 » 6.x-3.7
Status: Needs review » Fixed

Thanks committed. This will be included in the 3.8 version.

Status: Fixed » Closed (fixed)
Issue tags: -PHP 5.3

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue tags: +PHP 5.3

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.