The uc_product_forms function calls

<?php
array_keys(uc_product_node_info())
?>

instead of

<?php
module_invoke_all('product_types')
?>

By not invoking hook_product_types, custom product modules cannot utilize the uc_product_add_to_cart_form or uc_catalog_buy_it_now_form without creating their own custom forms.

I've submitted a patch that fixes this.

CommentFileSizeAuthor
#26 hook_forms.patch3.37 KBIsland Usurper
#24 uc_product.diff996 bytesgreenskin
#18 hook_forms.diff2.28 KBgreenskin
#17 hook_forms.diff1.7 KBgreenskin
#15 hook_forms.patch2.92 KBIsland Usurper
#8 uc_product.diff1.02 KBgreenskin
uc_product.diff636 bytesgreenskin

Comments

rszrama’s picture

rszrama commented
Status: Needs review » Needs work

Good catch. We'll put a modified version of this fix in as soon as Lyle commits a fix for #334390: Add an API function to return the product types array.

Island Usurper’s picture

Island Usurper commented
Status: Needs work » Closed (works as designed)

By not invoking hook_product_types(), custom product modules can use different add to cart form functions. Product kits do this with hook_forms() by checking that the argument is a product kit node. ($arg[0] for D6.)

If this didn't happen, every custom product module would have to use uc_product_add_to_cart_form(), because each modules' hook_forms() would overwrite each other, and I don't want to mess with module weights to straighten it out.

rszrama’s picture

rszrama commented

Can you tell greenSkin what he needs to do instead?

Island Usurper’s picture

Island Usurper commented

Basically, copy uc_product_forms() into your own module, but check for your specific node type instead of the uc_product_node_info() array. You want to use the same form functions, so those would stay the same. uc_product_kit_forms() uses the product kit table because it is a smaller table than {node} as it only contains product kit nodes in it. If you have a similar table for your module, you could use that.

greenskin’s picture

greenskin commented
Status: Closed (works as designed) » Needs work

@Island Usurper
It is important that custom product modules can still use different add to cart form functions but does not prevent uc_product_forms from using the hook_product_types info instead of uc_product_node_info. The uc_product_forms specifies form_id's that equal 'uc_product_add_to_cart_form_'. $product->nid will point to 'uc_product_add_to_cart_form'. All a custom module that wanted to use a different add to cart form would use something like 'uc_custom_add_to_cart_form_'. $product->nid and implement their own hook_forms function that would point it to their own add to cart form. So basically the if statement currently in uc_product_forms is not even needed.

By implementing the hook_product_types (even the new API for it) or just remove the IF statement entirely allows custom modules (like uc_donation) to utilize uc_product's add to cart form without unnecessary duplication of code.

greenskin’s picture

greenskin commented

Has this been fixed?

snowball43’s picture

snowball43 commented
Title: Product add to cart forms not invoking hook_product_types » Simplify and remove extra code from uc_product_forms
Status: Needs work » Active

The uc_product_forms does not need to do a database query or perform an IF statement to check the form_id being called, as the use of hook_forms should simply return an array of form_ids that point to an different form_id. Drupal makes lots of database calls so if it can be avoided, don't make unnecessary calls to the database.

<?php
function uc_product_forms($form_id, $args) {
  $forms = array();
  if ($args[0]->nid) {
    $forms['uc_product_add_to_cart_form_'. $args[0]->nid] = array('callback' => 'uc_product_add_to_cart_form');
    $forms['uc_catalog_buy_it_now_form_'. $args[0]->nid] = array('callback' => 'uc_catalog_buy_it_now_form');
  }
  return $forms;
}
?>

These form_id's are only going to be used if the $form_id actually equals "'uc_product_add_to_cart_form_'. $args[0]->nid" or the the one for the catalog. Having the IF for $args[0]->nid just ensures that a node id is indeed being passed. This should be fixed before the next release is made for Ubercart as I can understand for the development of uc_donation can benefit from it as it sounds that it's waiting for this change to continue in the development to prevent unnecessary duplication of code which is the Drupal way.

greenskin’s picture

greenskin commented
Status: Active » Needs review
StatusFileSize
new uc_product.diff1.02 KB

Here's a patch utilizing the change mentioned by snowball43 in #7. By implementing this patch, uc_donation's Drupal 6 release will allow donations to correctly get added to the cart.

rszrama’s picture

rszrama commented

Does this affect any other forms on the site? For example, what if some random form passes in a node as the first element in that argument array? Node edit form perhaps? Just curious. Is there any way to make this more UC specific?

fang27’s picture

fang27 commented

As far as I can tell, it does fix the issue, but now donations cannot be adjusted in the cart, or removed from the cart.

greenskin’s picture

greenskin commented

This only affects forms with an ID of 'uc_product_add_to_cart_form_xx' or 'uc_catalog_buy_it_now_form_xx', with xx being a node id.

If you really wanted to ensure that the nid used is one that is a product then query the uc_products table, but I don't recommend this as it just makes an additional query to the database. I feel it is better to have unused form_id's being pointed than to make a call to the database to get an exact listing, because who is really going to call the form 'uc_product_add_to_cart_form_102' if nid 102 is not an actual product.

Basically all this patch is doing is taking any form that's form_id begins with 'uc_product_add_to_cart_form_' or 'uc_catalog_buy_it_now_form_' and points it to the appropriate form_id like 'uc_product_add_to_cart_form'. The hook_forms function just generates an array of form_id's that point back to the parent id, so there is no need to run checks to check that the form_id that is passed to the hook is a product add to cart or catalog buy it now as the first part of the form_id already makes it semi unique.

grendzy’s picture

grendzy commented

subscribing

greenskin’s picture

greenskin commented

The patch in #8 is still good for beta3 release.

rszrama’s picture

