Optionally require all panes on a checkout page to validate before processing them

Patch is working for me. Thanks!

Working here as well.

Hmm, I'm not sure I understand this patch. Can I get more info on how to recreate the scenario? Perhaps a screenshot of the checkout form and checkout form builder so I can recreate the form?

The patch reads to me that $validate will be set to TRUE if both $validate is TRUE and there are no previous messages, but it doesn't seem to me it should result in $validate being set to FALSE if there were in fact previous messages and $validate wasn't already FALSE.

But that's just my best guess based on http://www.php.net/manual/en/language.operators.bitwise.php.

I have a similar problem where form_set_error() in a checkout pane's validation handler doesn't stop the checkout from moving onto the next page.

We have a one step checkout process with the example payment method enabled. We aure using the commerce_module which defines its own checkout pane, and runs some validation the checkout panes validation hook. If both the validation for the commerce_registration checkout pane AND the validation for the example payment processor fail, both errors are show. If only the example payment processor fails the validation then its error message shows, but if only the commerce_registration module's checkout pane validation fails, the example payment processors submit handler runs, and the order is processed, causing the commerce_payment_order_paid_in_full rule to fire, which results in the error at #1482912: Fatal error when marking event as paid

The patch above doesn't help.

Looks like the problem is that you are running the submit handler for each checkout pane right after it passes validation, instead of waiting until all the checkout panes have validated before starting on the submit handlers. Attach patch reworks so that the submit handler are only called once all of the checkout panes have validated.

Thx for the patch mrfelton
Fixxed the issue overhere

Hmm, the problem is that we actually intentionally executed submit handlers this way as described in #1504966: Submit Handlers for checkout executed even if error on another pane.. That's not to say this isn't a better method, but the idea before was to allow any pane that validated to submit immediately to capture as much user information as possible. This approach may actually be a solution to the linked issue as well, but it would then go above and beyond the scope of this issue as I understood it.

In any case, patch in #6 did fix my problems with Commerce Fieldgroup Panes validation. So if this patch goes "above and beyond", could it have any bad consequences?

@rszrama - I think that's a really bad idea run submit handlers when there are validation errors. You are essentially treating checkout panes as individual forms, when in reality they are combined together to make just part of a larger form. I suspect this is what causes #1445464: Cards are being charged even when errors are present on the checkout form, a really serious error leading to customers being charged before they have even successfully submitted an order.

Yeah, it's definitely what's causing that. That issue comes up quite often unfortunately, and there are other issues that deal with it... I should probably close that one as a duplicate when I can find the better thread to point to. That said, payment method modules could be written to take this behavior into account if it were documented better, and for those that aren't the two step configuration that comes by default could be documented as necessary.

Found a bit more context in #842292: Consider altering the checkout form validation process where it's obvious this was at least an intentional decision, even if the thread doesn't include all the reasoning behind it. One item that comes to mind off the top of my head is we really wanted to be able to support an #ajax driven checkout form where panes could be progressively filled with their data being validated prior to adding additional panes to the page. This was due to requirements we'd had on projects in the past for a single page checkout that was continually updated with new elements like what you might have on bn.com as opposed to a true multi-step like amazon.com.

There's no reason we can't decide to re-architect the form, especially since a contrib could just as easily alter the checkout form to reintroduce this functionality through alternate validate / submit handlers, but it's not a decision I'd make lightly in the 1.x branch. Who knows what checkout forms and contributed modules you might break by fixing it for others. : ?

So how do we fix this in a payment module, as that is the main problem? In the payment validate handler, add another check? But it doesn't seem to have access to any of the required values to do that, as it doesn't have the form_state variable passed in.

Thanks.

I was able to do this in a custom pane by just wrapping the relevant action in the submit handler with an if statement checking for other errors on the form. Not sure if this is the best way or not.

  if(empty($form_state['storage']['errors'])) {
  //perform submit handler stuff
}

However, payment methods are handled differently, I think. At least, you don't have access to that form_state variable? Would that be a solution? Passing another variable to the payment submit handler by default? Not necessarily the form_state variable, but something that indicates if other errors exist on the form?

I am having problem using "commerce fieldgroup panes" with required fields.

The patch #6 above works great, as an alternative, you may do your own validation:

function mymodule_commerce_checkout_pane_info_alter(&$checkout_pane){
  $checkout_pane['commerce_fieldgroup_pane__group_mymodule_info__1']['callbacks'] = array(
    'checkout_form_validate' => 'mymodule_checkout_review_pane_checkout_form_validate',
  );
}

function mymodule_checkout_review_pane_checkout_form_validate($form, &$form_state, $checkout_pane, $order){
  $valid = TRUE;

  //do your validation
  if(!$valid){
    form_set_error('', 'Error');
  }

  return $valid;
}

I am not sure if this is the "proper way", but this works for our project.

Refocusing this issue into a feature request for more general use - namely the ability to delay checkout pane submission if desired until all panes on a page have validated. We're too late into 1.x to change this completely, but I wouldn't be opposed to adding an option to toggle this one way or the other with the default being the current behavior.

That said, the original issue involved payments being submitted when there are errors on the page, and that specific use case was accommodated in #1415670: Some CIM Orders Stuck In Checkout. I'd close this as a duplicate, but it seems some folks are also having issues w/ Commerce Fieldgroup Panes.

I should add that we're holding a 2.x planning sprint in a couple weeks, and I'll put this functionality on the to-do list to talk over.

I just discovered this issue as I dig deeper into a problem I'm having with commerce_donate.

I think I have a duplicate issue: #1851312: Payment validation occurs after other panes execute submit callbacks rather than before, resulting in non-standard form behavior but I can't tell if it's the same problem or not.

Can someone take a look and close my issue if it's in fact a duplicate?

