hook_uc_order() needs a 'load' op-code [#2185193]

Hello,

One issue I had with Ubercart in a project I've been working on. I needed to do some customizations to the standard uc_order object, e.g. adding fields etc. For that hook_uc_order seemed the obvious choice. I.e there I can attach new fields on 'new' and etc.

However, there is one big drawback and that is the hook_uc_order doesn't provide a 'load' op-code. So, the obvious (although sub-optimal) approach is to hack uc_order.module. So let me propose the following change to uc_order_load:

function uc_order_load($order_id, $reset = FALSE) {
  if (is_null($order_id) || $order_id < 1) {
    return FALSE;
  }
  
  $orders = uc_order_load_multiple(array($order_id), array(), $reset);

  $order = $orders ? reset($orders) : FALSE; 
  if ($order) {
	  uc_order_module_invoke('load',$order,NULL);	
  }
  return $order;
}

I think that this change would provide a more complete definition of hook_uc_order.

skari

Comments

Comment #1

longwave

he/him

English

CreditAttribution: longwave commented 30 January 2014 at 10:27

Status:	Active	» Postponed (maintainer needs more info)
Issue tags:	-uc_order

This op already exists, see the documentation: http://api.ubercart.me/api/drupal/ubercart!uc_order!uc_order.api.php/fun...

This is called from UcOrderController::attachLoad() in uc_order.controller.inc:

/**
 * Controller class for uc_order entity.
 */
class UcOrderController extends DrupalDefaultEntityController {

  function attachLoad(&$orders, $revision_id = FALSE) {
...
      uc_order_module_invoke('load', $order, NULL);

Is this not suitable for your use case?

Comment #2

skari CreditAttribution: skari commented 30 January 2014 at 12:42

Hello,

Yes, sorry this op already exist. It's been a while since I added this to my installation, and now updating Ubercart and applying my custom patches for the n-th time I felt it was time to make an official feature request, but forgot to mention the details behind my change.

Ok, so let me elaborate:

Burying 'load' inside the ordercotroller is not optimal. One issue is that entity_load caches already loaded entities (unless reset is set). So calling uc_order_load is not guarantee to trigger the 'load' opcode, and that is counter intuitive. Of course, the user/coder can always call with $reset=true, but that entails awareness of an (irrelevant) implementation detail. All the other hook op codes (i.e. 'delete', 'save' etc.) are invoked directly inside the appropriate uc_order calls. So why not 'load'?

Another issue is that (in my case) the 'load' is called "too late", i.e. after the addition of the order lines.

One might be worried that adding/changing this now would break something in other 3rd party modules. But, in what cases might that be? Someone calling an entity load on an order, making some changes in the code, and then calling uc_order_load. If he/she doesn't call with reset=true, then nothing would happen, so why would he/she be calling uc_order_load? If calling with reset=true then the 'load' op code will be invoked. In other words, it is difficult to see that this change would break anything.

Comment #3

skari CreditAttribution: skari commented 30 January 2014 at 12:48

hmmm....

reflecting a little bit more on the situation. Of course, it seems that someone had a reason to call 'load' before adding the line_items. So we have a catch-22 position. Perhaps, after_load? or pre_load needs to be added?

Comment #4

longwave

he/him

English

CreditAttribution: longwave commented 30 January 2014 at 13:41

uc_order_load() is just a convenience wrapper around entity_load(), so by adding or moving the invoke call to uc_order_load() there is no guarantee it will always be called - the order controller class is the right place to do it.

If you need the line items in your 'load' op, can't you just load them yourself?

Comment #5

skari CreditAttribution: skari commented 30 January 2014 at 14:01

My point is not to guarantee that entity_load will always be called. Rather, I think that hook_uc_order hooks should always be called, with opcode 'load', when uc_order_load is called. Just as _uc_order hooks are guaranteed to be called with op code 'delete' when calling uc_order_delete (provided there is an order of course).

>>If you need the line items in your 'load' op, can't you just load them yourself?

Of course, I can always find a way to do that. But as this stand, I can't use the 'load' op code for that, as would be the most natural approach.

Comment #6

longwave

he/him

English

CreditAttribution: longwave commented 30 January 2014 at 14:16

I actually think it should be the other way round - the 'save', 'delete' ops should be moved to the controller class if possible, so generic entity functions (entity_load, entity_save, entity_delete, etc) can be used instead of requiring the uc_order wrappers.

This change has already been made in 8.x-4.x, but I am wary of doing this at this stage in the 7.x-3.x cycle without extensive testing to ensure it doesn't break contrib or custom code.

Comment #7

skari CreditAttribution: skari commented 30 January 2014 at 16:06

Yes, I see your point. On the other hand, my point is that you need to provide a consistent api. Moving everything into the controller makes sense, and I think the user/coder shouldn't then be surprised that he needs to reset the cache in order for his hook 'load' handler to run. But as this stands in the 7.x- packages I don't think you have a consistent interface. I actually feel that drupal 7 and a lot of the contrib modules suffer from being in a transition state from "old style drupal" to OOP drupal.

Well, I guess I keep patching my Ubercart installs then. There are a lot of things that need patching anyway, esp. now since it appears that I am out of ammo battling the consistent complaint of my users about that Ubercart doesn't support fractional quantities. But that is another story... (hmm... is there any such support in 8.x.? Haven't checked that out yet)

cheers,
skari

Comment #8

longwave

he/him

English

CreditAttribution: longwave commented 30 January 2014 at 17:18

Can you use hook_entity_load() (invoked for all entities) or hook_uc_order_load() (invoked for orders only)? This is called by the parent attachLoad() after the order controller has done all its work.

Comment #9

skari CreditAttribution: skari commented 30 January 2014 at 19:51

Ok,

thanks that appears to be an option. I guess I really need to try it out, as I am calling uc_order_load quite a bit in my code now. But I guess I can change the status of this issue to 'closed'. I have some further comments on other aspects of the uc code, that I will be posting as separate issues/feature requests.

thanks for taking the time to look into this with me.

skari