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;
...
}
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | preserve_messages-1243500-6.patch | 2.66 KB | megensel |
| #7 | 1243500-checkout-messages.patch | 4.26 KB | damien tournoud |
Comments
Comment #1
j0rd commentedsubsSribe. 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.
Comment #2
joachim commentedSubscribe.
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.
Comment #3
rszrama commentedfwiw, I end up having to use watchdog(). : (
Tagging this.
Comment #4
joachim commentedBTW, 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.)
Comment #5
rszrama commentedThanks for the contrib, joachim. Very handy.
Comment #6
megensel commentedAfter some digging I found that the messages were being caught before and after the
foreachloop 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.Comment #7
damien tournoud commentedAs identified by megensel, we do have two slightly different issues:
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.
Comment #8
rszrama commentedVery 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
Comment #9.0
(not verified) commentedadded code snippet in question