As another use case (related to #1669810: Custom validation callbacks don't fire on checkout form) setting your own form errors for a specific element (for example postal codes) means that you don't receive the error message until the next form page. (oops).

Then it actually accepts the address regardless of the errors that were posted.

So I feel that using some kind of validations (even specific to that pane) should be done before firing the next page.

Hi,
I've got the same issue in the checkout review page. In my case a payment transaction is created regardless of errors generated in previous panes. This is because submit handlers are always called. Please apply the patch to call submit handlers only after all panes are validated and there are no errors.

Update: I've realized that this issue arise when any pane's validation callback returns FALSE or nothing without set a form error. The validation callback always must return TRUE or if validation fails must return FALSE and set the form error.

A related (but not necessarily the same) issue for this is that you are calling the submit handler from the validate handler at all.

If I implement some custom validation that runs after commerce_checkout_form_validate() I would expect my validation to run before any panes submit (that's how Drupal works everywhere else).

Because of the way this works though, the submission of panes (in my case the payment pane) runs before my validation hook runs and so things fail.

In my case it is not really feasible to have my validation run first because my validation functionality depends on previous pane validation passing.

There is also the possibility of things using form_set_value() in the validation phase, which would then not get picked up by the pane submit handlers.

I think this is a fairly major issue.
Also, I can't see how this is a feature instead of a bug, when functions like Drupal core's form_set_value() are effectively broken by this.

Also, having the payment handler submit incorrect information to a payment gateway due to not considering other validation sounds like a major bug to me.

What is the reason that all the pane submissions are not handled in commerce_checkout_form_submit(), after commerce_checkout_form_validate() has run for all panes?

Would it be viable to move

<?php
    // Submit the pane if it validated.
    if ($validate && $callback = commerce_checkout_pane_callback($checkout_pane, 'checkout_form_submit')) {
      $callback($form, $form_state, $checkout_pane, $order);
    }
?>

into commerce_checkout_form_submit() along with a commerce_order_save($order);?

Here is a (currently untested by me) patch of my fix idea, against 7.x-1.x-dev.

Alternatively, maybe there needs to be an option at the pane level (not exposed to the UI) that says whether or not the pane should be submitted immediately or not.

That way panes like payment (or other panes that aren't just saving data but running an action) could wait until the end and other panes could still save their data.

This still leaves an inconsistent experience to the rest of Drupal but would allow a non-hacky solution to some current problems.

This has been covered extensively in the past. There are multiple reasons for the architecture, but I don't have time to explain them all here. I can sum it up as "capture as much information as you can as early as you can" paired with "know exactly where to display errors in context when form submission fails."

Regardless, even if we wanted to change this, it would be a tremendously breaking API change that is not feasible for 1.x.

I read that in an earlier comment but it doesn't make it less of a bug.

What do you think about the fact that it breaks form_set_value() usage and can result in sending data to a payment gateway that hasn't fully passed validation?

Taking my patch as a possible solution, what API impacts would it have by making a change to that? (that's not a smart ass question, I'm not as across the possible impacts as you would be.)

The only thing I think of off the top of my head is that the order of the validation / submission process changed so if there are any checkout pane modules that were previously relying specifically on the current order of operations they would have to be amended if the order was changed.

I could be wrong but that doesn't sound hugely problematic.

Although there are also possibly rules that would be impacted, which could possibly be fairly problematic for site builders.

In terms of the feature request, do you have a strong preference for whether or not the configuration is per pane or store wide?

I haven't considered the possibilities in too much detail yet but my initial thought is that per pane would be more useful.

I will work on a patch sometime in the near future.

The main drawback? Any single form error will prevent the submit handlers from invoking and saving data. This is a huge change in terms of how the checkout form functions for any site that's dependent on early collection of data for cart abandonment logic, etc. That's the first one that comes to mind, but there are bound to be issues related to panes depending on other panes actually saving data to the database, such as to save coupons to an order object or save address updates for reference in future operations (e.g. Ajax based shipping recalculation or other Ajax based form updates).

Again, I don't think this is a bug so much as an explicit design decision. Just because it doesn't support every possible way the form API is used doesn't mean it's buggy... just means we need to do a better job of documenting how to interact with panes.

In your case, why not use an element validate handler instead of a form level handler, for example? Or prepend your validate handler to the #validate array? If you're feeling spunky, you could even alter the checkout pane info array to change the validate callback to a wrapper function that invokes the default validate callback and then performs additional validation - I've done this w/ submit callbacks for payment panes, for example.

Thanks for the response.

My workaround was the same as your last suggestion. I wrapped the last pane (payment) validation callback with my own function and I run the default pane validation and then my additional validation, making sure to update the $order in a way that data doesn't get lost when commerce_checkout_form_validate does it's save.

The only drawback is that it is tied to weight of the panes but in my case that isn't really an issue.
I could make it weight smart if I end up needing to but it's not worth the hassle for now since i have some additional complexity in multiple checkout lanes.

Good deal. It's worth noting that this architecture is very much on the table for improvement in 2.x. I'm not sure if there's a relevant issue open off the top of my head, but feel free to drop your notes in there re: difficulties in customizing checkout. Every bit of feedback helps. : )

Hi,

I have the following issue which may be related to the topic :

My checkout process should be based on two phase:
-> type any information need to process payment , payment type included.
-> verify page with all info and then validate all info.

But when I choose "back" button, and then choose another payment method, I finally have an order which has two payments instances related to.
So I believe that "commerce_checkout_form_submit" is called after each page validate button click...
Is there a submit callback which is called only once at the end of the checkout tunnel ?

Thx

As #29 explains, changing this logic would represent a large backwards compatibility break, and it's too late to make one in D7.

Commerce 8.x-2.x already works in the described way (all panes must pass validation before submission is started).