Product Feature API

Wondering if I should focus on getting these changes into 7.x first then have them backported?

Firstly I have attached new patches as I noticed a couple of minor typos.

Just to explain the changes being made here further...

Products in ubercart don't actually need an API as they are nodes and therefore you can just use node_load() and hook_nodeapi() function or the D7 equivalents to work with those object.

The patches here are actually for the Product feature data which is part of the uc_product module.

My definition of the API's that ubercart require are that there should be dedicated functions that deal with the database/SQL interface and anytime a features wants to interact with the database it ideally should go through one of these functions. I understand that there are cases where this is not always possible (e.g. complex one off SELECT's that you want to be able to optimize rather then having lots of database queries) but in general there should be functions available that provide the CRUD (create, read, update, delete) operations for each data object that ubercart has in its database.

So in this patch instead of using SQL directly we replace with a call to a function that executes the same SQL (ie: the SQL query now becomes re-usable by other function include third party modules/developers via a simple function call):

+++ uc_product/uc_product.admin.inc
@@ -404,18 +404,11 @@ function uc_product_features($node) {
-      $result = db_query("SELECT * FROM {uc_product_features} WHERE pfid = :pfid AND fid = :fid", array(
-        ':pfid' => arg(5),
-        ':fid' => arg(4),
-      ));
-      if ($feature = $result->fetchAssoc()) {
+      $feature = uc_product_feature_load(arg(5), arg(4));
+      if (isset($feature)) {

Instead now that the code is in its own function and you can see the code hasn't really changed, its still performing the same SQL query and returning the result.

+++ uc_product/uc_product.module
@@ -1999,6 +1999,66 @@ function uc_product_feature_save($data) {
+function uc_product_feature_load($pfid, $fid = NULL) {
+  $sql = "SELECT * FROM {uc_product_features} WHERE pfid = :pfid";
+  $arg = array(':pfid' => $nid);
+  if (isset($fid)) {
+    $sql .= ' AND fid = :fid';
+    $arg[':fid'] = $fid;
+  }
+  $feature = db_query($sql, $arg)->fetchAssoc();
+  return $feature;
+}

There is another problem with that SELECT query, any database person will tell you that you should never do a "SELECT *" - depending on the database it can be inefficient as it can require extra query(s) to retrieve the schema information before it can perform the actual query. When the list of each of the fields could easily be included in the original query there is no reason to use a "SELECT *".

The functions I am adding with this patch provide the R(ead) and D(elete) functions. There is already a uc_product_feature_save() function, which provides the last two required CRUD operations - C(reate) and U(pdate).

I think some work could be done to make that *_save function better, but an API is better then no API and I am looking for the easiest way to get an API in place and I think if we just refactor the code so that there are CRUD functions for each data object in ubercart we can then look to focus on making the API better.

The changes I'm proposing here in this patch I think are minor as they do not really change any code they just move it around into specific function for each of the operations to make up an API.

Powered by Dreditor.

if we write down all the functions on one sheet of paper

Any suggestions where I could start writing such a document? somewhere on ubercart.org? or should I just use something like google docs for this?

Totally agree with you on the arg()'s, it was painful looking at them - what you have done looks good to me, although I haven't done any formal testing.

Also the only reason I left in the $fid argument was it was previously being used in the original query, I was presuming it was only there as a sanity check myself.

Re-rolled this patch.

So this is been committed to D7, can it also be considered for D6.

The D6 patch in #3 above still applies, although does not include all the additional changes made in subsequent patches - I'm going to review to see if they are needed in D6 as well.

The changes between #3 and #10 are related to the following refactoring work and not specific to the actual issue of adding functions to load and delete product features:

I've also cleaned up the menu callbacks concerning product features, since I couldn't stand looking at all of those arg() calls. This just involves adding menu items and splitting up some callback functions as needed. I expect it to just behave the same as before, but we'll need to make sure of that during testing.

That said I did attempt to try and port the code, but there are things being done in "new D7 ways" which means it can't be achieved with a straight port and required some refactoring itself. Doing any major refactoring like this in D6 is probably best avoided unless it provides some benefit - in this case it doesn't and I would like to keep the patch reasonably simple to read/review.

It should be easy to see from just reading the patch in #3 that no functionality is being changed with this issue. The only difference is a couple of blocks of code are moved to their own function, making them re-usable and providing a full CRUD api for product features.

So I would like to propose we review the current patch as is and get it committed, if there is agreement that the refactoring of the menu handlers for the products features should be done like in D7 then we can follow that up with another issue/patch.

Anyone had a go at testing this patch for D6?

Would be good to get this in before the next release. Its been in the issue queue now for almost a year. It doesn't change any functionality. It cleans up some code making it more readable and exposes a couple of functions that make it possible for developers to work with product features outside the UI.

Its a return code to indicate stuff was deleted:
http://api.drupal.org/api/drupal/includes--common.inc/constant/SAVED_DEL...

Example:
http://api.drupal.org/api/drupal/modules--taxonomy--taxonomy.module/func...