Issue with payment_ubercart_callback()

Patch attached. :)

Replaces...

elseif ($op == 'cart-process') {

With...

elseif ($op == 'cart-process' && isset($form_state['payment'])) {

Which prevents both the warning and the entity_save stuff from firing when the "cart-details" op has not yet been ran.

Meh, forgot the status.. so resubmitting. :)

Heh, sorry about the duplicate test. Didn't realize it'd submit the original one if I changed the status. :P

Popping this one over to a more appropriate status. :)

Is that really necessary though? Surely checking whether $form_state['payment'] is present before trying to save an entity from it is sensible regardless?

Perhaps. Or the documentation is incomplete / outdated?

The core ajax system of Ubercart builds upon the principle of "Any pane may wish to update its own display or that of another pane based on user input from input elements anywhere on the form". I believe this involves processing panes upon various changes so that inputted data is saved to the order and made available across the panes. For an example that the shipping pane can calculate shipping when someone types in their postal code. Or that the payment pane can have refreshed data available when you select a payment method.

I have my environment at work and unfortunately I don't have the opportunity to download everything I'd need to test in detail right now.

As I mentioned in my original description the issue is probably more likely to occur when you mix payment methods based on the standard ubercart hooks, with one or more Payment based payment methods, and the former type is set as default. When the panes are loaded on page load "cart-details" would not be called on payment_ubercart, so $form_state['payment'] is not populated at that point. When you then select a Payment based payment method... I assume the ubercart AJAX framework causes the panes to process, which in turn invokes the pane callbacks with the process op (according to module and/or pane weight). When it iterates through the panes and gets to uc_payment's pane callback... this considers payment_ubercart (representing the Payment based payment method you just selected) the currently selected payment method (perhaps saved to the order by previously processed panes) and invokes it's hook_uc_payment_method_callback with the "cart-process" op.

The question is whether it is intended that this processing happens prior to the op "cart-details", which is triggered by the payment pane when it's callback is called with the "view" operation. That brings us back to Ubercart's AJAX behavior, which is based on rebuilding and replacing the entire pane upon updates. With that in mind it seems somewhat sensible that processing occurs prior to panes being rebuilt/refreshed/replaced, because otherwise you risk losing inputted data, selections, and such (and/or working with outdated data) whenever an AJAX feature is triggered. Seeing that the AJAX behavior also involves that any pane can choose to update the display of other panes... I'd say that one should not assume that payment_ubercart's "cart-details" op will always be called prior to its "cart-process" op. Hence the suggested safe-guard against attempting to save an entity based on non-existing data. Especially when it involves saving an entry against a database table with fields that do not allow null values (triggers an uncaught PDOException -> fatal PHP error -> AJAX error).

I suppose you could dig deeper to properly verify whether or not the order of these ops is indeed intended behaviour, but the patch is such a light weight and simple solution to a potentially critical problem that I struggle to see how it'd be worth it just for the principle of it. All it does is checking the existence of the entity data before relying on it, which is not unreasonable.

Incorrect documentation is a bug as well.

I suppose you could say that; your code would not run into issues if Ubercart behaved precisely like the documentation describes the operations in question. I do still suspect the behaviour we are seeing is intentional, even if a bit messy, though.

