Fatal error: Unsupported operand types in .../uc_order.module on line 1459

Ahh, great! Someone else posted this issue but I had no clue where the conflict was. : P

Poor namespace of the hook is the true issue, but for 2.x I would settle for a simple && is_numeric($value).

#610276: Lisa Has the BUG also----Fatal error: on line 1459 was a duplicate.

So, should this go back to an RC, or how will we learn to play nicely with one of my other favorite modules?

Subscribing.

subscribe

@Cayenne: we'll have a simple patch for it and roll the fix into 2.1. I'll make note of the conflict on the release announcement.

Hi,

here is a short quickfix adding just an if statement to get this new release running until it comes to a decision

I also added the patch, just apply this patch on the ubercart module folder (not directly on the uc_cart.module)

<?php
foreach (module_list() as $module) {
    $function = $module .'_order';
    // $order must be passed by reference.
    if (function_exists($function) && ($value = $function('total', $order, NULL))) {
      if (is_numeric($value)) {
        $total += $value;
      }
    }
}
?>

Patch in #10 works great.
Thanks!!

I second #3 Damien Tournoud. The name of the hook should be more specific than "order".

subscribing #3 and #12
maybe add ubercart in the hook like "hook_ubercart_order()" or "hook_uc_order_get_total()" or something similar

The idea moving forward is to "namespace" all the hooks with a uc_ prefix. That would've happened already if the 2.0 didn't take 16 months to produce. : P

Thanks for the patch! Hopefully Lyle can get us some review juice on it... I don't have time atm.

subscribing

patch in #10 works great for me as well... Thank You!

This is actually just a long-running struggle. Ubercart MUST fix its hooks to namespace them. This is actually a duplicate of long-open issue #510382: Ubercart must namespace its hooks. Please, please follow up this quick fix with the real fix: Taking the time to namespace the hooks and play right within drupal.

#10 doesn't deal with the fact that we are calling any function in the drupal install with the name module_order. That function might do something worse than returning the wrong datatype. It might call drupal_goto(). We have to avoid calling functions we don't know anything about.

subscribe

Chiming in to keep an eye on this issue. Patch in #10 at least allowed us to stay at 2.0.

@rfay: As I posted above, I definitely agree that hooks must be adjusted, but this issue should be restricted to resolving the point of conflict for now. Consider where the 2.0 development lifecycle is and has been. We've essentially been in beta mode and then RC mode for months trying to push out a release so we could open up dev and fix the hook issues. Our API adjustments during this time were as non-obstructive as possible (even if they did still break things). Changing every hook in Ubercart and then expecting every contrib and site-specific module to convert would be disastrous at this point. As a point of comparison, I doubt we'd ever see Drupal changing all of its hooks after it moves into beta mode. This is definitely on the "hey, don't do this again" list for Ubercore dev. I'll make sure it's reflected in the docs as we get to tidying them up.

@rszrama: One other way to short-term fix this is to put a list of non-allowed function names in there. That's actually what I did - but I just put date_order in. Maybe it should be keyed on a variable in the variables table, so it can be fixed if a given module shows up with hook_order.

Then we check to see if the function name is in the list of disallowed functions and skip it if it is. This is extensible in the case that another hook conflict shows up.

Interesting... so, you're keeping something like a registry of modules that inadvertently implement Ubercart hooks and bypassing them somehow in hook invocations like this one?

Also, I updated the pertinent doc page on d7uc, though it can be fleshed out a little bit. : D

How I install a patch file to the Ubercart Module folder?
Sorry, I'm new to Drupal and Ubercart, but loving them both.

Here is a patch which is more generic, and prevents calling the modules which have unintentional hooks. This is what I meant in #21.

The basic approach: Whenever ubercart is looking for modules with hooks, instead of calling module_list() it should call uc_modules_hooks_allowed() (introduced here) instead of module_list(). uc_modules_hooks_allowed() subtracts the modules in the Drupal variable uc_ignore_modules from module_list().

uc_modules_hooks_allowed() modules should probably be in another location.

I didn't attempt to fix any other instances of the hook issue.

The hook_update_N deals with today's emergency only, but the approach is extensible and configurable.

This remains an ugly hack, no matter what.

If you install this patch, you need to either run update.php or manually set the value of the variable uc_ignore_modules to 'date'. Otherwise the patch with do NOTHING. That assumes of course, that others are having this problem for the same reason I am, by calling date_order(). If you're having trouble with other modules, add them into the variable.

subscribe

Subscribe

@jmartin_prwa: The patch gets installed to uc_order.module. Go into the ubercart/uc_order/ folder and edit uc_order.module file.

