uc_cart_login_update: missing cart rebuild, if cart of logged user was empty

Shouldn't this patch be checking db_affected_rows() instead of the result of db_query()?

I have no commit access to this project; I suspect the maintainers will want a fully working patch they can apply immediately.

Kicking the bot to get it to test the patch.

I am still slightly confused as to what this actually fixes. Can you demonstrate the issue in core alone, or does it require custom code?

But $items isn't returned so what difference does this make? The point of that code is to merge identical items, but if the logged in user has an empty cart, there is nothing to merge.

I am still confused by the original uc_cart_login_update() and why it does things the way it does. The attached patch/code below makes it much simpler and it still seems to work, please review.

function uc_cart_login_update($uid) {
  if (!isset($_SESSION['uc_cart_id'])) {
    return;
  }

  // If there are items in the anonymous cart, consolidate them.
  if ($items = uc_cart_get_contents($_SESSION['uc_cart_id'])) {
    // Remove the anonymous cart items.
    db_query("DELETE FROM {uc_cart_products} WHERE cart_id = '%s'", $_SESSION['uc_cart_id']);

    // Merge the anonymous items into the user cart.
    foreach ($items as $key => $item) {
      uc_cart_add_item($item->nid, $item->qty, $item->data, $uid, FALSE, FALSE, FALSE);
    }

    // Unset the anonymous cart ID, it's no longer needed
    unset($_SESSION['uc_cart_id']);
  }
}

uc_cart_add_item() calls uc_cart_get_contents($cart_id, 'rebuild') so the suggested improvement in #13 should handle this. Can you try that patch and see if it helps for your case? As I still don't know how to demonstrate this as an actual bug, I need someone else to test things for me.

In the original code doesn't hook_update_cart_item get called as well? Seems that uc_cart_add_item() is called for every item in both the anonymous and authenticated carts, which should trigger this hook when consolidating identical items?

But won't hook_update_cart_item() still be triggered in both cases only if both carts have an identical item (with the same nid and data), which is the point of consolidating the items?

An example, let's assume attributes etc are disabled so $data is empty. Anonymous cart has nids 1 and 2. Authenticated cart has nids 2 and 3.

The original code:
- merges the carts so the authenticated cart contains items with nids 1, 2, 2 and 3.
- deletes all items
- calls uc_cart_add_item for each item, adding nid 1, nid 2, nid 2 again (triggering hook_update_cart_item) and nid 3

The simplified code:
- calls uc_cart_add_item for each item in the anonymous cart, adding nid 1 (no trigger, as it's not in the authenticated cart) and nid 2 (triggering hook_update_cart_item, as it's already in the authenticated cart)

uc_cart_get_contents() doesn't perform consolidation:

    $result = db_query("SELECT * FROM {uc_cart_products} WHERE cart_id = '%s' ORDER BY cart_item_id ASC", $cid);

    while ($item = db_fetch_object($result)) {
      if ($item = uc_cart_get_item($item)) {
        $items[$cid][] = $item;
      }
    }

There was code in there that *tried* to do this before, but it never worked because it compared the existing unserialized data items to the serialized version.

I think calling hook_update_cart_item() is actually the right thing to do here, it is not a proper hook (only invoked on the module that owns the product), and perhaps a product type module would want to know when an anonymous cart and authenticated cart with the same items were being merged.

uc_product_update_cart_item() will be the function called in 99% of cases and this is necessary to update the quantity field in the db anyway!

Committed #13 to both branches.