hook_nodeapi() in Ubercart modules

Project:Ubercart Contributions
Component:Code
Category:
Priority:critical
Assigned:Unassigned
Status:active
Description
Project: 
Ubercart

This bug occurs in UC Beta 1 and all earlier versions.

hook_nodeapi() is implemented in the modules uc_cart, uc_quote, uc_flatrate, uc_ups, uc_usps, and uc_weightquote, and uc_repeater, as well as many other modules throughout Ubercart.

However, for the named modules, the type of the node is not checked at all inside the hook implementation. At best, this impacts the speed of the site because all these hook_nodeapi() implementations are run in their entirety even for non-Product nodes. At worst, these modules could corrupt the database when they manipulate non-Product nodes.

Many of these implementations have multiple database queries performed whether or not the node in question is a Product!

For example see the following code in uc_quote, in the function uc_quote_nodeapi():

<?php
   
case 'insert':
    case
'update':
     
db_query("DELETE FROM {uc_quote_product_locations} WHERE nid = %d", $node->nid);
     
db_query("INSERT INTO {uc_quote_product_locations} (nid, first_name, last_name, company, street1, street2, city, zone, postal_code, country) VALUES (%d, '%s', '%s', '%s', '%s', '%s', '%s', %d, '%s', %d)",
       
$node->nid, $node->first_name, $node->last_name, $node->company, $node->street1, $node->street2, $node->city, $node->zone, $node->postal_code, $node->country
     
);
?>

In this case, there are two DB queries run every time a node of any type is inserted or updated, with all sorts of extraneous information added to the DB keyed to non-Product nodes! (This can be quickly confirmed by examining your own DB - see all the empty rows in uc_quote_product_locations corresponding to nids for your non-Product nodes like forum posts, book pages, faq, etc.).

I suggest testing $node->type to see if it equals 'product' as the first statement in each of these hook_nodeapi() implementations.

Ryan's picture
Offline
Joined: 08/07/2007
Juice: 15438
Re: hook_nodeapi() in Ubercart modules

Aye, good idea. I'm going to recommend that we check the node type this way and have Lyle confirm it's correct before I apply it to the cart module:

<?php
 
if (in_array($node->type, array_keys(uc_product_node_info()))){
   
// Run dem queries...
 
}
?>
Lyle's picture
Offline
AdministratoreLiTe!
Joined: 08/07/2007
Juice: 6846
Re: Re: hook_nodeapi() in Ubercart modules
Assigned to:Ryan» Lyle

Yup, that's what I was going to do. Also need to check the module dependencies now to make sure they're all dependent on uc_product.

Update: Committed.