Object-valued by-reference parameters in Ubercart - Trouble with PHP 5.3 call_user_func_array [#574066]

Comment	File	Size	Author
#39	ubercart-574066-39.patch	2.94 KB	univate
#29	ubercart-574066.patch	2.24 KB	monotaga
#23	574066_hook_references.patch	11.02 KB	drasgardian
#21	574066_hook_references.patch	11 KB	Island Usurper
#11	ubercart-6.x-2.x-dev.patch	8.51 KB	drasgardian

Comment #1

rszrama commented 11 September 2009 at 02:07

Just to be clear, are these errors only visible with strict error reporting?

Log in or register to post comments

Comment #2

donquixote commented 11 September 2009 at 02:42

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.

Log in or register to post comments

Comment #3

donquixote commented 11 September 2009 at 03:28

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', ..)

Log in or register to post comments

Comment #4

rszrama commented 11 September 2009 at 14:01

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.

Log in or register to post comments

Comment #5

donquixote commented 11 September 2009 at 14:34

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.

Log in or register to post comments

Comment #6

rszrama commented 11 September 2009 at 14:37

+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

Log in or register to post comments

Comment #7

joehudson commented 19 September 2009 at 23:34

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?

Log in or register to post comments

Comment #8

torgospizza

he/him

English

Portland, OR

commented 20 September 2009 at 18:22

Probably not, since it's a new issue that's not considered a release blocker by rszrama.

Log in or register to post comments

Comment #9

jbrauer commented 25 September 2009 at 05:28

Issue tags:

+PHP 5.3

Log in or register to post comments

Comment #10

drasgardian commented 25 September 2009 at 19:58

subscribing

Log in or register to post comments

Comment #11

drasgardian commented 25 September 2009 at 21:27

Status	File	Size
new	ubercart-6.x-2.x-dev.patch	8.51 KB

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.

Log in or register to post comments

Comment #12

NelM commented 28 September 2009 at 01:02

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.

Log in or register to post comments

Comment #13

drasgardian commented 28 September 2009 at 02:54

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.

Log in or register to post comments

Comment #14

NelM commented 30 September 2009 at 13:58

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?

Log in or register to post comments

Comment #15

drasgardian commented 30 September 2009 at 19:19

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.

Log in or register to post comments

Comment #16

Island Usurper commented 30 September 2009 at 19:42

Status:

Active

» Needs review

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

Log in or register to post comments

Comment #17

rszrama commented 30 September 2009 at 21:32

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.

Log in or register to post comments

Comment #18

foo commented 2 October 2009 at 14:32

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.

Log in or register to post comments

Comment #19

rszrama commented 2 October 2009 at 18:03

@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.

Log in or register to post comments

Comment #20

donquixote commented 2 October 2009 at 18:11

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'));
?>

Log in or register to post comments

Comment #21

Island Usurper commented 7 October 2009 at 14:47

Status	File	Size
new	574066_hook_references.patch	11 KB

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.

Log in or register to post comments

Comment #22

donquixote commented 7 October 2009 at 20:43

Title:

Object-valued by-reference parameters - Trouble with PHP 5.3 call_user_func_array

» Object-valued by-reference parameters in Ubercart - Trouble with PHP 5.3 call_user_func_array

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.

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.

<?php
function 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...

Log in or register to post comments

Comment #23

drasgardian commented 15 October 2009 at 11:09

Status	File	Size
new	574066_hook_references.patch	11.02 KB

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;
    }
  }

Log in or register to post comments

Comment #24

Island Usurper commented 21 October 2009 at 19:37

Status:

Needs review

» Fixed

Whoops. Nearly forgot to mention that I committed this. Thanks for catching the shipping quotes bug.

Log in or register to post comments

Comment #25

willazilla commented 30 October 2009 at 01:13

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.

Log in or register to post comments

Comment #26

rszrama commented 30 October 2009 at 03:54

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.

Log in or register to post comments

Comment #27

willazilla commented 1 November 2009 at 19:52

Very correct! Turned off the menu and no more message. Thanks Ryan!

Log in or register to post comments

Comment #28

15 November 2009 at 20:00

Status:

Fixed

» Closed (fixed)

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

Log in or register to post comments

Comment #29

monotaga commented 27 September 2010 at 17:55

Status:

Closed (fixed)

» Active

Status	File	Size
new	ubercart-574066.patch	2.24 KB

I'm using PHP 5.3. and finding this error in a few places:

warning: Parameter 2 to uc_payment_order() expected to be a reference, value given in /var/www/drupal/includes/module.inc on line 462.
warning: Parameter 2 to uc_quote_order() expected to be a reference, value given in /var/www/drupal/includes/module.inc on line 462.
warning: Parameter 2 to uc_googleanalytics_order() expected to be a reference, value given in /var/www/drupal/includes/module.inc on line 462.

I'm not sure if the included patch breaks anything, but it gets rid of the error message for me. Thoughts?

Log in or register to post comments

Comment #30

torgospizza

he/him

English

Portland, OR

commented 27 September 2010 at 19:48

It shouldn't break anything, really.

Log in or register to post comments

Comment #31

Island Usurper commented 21 October 2010 at 17:12

Status:

Active

» Needs review

It'd be nice to figure out where that error was showing up, but I guess changing this fixes the problem too. We ought to fix hooks.php to match the function signatures of all of the hooks changed this way.

Log in or register to post comments

Comment #32

MattBrigade commented 22 October 2010 at 15:38

So far I've just been *ignoring* the warnings and Ubercart seems to be working okay so far. Since the patch removes the "&" from the offending functions' parameters, for those using PHP 5.2 the data wouldn't be passed as reference (is that correct?). I'm afraid that would end up doing more harm than good.

Log in or register to post comments

Comment #33

Island Usurper commented 22 October 2010 at 16:42

mattapus, passing objects by reference or by value has far less effect in PHP 5 than it does for other data types. If you change a property of the object within a function, it will be changed even after the function returns. The difference is what happens when you assign a new object to that variable. Pass-by-reference means that change continues outside of the function scope, pass-by-value means it doesn't. In general, Ubercart's and Drupal's hooks shouldn't be creating new objects for their arguments, so it doesn't really matter if we use & or not.

Log in or register to post comments

Comment #34

MattBrigade commented 22 October 2010 at 22:30

Island, you are right! Please dismiss my noobness.

Log in or register to post comments

Comment #35

roam2345 commented 13 March 2011 at 13:25

Is there an official fix for this or has nothing been moved to a release yet?

Log in or register to post comments

Comment #36

James-5642 commented 15 March 2011 at 23:56

I have just installed the latest ubercart 6.x-2.4 on a debian squeeze server and now I see the following errors on the /cart/checkout/review page:
* warning: Parameter 2 to uc_payment_order() expected to be a reference, value given in /.../includes/module.inc on line 462.
* warning: Parameter 2 to uc_quote_order() expected to be a reference, value given in /.../includes/module.inc on line 462.

It seems that all customers can see this warning also => noone will buy products if they see such errors in a online shop.

So what can I do. Is there a working version of ubercart?

Log in or register to post comments

Comment #37

roam2345 commented 16 March 2011 at 00:20

We have the same issue.. Only way is to remove the & in the functions with the errors suxs that we have to hack the module still waiting for an update here strange to see no resolution on this ESP as drupal 7 has these newer requirements that a lot of people will now have if they are pushing newer sites out and this issue only gets bigger as time goes on.

Log in or register to post comments

Comment #38

tommytank commented 20 March 2011 at 12:02

The above patch seems to work. However, it doesn't include changes to the uc_credit_module and the uc_coupon_module. Perhaps someone who knows how to do this could include this in an updated patch file.

Log in or register to post comments

Comment #39

univate commented 30 March 2011 at 03:11

Status	File	Size
new	ubercart-574066-39.patch	2.94 KB

As noted in #22 as long as we know we are passing objects there is no need to add the &. Since ubercart is already specifying PHP 5 in its info files this change should not be a issue.

This patch takes #29 and also add fix for uc_credit_order and updates the hook.php docs.

So this patch should fix all the hook_order() functions.

Log in or register to post comments

Comment #40

longwave

he/him

English

UK

commented 30 March 2011 at 15:31

Status:

Needs review

» Fixed

Committed.

Log in or register to post comments

Comment #41

tr commented 30 March 2011 at 21:10

Status:

Fixed

» Patch (to be ported)

Shouldn't this be changed in 7.x-3.x too?

Log in or register to post comments

Comment #42

longwave

he/him

English

UK

commented 30 March 2011 at 21:18

Version:

6.x-2.x-dev

» 7.x-3.x-dev

Probably - I haven't tested 7.x with PHP 5.3 at all yet.

Log in or register to post comments

Comment #43

Island Usurper commented 6 May 2011 at 15:46

Status:

Patch (to be ported)

» Fixed

Ported and committed the patch by monotaga and univate. I've been using PHP 5.3 and I haven't seen this problem, and I had no noticeable difference with the patch applied or not. But we should probably stay consistent with the 2.x branch.