Take the patch from #10:

Index: uc_order/uc_order.module
===================================================================
--- uc_order/uc_order.module	(revision 33804)
+++ uc_order/uc_order.module	(working copy)
@@ -1456,7 +1456,9 @@
     $function = $module .'_order';
     // $order must be passed by reference.
     if (function_exists($function) && ($value = $function('total', $order, NULL))) {
-      $total += $value;
+      if (is_numeric($value)) {
+        $total += $value;
+      }
     }
   }
 

At or around line 1459 you should see "$total += $value;" match the code up to what you see in the patch to make sure you are in the right place.

Remove the lines with "-" and add lines with "+" as indicated in patch.

Patch worked for me...

Rachel

@jmartin_prwa, @rachelNY, general instructions on how to install and use patches are at http://drupal.org/patch.

Patch worked for me as well.

Thanks,
Steve

Haven't upgraded to Ubercart 2.0 just yet, but wanted to keep an eye on this page to apply the patch...

Subscribe
Patch at 10 solves for now...

plus 1 for the solution #10
for information, I had the problem because the date module has a function date_order() ... maybe this hook should be "namespaced" (something like: hook_uc_order()).

Both the patches at #10 and #26 work for the 'date' module case, however they're just workarounds and both have their downfalls. They may solve the problem with hook_order() for now, but what about other hooks ubercart provides? I wouldn't want to see either committed to ubercart core tbh. The real solution is to fix the hooks so they are less generic, e.g. 'hook_ubercart_order()'. While hooks aren't mentioned specifically, it is in the coding standards:

Functions should in addition have the grouping/module name as a prefix, to avoid name collisions between modules.

Changing the hooks would impact a lot of ubercart add-on modules I admit, so maybe we could extend rfay's patch to apply to other hooks, and remove the reference to 'date_order', keeping in mind that you can't have a blanket ban on all hooks from one module. For example, you want to exclude 'date_order' when invoking hook_order(), but still allow the date module to implement other ubercart hooks.

So while a quick fix might be the way to go now, I'd like to see a new release soon with the hook names fixed. I'd be happy to help test this and create patches for the add-on modules.

Cheers,
Stella

subscribe

Patch worked perfectly. Thank you all sooooo much!

subscribe

subscribe

patch in no 10 worked for me

subs

subscribe

Subscribing...

subscribe

Subscribe.

subscribe.

I applied the patch, which fixed problem, but then had a fatal error afterwards. I then tried to revert to earlier version of ubercart... not being experienced with 'downgrading' via update.php, I was unable to revert and had to remove ubercart completely.

Subscribe.

Subscribe.

Subscribe

New to patching. It asks to type the file to patch which #10 says is the ubercart folder. In terminal I get the message "is not a regular file -- can't patch. WHat am I doing wrong? :(

Not sure what os you are on, but from a linux terminal you would first look at the file that is to be patched (read the patch file at the top you see - --- uc_order/uc_order.module (revision 33804)). This means that within the ubercart/uc_order folder you are patching uc_order.module.

First backup the original file so:
cp uc_order.module uc_order.module.orig

Then patch the file
patch uc_order.module patchfile.patch

That's it.... If you search the drupal site there are guides for building and deploying patches...

Thanks. Got it to work! But I found a new problem with Ubercart Marketplace. Sellers are getting an error message when they check their sales overview. Its just the overview page, they can still go to selling/fulfill and selling/view. I opened an issue there http://drupal.org/node/615912 but I think it may have something to do with this.

subscribe

here is the real patch ... i will save everyone time and energy ... and do this the correct way ....

here is the ORIGINAL FILE ... the one that came with ubercart 2.0. i have also attached my updated version of the original file. Now in this file all i have done is what RachelNY did in post #29 ... which is referencing post #10. Now what i did exactly was .... delete the line with the - in front of it .... then added in the lines with the + .... oh yeah ... dont forget to take out the +'s at the beginning of each line ....

Or just simply .... download my version of this file below .... and remove the .txt extension..... then upload it tothe (sites/all/modules/ubercart/uc_order/uc_order.module) directory.

#10: Thanks for the patch. (I just installed it.)

#26: Cool patch, but I'm concerned that if your patch doesn't make it into UberCart core, installing it now will make upgrading UberCart later tricky, since you've created a new database version number. Won't the next version of UberCart (to have a DB change) use that same number? If so, Drupal won't know it needs to run the new UberCart DB change.

#55: thanks for the patched file which worked for me!

subscribe

