Object-valued by-reference parameters in Ubercart - Trouble with PHP 5.3 call_user_func_array
| Project: | Ubercart |
| Version: | 6.x-2.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
| Issue tags: | PHP 5.3 |
To make ubercart ready for 5.3, we need to face some issues with call_user_func_array.
Object-valued by-reference parameters
Objects in PHP 5.x are passed as pointers. All ubercart modules have PHP 5 as a requirement, so no need to keep the legacy "&" in the function signature. This way we can already get rid of a bunch of errors.
Found this in the following places:
uc_product_content_fieldapi()
hook_cart_item($op, &$item)
hook_order_product_alter(&$product, $order)
etc
A fulltext search for "&$" will find them all. Check if the argument is supposed to be an object. If yes, replace the "&$" with "$".
I will not be surprised if I find some other places with call-by-reference issues, where the argument in question is not object-valued. I will let you know when I run into these things.
#1
Just to be clear, are these errors only visible with strict error reporting?
#2
I should have noted down the exact error message.. now I fixed all of them and forgot how exactly to reproduce. What I can say is:
- I did not have E_STRICT enabled (searched my php.ini and my drupal installation). php.net says that E_STRICT needs to be explicitly activated, so I assume that's not the case on my system.
- You need PHP 5.3 to reproduce the error.
- Similar errors have been found in core Drupal, see http://drupal.org/node/360605, http://drupal.org/node/457532. and we can expect more of them popping up in contributed modules.
If you upgrade to PHP 5.3 with an unpatched D6, you will sooner or later notice some of them. But sorry again for not having the exact error message.
#3
I still didn't manage to reproduce, but here is the message from apache's error.log:
PHP Warning:Parameter 2 to uc_attribute_cart_item()
expected to be a reference,
value given in D:\htdocs\mesport\includes\module.inc on line 471
It must be one of the calls to module_invoke_all('cart_item', ..)
#4
kk - yeah, well at least it's just noted by PHP as a warning. Definitely needs to be fixed, but I don't think I need to tag it as a release blocker. If we can get it patched, we can get this fixed for the 2.1.
#5
I recommend to do it all in one go. Don't wait for all the theoretically possible warnings to be reported by someone, just do a text search for "&$" and fix all places where the parameter is clearly an object.
Ubercart is in the fortunate situation that it does not need to support PHP 4.x, which does not pass objects around as pointers.
#6
+1 to that. : D
Yeah, that's the goal - get a patch going that knocks it all out and commit it ASAP. We're just not giving time atm to new issues, b/c we have set a deadline for a 2.0. For more information, see: http://bit.ly/wlxlz
#7
hmm, trying to fix these errors in 6.x-2.x-dev (mid Aug release) and PHP 5.3 on a local copy of a production site. They are everywhere, in so many of the sub modules!
This is what I got when I added something to my cart them removed it:
* warning: Parameter 2 to uc_credit_order() expected to be a reference, value given in C:\xampp\htdocs\hlr\includes\module.inc on line 471.* warning: Parameter 2 to uc_google_checkout_order() expected to be a reference, value given in C:\xampp\htdocs\hlr\includes\module.inc on line 471.
* warning: Parameter 2 to uc_payment_order() expected to be a reference, value given in C:\xampp\htdocs\hlr\includes\module.inc on line 471.
* warning: Parameter 2 to uc_quote_order() expected to be a reference, value given in C:\xampp\htdocs\hlr\includes\module.inc on line 471.
* warning: Parameter 2 to uc_googleanalytics_order() expected to be a reference, value given in C:\xampp\htdocs\hlr\includes\module.inc on line 471.
* warning: Parameter 2 to uc_credit_order() expected to be a reference, value given in C:\xampp\htdocs\hlr\includes\module.inc on line 471.
* warning: Parameter 2 to uc_google_checkout_order() expected to be a reference, value given in C:\xampp\htdocs\hlr\includes\module.inc on line 471.
* warning: Parameter 2 to uc_payment_order() expected to be a reference, value given in C:\xampp\htdocs\hlr\includes\module.inc on line 471.
* warning: Parameter 2 to uc_quote_order() expected to be a reference, value given in C:\xampp\htdocs\hlr\includes\module.inc on line 471.
* warning: Parameter 2 to uc_googleanalytics_order() expected to be a reference, value given in C:\xampp\htdocs\hlr\includes\module.inc on line 471.
* warning: Parameter 2 to uc_credit_order() expected to be a reference, value given in C:\xampp\htdocs\hlr\includes\module.inc on line 471.
* warning: Parameter 2 to uc_google_checkout_order() expected to be a reference, value given in C:\xampp\htdocs\hlr\includes\module.inc on line 471.
* warning: Parameter 2 to uc_payment_order() expected to be a reference, value given in C:\xampp\htdocs\hlr\includes\module.inc on line 471.
* warning: Parameter 2 to uc_quote_order() expected to be a reference, value given in C:\xampp\htdocs\hlr\includes\module.inc on line 471.
* warning: Parameter 2 to uc_googleanalytics_order() expected to be a reference, value given in C:\xampp\htdocs\hlr\includes\module.inc on line 471.
* warning: Parameter 2 to uc_credit_order() expected to be a reference, value given in C:\xampp\htdocs\hlr\includes\module.inc on line 471.
* warning: Parameter 2 to uc_google_checkout_order() expected to be a reference, value given in C:\xampp\htdocs\hlr\includes\module.inc on line 471.
* warning: Parameter 2 to uc_payment_order() expected to be a reference, value given in C:\xampp\htdocs\hlr\includes\module.inc on line 471.
* warning: Parameter 2 to uc_quote_order() expected to be a reference, value given in C:\xampp\htdocs\hlr\includes\module.inc on line 471.
* warning: Parameter 2 to uc_googleanalytics_order() expected to be a reference, value given in C:\xampp\htdocs\hlr\includes\module.inc on line 471.
* warning: Parameter 2 to uc_credit_order() expected to be a reference, value given in C:\xampp\htdocs\hlr\includes\module.inc on line 471.
* warning: Parameter 2 to uc_google_checkout_order() expected to be a reference, value given in C:\xampp\htdocs\hlr\includes\module.inc on line 471.
* warning: Parameter 2 to uc_payment_order() expected to be a reference, value given in C:\xampp\htdocs\hlr\includes\module.inc on line 471.
* warning: Parameter 2 to uc_quote_order() expected to be a reference, value given in C:\xampp\htdocs\hlr\includes\module.inc on line 471.
* warning: Parameter 2 to uc_googleanalytics_order() expected to be a reference, value given in C:\xampp\htdocs\hlr\includes\module.inc on line 471.
* warning: Parameter 2 to uc_credit_order() expected to be a reference, value given in C:\xampp\htdocs\hlr\includes\module.inc on line 471.
* warning: Parameter 2 to uc_google_checkout_order() expected to be a reference, value given in C:\xampp\htdocs\hlr\includes\module.inc on line 471.
* warning: Parameter 2 to uc_payment_order() expected to be a reference, value given in C:\xampp\htdocs\hlr\includes\module.inc on line 471.
* warning: Parameter 2 to uc_quote_order() expected to be a reference, value given in C:\xampp\htdocs\hlr\includes\module.inc on line 471.
* warning: Parameter 2 to uc_googleanalytics_order() expected to be a reference, value given in C:\xampp\htdocs\hlr\includes\module.inc on line 471.
nice.
Not all instances are objects, some are arrays. I imagine in some cases it might actually be better to pass a reference, so the calling function ought to be changed rather than the called function definition.. Is anyone working on this at the moment?
#8
Probably not, since it's a new issue that's not considered a release blocker by rszrama.
#9
#10
subscribing
#11
ok, I decided to have a crack at this. I've attached a patch file that removes the ampersands from the function definitions mentioned above and a few others that I was getting errors for.
#12
hi! sorry for a newbie question, but how do i apply the patch? im using a localhost running on windows xp sp2 and xampp with php 5.3, been scouring the net the whole weekend but no luck, i have another machine that has xampp and the lower version of php and the ubercart is working perfect, but with my fresh install i can't get my images working and getting a lot of those warning msgs. thanks in advance.
#13
I've never done it with windows, but this page should help: http://drupal.org/patch/apply
That patch should be applied from within the ubercart folder.
#14
I've installed CygWin and run the patch utility, but I got erros says something line "cannot find line.." and it went thru all the lines of the file im trying to patch but just kept on skipping it..help plsss..or should i downgrade back to php 5.2?
#15
Downgrading to php 5.2 mightn't be such a bad idea for a production site as there are still quite a lot of drupal modules that cause warning messages when using php 5.3, and there may well be more ubercart/php5.3 issues that I didn't spot and hence didn't include in that patch.
If you open that patch file in a text editor though it's very easy to see what it's doing. The lines marked with "-" are being removed, the lines marked with "+" are being added. All it really ends up doing is removing ampersand (&) symbols from a bunch of different lines.
#16
Patch seems to work with both PHP 5.2 and 5.3, once I filtered out all of the E_DEPRECATED errors with Devel. I'd like to get a few more people testing it, though.
Make sure you're applying the patch to the 2.x-rc7 version of Ubercart
#17
I think this needs to be field-tested by sites with contrib and custom modules, too, just in case... not really sure any harm can come from the change, but you never know. : ? Also, can we wait till we have a 2.0 out to do this? This is hardly a pressing issue.
#18
I just built a fresh 6.14, added CCK, Views, a couple of basics, then uc 6.x-2.0-rc7, and I see tons of these errors. I'll try the patch, and watch as I install other modules and test stuff.
Downgrading to PHP 5.2 is not an option for me, as other components of this project require it, so hopefully this can make it into 2.0 final.
#19
@foo - you could just change your error reporting level/options so warnings aren't displayed to your end users. I'm not against the adjustment itself, but more against the amount of time such a patch will take to fully test versus what other issues (i.e. actual bugs) that time could be spent resolving.
#20
As far as I know, PHP 5.3 does not just produce some error messages, it also behaves differently with call_user_func_array. I would try this, but I just downgraded my own PHP..
The following code could be interesting for a test with PHP 5.2 and PHP 5.3:
<?php
function foo(&$x) {
echo "<br/>$x";
}
$args = array('message1');
call_user_func_array('foo', $args);
call_user_func_array('foo', array('message2'));
?>
#21
While drasgardian's patch in #11 removes the errors, I'm not so certain that removing & from the function definition is the best way to handle it. Like joehudson mentioned, sometimes we're not passing objects to these hooks, so those parameters will need the & in the function definition.
I also think it's good idea to leave the &, even when we know only objects will be used there. This acts as a reminder to any developer that's looking over the API that the function will use it as a reference, and thus could have side effects on that variable. This sounds much easier than trying to remember if you passed an object to the function or not.
I've tested checkout and order administration on PHP 5.2 and 5.3. So far I haven't seen any warnings. Make sure you are using Drupal 6.14 because that is the version that got the changes to make it 5.3 compatible.
#22
For PHP 5.x+ developers it is a known fact that objects are passed as pointers to the object, not as copies. This is in fact not the same as by-reference parameters, but usually the difference doesn't matter that much.
For instance.
<?phpfunction foo($object, &$object_by_ref) {
$object->x = 'foo was here';
$object = new stdClass;
$object->x = 'foo was here*';
$object_by_ref->x = 'foo was here';
$object_by_ref = new stdClass; // replaces with a different object
$object_by_ref->x = 'foo was here *';
}
$obj1 = new stdClass;
$obj2 = new stdClass;
foo($obj1, $obj2);
echo $obj1->x; // "foo was here"
echo $obj2->x; // "foo was here *"
?>
Usually the effect with the by-ref parameter is not intended. We want to change values in the object, but we don't want to change the pointer to that object.
I would love if we could do this with type hints, but often we don't know the exact class or interface :(
http://www.stubbles.org/archives/5-My-wishlist-for-PHP-6,-pt1-The-object...
#23
I tried the patch from #21 but it broke the ability to retrieve shipping quotes. I've attached an updated version which changes this
foreach (module_list() as $module) {$func = $module .'_cart_item';
if (function_exists($func)) {
// $product must be passed by reference.
$result[] = $func('can_ship', $product);
}
}
to this
foreach (module_list() as $module) {$func = $module .'_cart_item';
if (function_exists($func) && $func('can_ship', $product)) {
// $product must be passed by reference.
$result[] = TRUE;
}
}
#24
Whoops. Nearly forgot to mention that I committed this. Thanks for catching the shipping quotes bug.
#25
I just did a fresh install earlier today from UberDrupal and am getting the warning:
warning: Parameter 1 to admin_menu_admin_menu() expected to be a reference, value given in /opt/lampp/htdocs/uberdru/includes/module.inc on line 471.
I checked line 471 and there are no occurrences of &$ in that bit of code.
#26
Not sure that's a problem w/ our modules... UberDrupal will automatically install the Admin Menu module which might not be PHP 5.3 compatible.
#27
Very correct! Turned off the menu and no more message. Thanks Ryan!
#28
Automatically closed -- issue fixed for 2 weeks with no activity.