Order views actions handler HUGE performance leak

I propose to remove the following logic from attachLoad():

  /* ... */
      $fields = array();

      // Make sure the total still matches up...
      if (($total = uc_order_get_total($order)) !== $order->order_total) {
        $fields['order_total'] = $total;
        $order->order_total = $total;
      }

      if (($count = uc_order_get_product_count($order)) !== $order->product_count) {
        $fields['product_count'] = $count;
        $order->product_count = $count;
      }

      if (count($fields)) {
        $query = db_update('uc_orders')
          ->fields($fields)
          ->condition('order_id', $order->order_id)
          ->execute();
      }
  /* ... */

I think that it should remain a read function and we should implement this in save() method.

Also we should not invoke more than one query / view where this is possible, so I suggest to pass to uc_order_actions(), an std object that emulates only order fields needed by uc_order_actions() like:

• order_status
• order_id
• uid

So, this way, we could query those fields as additional fields in handler::query().

I'm waiting for maintainers proposed fix before working on a patch for this major performance leak.

I'll dig into it today and let you know. Anyway, I thing we still have not to mix write methods with read methods and I think that uc_order should be fixed like in the attached patch.

With the attached patch I obtained the following timings on the same order views:
Before:

Query build time: 38.38 ms
Query execute time: 103.22 ms
View render time: 2476.98 ms

After:

Query build time: 29.49 ms
Query execute time: 77.8 ms
View render time: 889.72 ms

The problem is from typecasting.

class UcOrderController extends DrupalDefaultEntityController {

  function attachLoad(&$orders, $revision_id = FALSE) {
    foreach ($orders as &$order) {
    /* ... */
      // Make sure the total still matches up...
      if (($total = uc_order_get_total($order)) !== $order->order_total) {
        dpm ('Order total before: ' . $order->order_total);
        dpm ('Order total after: ' . $total);
        dpm ('Type of $total: ' . gettype ($total));
        dpm ('Type of $order->order_total: ' . gettype ($order->order_total));
        $fields['order_total'] = $total;
        $order->order_total = $total;
      }

      if (($count = uc_order_get_product_count($order)) !== $order->product_count) {
        dpm ('Order products count before: ' . $order->product_count);
        dpm ('Order products count after: ' . $count);
        dpm ('Type of $count: ' . gettype ($count));
        dpm ('Type of $order->product_count: ' . gettype ($order->product_count));
        $fields['product_count'] = $count;
        $order->product_count = $count;
      }
    }
  }
}

I've obtained the following output:

Order total before: 4533.00000
Order total after: 4533
Type of $total: double
Type of $order->order_total: string

Order products count before: 17
Order products count after: 17
Type of $count: integer
Type of $order->product_count: string

So

/* ... */
if (($total = uc_order_get_total($order)) !== $order->order_total) { // Always returns TRUE because decimals are !== than strings
/* ... */
if (($count = uc_order_get_product_count($order)) !== $order->product_count) { // Always returns TRUE because integers are !== than strings
/* ... */
 if (count($fields)) { // Because of previous wrong evaluations, always evaluates to TRUE.
    // This query is always executed, so performance leak is directly proportional with orders displayed per page in the view.
        $query = db_update('uc_orders')
          ->fields($fields)
          ->condition('order_id', $order->order_id)
          ->execute();
      }

So we fix this typecasting error, and we ensure that we are comparing apple with apple and pears with pears.

uc_order_handler_field_order_actions::render() loads an order for each displayed row, so an additional query is made to the database on each rendered row.
Because in uc_order_actions() we only need 3 order fields, order_id, order_status and uid I think a better approch would be to load those 2 additional fields order_status and uid in the views query, like core comment link does, and just mimic an order object when passing to uc_order_actions().
The patch also preserves backwards compatibility, but hook_uc_order_actions() and hook_uc_order_actions_alter() should be changed in future to get only $order_id as arg not the full $order object.
With all those 3 patches applied, I've got stunning results, attached screenshots speak for themselves.
I've tested with a view with 1000 orders / page for results to be more obvious.

Ups, I forgot to attach screenshots...

I am not sure it is safe to round order_total like that; there are edge cases with some taxes where order_total has more precision than uc_currency_prec specifies. Can we just use != instead of !== here?

I think no, we cannot use != here because we cannot rely on PHP calculating float value correctly. See the WARNING on PHP: Floating point numbers, exactly the part: "(...) So never trust floating number results to the last digit, and do not compare floating point numbers directly for equality. (...)

Se also user commens, including #113703:

$x = 8 - 6.4;  // which is equal to 1.6
$y = 1.6;
var_dump($x == $y); // is not true

// PHP thinks that 1.6 (coming from a difference) is not equal to 1.6. To make it work, use round()

var_dump(round($x, 2) == round($y, 2)); // this is true

So, i've executed the following in devel/php:

$x = 8 - 6.4;  // which is equal to 1.6
$y = 1.6;
dpm(var_dump($x == $y)); // is not true

and got bool(false) which is definitely NOT TRUE.

The uc_order_actions patch looks good though. I wonder if we can just get away with sending only those three properties to all implementations; there are likely very few other implementations of this hook out there.

So, you think that we should not preserve backward compatibility at all and just modify the hook and pass only those three params?

I think this should be reviewed and commited as fast as possible because views that contains order action are very slow and very annoying.