Ryan and I talked about this, and we think the most elegant solution is the is_numeric() check in #10. It's a very small hack, and should work for most conflicts that may arise between now and the foreseeable future. Considering that this is the first major namespace conflict we've had in 3 years of development, I don't feel like we absolutely have to fix it in the 2.x branch. Needless to say, we will be a lot better about establishing a proper namespace in Ubercart 3.

Expect a 2.1 release sometime next week, since we're in the process of fixing some other bugs that have come up.

How about my patch?

Here are changes from version 2.0-rc7 to 2.0:

  $result = module_invoke_all('order', 'total', $order, NULL);
  foreach ($result as $key => $value) {
    $total += $value;

changed to:

  foreach (module_list() as $module) {
    $function = $module .'_order';
    // $order must be passed by reference.
    if (function_exists($function) && ($value = $function('total', $order, NULL))) {
      $total += $value;
    }

As you can see, in previous version we have array. In current version we still have array but we are trying to use it like simple number.

This is my changed version:

  foreach (module_list() as $module) {
    $function = $module .'_order';
    // $order must be passed by reference.
    if (function_exists($function) && ($values = $function('total', $order, NULL))) {
      foreach ($values as $key => $value) {
        $total += $value;
      }
    }
  }

subscribe

No, that won't work because the return value of hook_order() for the 'total' $op is supposed to be a number. Your patch is treating it like an array which will throw an error. The reason we could treat $result as an array in the orginal code is because that's the way module_invoke_all() works. The new code got rid of module_invoke_all() so it can pass $order by reference, so it no longer has a ready-built array of results.

subscribe and thanks I will use #10 then. thanks ryan and everyone

ONLY i dont know how to add the patch into the .module file from #10 help.......thank you/ Drupal surface girl.

Show the money if you can please do tell how to use the patch, i love the uber and so do the customers they want to uber the uber and pay with the ubercart let us know, love lisa

@lmfisher100: http://drupal.org/node/60108

subscribe, is this going to be committed soon? Or has it already been to the dev version?

Already committed, we should have a minor release soon with the fix.

path #10 for me worked well.

Patch #10 works for me as well but now instead of producing operand error i get

"Fatal error: Cannot pass parameter 3 by reference in sites\all\modules\ubercart\uc_order\uc_order.module on line 1458"

For anyone still looking, I've attached a patch to this post. Find how to apply patches at http://drupal.org/patch/apply.

Thank you!!! (=

#55 works perfect for me!
Thank you very much!

Reply #10 worked for me. Simply locate the file ubercart/uc_order/uc_order.module and change the line

$total += $value;
to

      if (is_numeric($value)) {
        $total += $value;
      }

To anyone else who finds this, please use the patch in comment #71. This is what has been committed to the code and will be in the next version. There's no sense patching your module with something else.

Subscribing.

Just a good-natured ping on this... It's been a month since this critical bug was discovered and diagnosed, and a patch has been available for a LONG time. Shouldn't we have had this in a timely point release by now?

An if statement inside another if statement? Completely unnecessary.

@rszrama, I know you said you already committed the other, but I'm pretty sure the attached patch is what Damien Tournoud meant in #3.

It'd probably be good to add some commenting in there so the is_numeric() doesn't accidentally get removed.

I would like to echo rfay's good natured ping.

This is a critical bug that causes a fatal error during checkout. Meaning this bug is directly causing lost sales.

I can verify that the patches in #71 #78 and #79 fix the bug.

Please release an officially patched version of this module.

Eh, I guess comments can't hurt, so I've committed the changes in #79.

A new version will be released soon. Things have just been a bit hectic lately.

And just so everyone gets the idea out of their heads that Lyle and I were just dragging our feet, protocol dictated we couldn't talk about what was holding up the 2.1 release until it made its way through the security review process. ; )

See http://drupal.org/node/636576 for more information. This release obviously includes this conflict resolution patch and some other fixes as specified in http://drupal.org/node/636616.

@rszrama, we would never think such things... Thanks for all your hard work!

^_^

I'm getting "Parse error: syntax error, unexpected '}' in .../uc_order.module on line 1458" in 6.x-2.1..

I was about to post a new issue.

Yes, Ubercart 6x-2.1 is broken. And I see 2.2 is out.

I'm new to the community, but already LOVE it! Does the last comment mean that 2.2 fixes the problems? And if so, should this thread be "solved" or something? Again, I'm new here so I don't really know if that is the normal procedure.

Thanks.

Yes, 2.2 fixes this problem, and the thread status is marked as "fixed" to indicate this.

ahhhh. Thank you @longwave. I've learned my thing for the day.

At this stage I can't guide Ubercart what to do. But your patch is is great fix.

Thank you,