Class uc_order_handler_field_money_amount extends uc_product_handler_field_price, but uc_order does not have uc_product as a dependency, which causes fatal errors when uc_order is enabled without enabling uc_product.

CommentFileSizeAuthor
#3 1587336-uc_order-dependency.patch385 byteswodenx

Comments

xano’s picture

Priority: Normal » Major

Setting to major, because this seriously breaks uc_order.

longwave’s picture

Priority: Major » Normal

Not major as it doesn't affect most sites; I imagine over 99% of Ubercart installs have both uc_product and uc_order enabled anyway.

wodenx’s picture

Status: Active » Needs review
StatusFileSize
new385 bytes

I actually just bumped into this yesterday setting up a fresh dev site. Patch attached.

longwave’s picture

I guess another question that should be asked is: should these modules be dependent? Should we fix this another way? Is there a use case for having uc_order installed without uc_product?

wodenx’s picture

Hm, well, I can see a use case for uc_product without uc_order (e.g. maintaining an online catalog for a brick-and-mortar store), but not really the reverse -- what would you order? I guess you could have some other custom module that took the place of uc_product, but order_products are pretty closely linked to product nodes, so it would be tricky to make that work.

xano’s picture

Whether there is a use case or not, the code allows people to break stuff, which is a bug that requires fixing. Either uc_order conceptually depends on uc_product, in which case the dependency should simply be declared in its info file, or the class extension is incorrect. Thing is that Ubercart 3 is final and changing the class extension might break functionality, so specifying the dependency is certainly the safest way.

My use case is a test case that requires uc_payment, which depends on uc_order.

Rule of thumb: never expect your users to use your products in a particular way. If you let them, they'll invent a thousand new ways to make your products useful ;-)

longwave’s picture

@Xano: I didn't say I wasn't going to fix it; I was just asking whether the dependency is the correct solution or just the easiest one.

longwave’s picture

Status: Needs review » Fixed

Committed #3.

xano’s picture

Thanks! Just out of curiosity, do you think this was the best or the easiest solution?

tr’s picture

I think it's an expedient solution, but not the right thing to do in the long run. Concepts like price and address, among others, are used by more than one sub-module, so they shouldn't be part of any one sub-module. In particular, price should be on its own, not bundled into product. However, that's a bigger change than we can make in a minor release.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

xano’s picture

When is the next release (with this fix) due?

xano’s picture

Bump

xano’s picture

It's been 4.5 months since the last release and it's been 3.5 months since this fix was committed. Could you please release 7.x-3.2?