As a follow-up to #1212482-30: hook_commerce_payment_order_paid_in_full() does not fire in module hook, we've had a long standing issue regarding site messages on checkout pages (even on the completion page which is still built through the normal checkout form builder). We do a sort of early detection looking for error messages so we can display them inline with the particular checkout pane that failed, but it appears that normal messages are not being "put back" to appear properly. In fact, I tried to debug this for the last half hour and got nowhere... I believe the expected behavior is only:

1) Error messages from any given checkout pane should be displayed right above that pane (as opposed to at the top of the page with any other errors).

2) Other messages, including status messages, should be preserved.

I believe the issue is with the monkey'ing with $_SESSION['message'] in `function commerce_checkout_form_submit($form, &$form_state);` in modules/checkout/includes/commerce_checkout.pages.inc

function commerce_checkout_form_submit($form, &$form_state) {
  global $user; 

  $checkout_page = $form_state['checkout_page'];                                                                

  // Load a fresh copy of the order stored in the form.
  $order = commerce_order_load($form_state['order']->order_id);

  // Catch and clear already pushed messages.
  $previous_messages = drupal_get_messages();

  // Loop through the enabled checkout panes for the current page.
  $form_validate = TRUE;
  foreach (commerce_checkout_panes(array('enabled' => TRUE, 'page' => $checkout_page['page_id'])) as $pane_id => $checkout_pane) {
    $validate = TRUE;

    // If the pane has defined a checkout form validate handler...
    if ($callback = commerce_checkout_pane_callback($checkout_pane, 'checkout_form_validate')) {
      // Give it a chance to process the submitted data.
      $validate = $callback($form, $form_state, $checkout_pane, $order); 
    }

   // Catch and clear panes' messages.
   $pane_messages = drupal_get_messages(); 

    // 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);
    }
    // Generate status messages and theme them.
    $_SESSION['messages'] = array_merge_recursive($pane_messages, drupal_get_messages());
    $form_state['storage']['messages'][$pane_id] = theme('status_messages'); 

    // A failed pane makes the form fail. 
    $form_validate &= $validate;
  }

  // Restore messages and form errors.
  $_SESSION['messages'] = $previous_messages;
  ...
}

Comments

j0rd’s picture

subsSribe. This is important, else errors will get silently ignored by those debugging their checkout process with devel.

The only thing worse than a bug, is a bug which doesn't report itself.

joachim’s picture

Subscribe.

This is making working with custom checkout panes rather tricky, as there's no easy way to see what is in the $form_state in the submit handler.

rszrama’s picture

Category: feature » bug
Issue tags: +1.1 blocker

fwiw, I end up having to use watchdog(). : (

Tagging this.

joachim’s picture

BTW, for anyone needing a workaround until this is fixed, I recommend this sandbox project which provides a debug log: http://drupal.org/sandbox/gl90010/1257694

(And if you try it, help it on its way to becoming a full project: #1261904: Devel Debug Log.)

rszrama’s picture

Thanks for the contrib, joachim. Very handy.

megensel’s picture

StatusFileSize
new2.66 KB

After some digging I found that the messages were being caught before and after the foreach loop for the panes, but not in the loop. Also the $form_state['storage'] was being reset between redirects. The root problem was that the messages were not being restored during the loop though.

damien tournoud’s picture

Status: Active » Needs review
StatusFileSize
new4.26 KB

As identified by megensel, we do have two slightly different issues:

  • When moving from one step to the next, we lose messages generated during pane validation or submit, because those were caught in commerce_checkout_form_submit() but the form is never going to be displayed
  • We don't catch the messages generated in the pane callback, so those are displayed on top of the form

In this patch (largely based on #6 analysis), I simply re-add the messages, and move the generation of the themed version of the messages from the submit function to the form itself, which also solves #2.

rszrama’s picture

Status: Needs review » Fixed

Very good, guys. I've applied the patch in #7 with a minor change to the form element named [$pane_id]_errors. It is now [$pane_id]_messages. I've documented this as a minor API change in the commit log, but realistically there's no reason to suspect anyone has been altering this particular form element directly.

On a side note: if you're testing this, don't make the stupid mistake I did and try to test by not filling in required form elements. I totally forgot that Drupal handles these checks separately before any other form validation (completely silly if you ask me). So here I was throwing debugging messages around for half an hour when everything was really working as expected. :-/

Happy to lay this one to rest!

Commit: http://drupalcode.org/project/commerce.git/commitdiff/3aa2357

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

Anonymous’s picture

Issue summary: View changes

added code snippet in question