1) hook_uc_payment_method_callback() is only executed for the selected payment method (which illustrates it's not even a hook at all).

Yeah, it's a bit ambiguous. In fact, it's just an ordinary payment method callback:

$order->payment_method = $form_state['values']['panes']['payment']['payment_method'];
$func = _uc_payment_method_data($order->payment_method, 'callback');
if (function_exists($func)) {
  $result = $func('cart-process', $order, $form, $form_state);

2) If cart-process is invoked on a payment method without cart-details being invoked prior to that, how can we be sure that Ubercart doesn't also trigger payment execution without invoking the cart-details pane beforehand?

Actually... Ubercart doesn't appear to use the "cart-process" op internally to process the form upon submission. I looked a bit closer at it today, and what happens is the following:

- uc_payment defines uc_checkout_pane_payment() as the pane callback in it's implementation of hook_uc_checkout_pane().
- The callback uc_checkout_pane_payment() is called with the op "process" whenever the pane is to be processed. One case being AJAX behaviour.
- The code block for the "process" op within uc_checkout_pane_payment() calls the callback for the selected payment method, seemingly with the sole purpose of determining whether or not the checkout process should be aborted.

$func = _uc_payment_method_data($order->payment_method, 'callback');
if (function_exists($func)) {
  $result = $func('cart-process', $order, $form, $form_state);
  if ($result === FALSE) {
    return FALSE;
  }
}

So the documentation should probably be altered to state..

Called for the selected payment method when the payment pane is processed, and can be used to abort the checkout process. Return FALSE to abort the checkout process, or NULL or TRUE to continue with checkout.

You then have the choice of using it as you are currently, with the proposed extra check prior to saving, or for an example implementing hook_uc_order() with the "submit" op instead. The latter strikes me as cleaner/more appropriate, and would also allow ubercart_payment to cleanly abort the checkout process upon payment failure. See the related #1969968: Behaviour upon order abortion or completion

Just set up a clean test-install to have a crack at #1969968 (if I get the time) - so I thought I would double check the steps to reproduce the initially reported issue here while I was at it. Here goes:

- Installed clean D7
- Installed ubercart with dependencies (handled by drush), and enabled typical UC modules (all core, all but "Catalog" and "File downloads" in core (optional), Stock and Ajax Admin in extra, none under fulfillment, Test gateway and Credit card under payment).
- Installed payment, payment_ubercart, and paymentmethodbasic.
- Added paymentmethodbasic as a payment method under Payment administration, called it "Payment Basic".
- Altered the weights under admin/store/settings/payment so that Ubercart's Credit card method was at the top (thus default).
- Added a test-product (generated by devel, just altered to give it a SKU of "TEST-PRODUCT-01", price of $100, and a simple test attribute.)
- Added the test-product to cart, and clicked "Checkout" on the cart page.
- Selected "Payment Basic" (paymentmethodbasic) as the Payment method.

This resulted in a browser dialogue with:

An AJAX HTTP error occurred.
HTTP Result Code: 500
Debugging information follows.
Path: /system/ajax
StatusText: Service unavailable (with message)
ResponseText: PDOException: SQLSTATE[HY000]: General error: 1364 Field 'description' doesn't have a default value: INSERT INTO {payment} (pmid) VALUES (:db_insert_placeholder_0); Array
(
[:db_insert_placeholder_0] => 0
)
in drupal_write_record() (line 7139 of [path]/drupal_patches/ubercart_site/includes/common.inc).

It was in other words not necessary to touch any other fields, or tick for an example "My billing information is the same as my delivery information.", beforehand. The error can be easily reproduced by simply switching from a defaulted Ubercart payment method to a Payment based payment method after the checkout page has loaded.

If i alter the weights under admin/store/settings/payment so that the Payment based method (in this case "Payment Basic") is at the top and default, the issue does not occur.

See #11 - Xano wanted more information on expected Ubercart behavior here, thus moved it to the UC queue.

Am I correct in assuming that the op "cart-process" should only be used to check whether or not the checkout should be aborted? And that with the AJAX behavior of Ubercart, one should not assume that the op "cart-details" on a payment method will be called priror to "cart-process"?

Indeed - see #13 for steps to reproduce if necessary. This should also be the case with two standard ubercart payment methods, for the one not set as default.

In Payment's case perhaps it makes more sense to move the 'cart-process' op to a custom submit handler that is added to the checkout form with hook_form_alter?

I agree, and that happens to be a part of what I am proposing in the patch we are working on in #1969968-4: Behaviour upon order abortion or completion as well.

Anyway, thank you very much for clearing that up for us. It's appreciated. I'll move this out of your queue now. :)

@Xano Patch for this particular issue available in #1 if you'd like to temporarily address the issue so that Payment is functional with mixed payment methods. Imo. the issue is fairly major.