rszrama commented
Assigned: Unassigned » Island Usurper
Island Usurper’s picture

Island Usurper commented
StatusFileSize
new hook_forms.patch2.92 KB

I agree that removing the queries from the hook_forms() is a good idea, but I still want each module to map its own forms. It's only because uc_product_kit comes after uc_product that your patch lets product kits be added to the cart from the catalog pages. If it came before, it would be broken. That's not something I want to rely on.

That comes up because the catalog pages are looking for the same form regardless of the product's type, so there may be a solution there that would be better. I just figure that using hook_forms() is good enough.

greenskin’s picture

greenskin commented
Status: Needs review » Needs work

@Island Usurper
Your patch looks good to me except that I don't understand why you want each module to map it's own forms? The view for product kits calls theme('uc_product_kit_add_to_cart') so I don't see where a product kit would utilize the form 'uc_product_add_to_cart_form' so unless I'm missing something this isn't an issue. Did you test your theory of weight affecting product kits and if so what was the config for your product kit?

If I in fact did miss something where product kits do call the 'uc_product_add_to_cart_form' then my stand would be that since product kits are a part of core and a unique case that uc_product should accommodate it's needs.

greenskin’s picture

greenskin commented
Title: Simplify and remove extra code from uc_product_forms » Simplify the use of hook_forms
Status: Needs work » Needs review
StatusFileSize
new hook_forms.diff1.7 KB

Here's my revision of patch from #15.

EDIT: Sorry, disregard this patch. Use the following patch.

greenskin’s picture

greenskin commented
StatusFileSize
new hook_forms.diff2.28 KB

The previous patch I mistakening made against my already patched version of uc_product.module. This patch was made against the beta3 release.

flickerfly’s picture

flickerfly commented

Thanks guys, looking forward to using the donations product module.

Island Usurper’s picture

Island Usurper commented

On catalog pages with the table view (I'm not sure about the grid view), products and product kits both use uc_catalog_buy_it_now_form(), which gets mapped differently.

greenskin’s picture

greenskin commented

The catalog should be re-worked (probably need to start a different issue) so it doesn't hard code the buy it now form or do something like the following on line 725 of uc_catalog.module:

<?php
$product_table .= drupal_get_form($product->type .'_buy_it_now_form_'. $product->nid, $product);
?>

Probably a good thing also to add an IF statement around that to see if the function exists. Limiting the catalog to just uc_product and uc_product_kits seems kinda self-defeating for Ubercart and the power of Drupal. Contrib modules that create there own product types (again from my experience with uc_donation) have to find tacky work-arounds to allow the same functionality with their products in the catalog as uc_product and uc_product_kits.

Island Usurper’s picture

Island Usurper commented
Priority: Normal » Minor
Status: Needs review » Postponed

I went ahead and committed the patch in #15 since it is a bug to have those queries in hook_forms().

The fact that modules have to use of hook_forms() is also a problem, but not as bad since there is a workaround. We're almost done with Ubercart 2, and version 3 should bring a lot of changes to the inner workings of Ubercart, so this might not even be an issue when that happens. But if it is, that would be the time to fix it. The idea of what a product is will be better defined, and we'll be freer to develop the API more fully.

greenskin’s picture

greenskin commented
Title: Simplify the use of hook_forms » Allow contrib modules to utilize uc_product_add_to_cart_form
Category: bug » task
Priority: Minor » Normal
Status: Postponed » Needs work

I'm all for better developing how products are defined and a better API and I'm glad this has been realized as a problem, but this is such a simple fix that I don't understand what the hold off is. The uc_product_kit is a unique case and since it is part of core the function uc_product_forms should be able to check if the product type is 'product_kit' and use function uc_product_type_names to check to make sure the node is indeed a valid product instead of the restricting checking of only uc_product_node_info()'. Simple, very little code, easy, unobtrusive, clean and solves any issue aforementioned against the idea of the original patch.

By requiring contrib modules to use hook_forms() makes uc_donation in-particular add 17 lines of code. Now this isn't an extreme amount of code when just looking at it but when compared to such a simple fix to uc_product it's an excessive amount of additional code.

This is my last request that this gets done. I have been very pleased in the past how Ubercart's community/devs have worked together but this issue has felt like pulling teeth. Part of being in the Drupal community is debating functionality of projects. I will be content in waiting for Ubercart 3.0 and adding the 17 lines of code to uc_donation if a valid and legitimate reason with no simple fix is made to why this can't be done currently.

I don't mean to sound obtrusive but I'm not a fan of duplicating unnecessary code.

greenskin’s picture

greenskin commented
StatusFileSize
new uc_product.diff996 bytes

Here is the simple fix. This patch made after patch in #15.

Also, note that you don't need to substr check the $form_id.

Island Usurper’s picture

Island Usurper commented
Priority: Normal » Minor
Status: Needs work » Postponed

I can't make a special case for uc_product_kit when I'm doing the exact opposite for uc_attribute. (http://drupal.org/node/349135) Having to make allowances for core modules only serves to weaken the code and prevents modularity.

Checking that the node type isn't "product_kit" in uc_product_forms() prevents other product modules from doing something different in their add-to-cart form. Even worse, they wouldn't be able to use hook_forms() or anything else to work around it.

I'll take out the substr() calls, but until more fundamental changes are ready to happen to the product system, I think it's the way it has to be to work in every situation.

Island Usurper’s picture

Island Usurper commented
StatusFileSize
new hook_forms.patch3.37 KB

Patch committed to remove substr() calls.

greenskin’s picture

greenskin commented

Ok, I understand now. Again, I hope I didn't sound trite but was just standing up for the uc_donation module. Thanks for your consideration and time.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Postponed » Closed (won't fix)

If this is going to be fixed it should be handled in a new issue, as this one is so old now.