Disable links to guarantee form data integrity

+++ commerce_checkout_progress.install
@@ -18,4 +18,51 @@ function commerce_checkout_progress_unintall() {
\ No newline at end of file

This could cause issues with the packaging script if we include into Kickstart.

+++ commerce_checkout_progress.install
@@ -18,4 +18,51 @@ function commerce_checkout_progress_unintall() {
+  return t('Disabling links of Commerce Progress Checkout.');

We probably have to use $t = get_t(); here or st().

+++ commerce_checkout_progress.install
@@ -18,4 +18,51 @@ function commerce_checkout_progress_unintall() {
+  $checkout_progress_links_link = l(t('disable'), 'admin/commerce/config/checkout');

You're not using the $t here and it's not wrapped into the runtime condition.

+++ commerce_checkout_progress.install
@@ -18,4 +18,51 @@ function commerce_checkout_progress_unintall() {
\ No newline at end of file

Same here, we need the newline.

You didn't attach the correct patch :)

Hey, thanks for being active perusio and jsacksick ;) I guess that the hook_update_N() is not necessary. If we were to change the default value, we should change it in variable_get(). In the current path, even if the site build explicitely sets the value to TRUE in the settings page, it is reverted to FALSE, which is something not expected.

Sure. The last stable was a year ago and site usage has increased x5 since then.

+++ commerce_checkout_progress.install
@@ -18,4 +18,51 @@ function commerce_checkout_progress_unintall() {
+  if (variable_set('commerce_checkout_progress_link', FALSE)) {

Should be variable_get for the if check (Not variable_set).

+++ commerce_checkout_progress.module
@@ -214,15 +214,15 @@ function theme_commerce_checkout_progress_list($variables) {
+                 'items' => $progress,

Why did you update that part ?

Is it only me or the commerce_checkout_progress_update_7001() is not really necessary? It does not change anything, unless later we are mad and decide to revert this default value change (aka if the variable is not set, leave it not set).

A few nit picks on the review of the latest patch.

1. +++ commerce_checkout_progress.module
   @@ -214,15 +214,15 @@ function theme_commerce_checkout_progress_list($variables) {
   -    'items' => $progress,
   -    'type' => $type,
   -    'attributes' => array('class' => array('commerce-checkout-progress', 'clearfix', 'inline',)),
   -  ));
   +                 'items' => $progress,
   +                 'type' => $type,
   +                 'attributes' => array('class' => array('commerce-checkout-progress', 'clearfix', 'inline',)),
   +               ));
   

   This indent change is not necessary and is against the drupal coding standards.

2. +++ commerce_checkout_progress.module
   @@ -241,7 +241,8 @@ function commerce_checkout_progress_form_commerce_checkout_builder_form_alter(&$
   +      '#description' => t('If enabled this could cause problems in the checkout process. It\'s strongly <strong>unadvised</strong> to enable it.'),
   

   Maybe use positive language here: "It's strongly advised to leave this disabled."

Re-roll of this patch to work on latest dev. Fixed the formatting errors and included Joel's language suggestion. Slightly revised the README to be markdown friendly.

Also, hiding the other patches since they appear to be cumulative and a bit of a pain to go through.

Thank you for the fixes @nvahalik. Here are a couple on the new patch:

1. +++ b/commerce_checkout_progress.install
   @@ -20,3 +20,48 @@ function commerce_checkout_progress_update_7000() {
   +  if (variable_get('commerce_checkout_progress_link', FALSE)) {
   +    variable_set('commerce_checkout_progress_link', FALSE);
   

   Wouldn't this change existing sites to FALSE that may have wanted to use links?

2. +++ b/commerce_checkout_progress.install
   @@ -20,3 +20,48 @@ function commerce_checkout_progress_update_7000() {
   +      $description = t('Commerce Checkout Progress links are active on the checkout process. You should ' . $checkout_progress_links_link . ' it to avoid data integrity problems during checkout.');
   

   Should be a !placeholder not a concatenation inside this translation or it's effectively not translatable.

3. +++ b/commerce_checkout_progress.install
   @@ -20,3 +20,48 @@ function commerce_checkout_progress_update_7000() {
   +  } // runtime
   

   Probably don't need this comment.

Joel,

How about a compromise. We'll still disable the links (since again, that's the whole point this patch) but we'll also go ahead and issue a warning so that whether you're using Drush or you're doing your updates via the web, you'll get a nice warning message letting you know that you can turn them back on in the configuration (with a link).

Rerolled patch with Joel's excellent comments and a better warning for those who are updating.

How about setting their defaults to false like you have and leaving the existing sites be the way they have them? Put a warning if you'd like that they should disable them if you want but don't change existing sites.

Okay. Let's not change the functionality for existing users. But we'll add a proper warning during the update.

Duplicate payments sounds horrible, but in my case I'm not taking payments till after the order is submitted so I'm not to worried about this, unless there is another reason these get requests cause issues?

Thanks for the change @nvahalik

One potential issue would be if someone didn't realize that changes they've made to the existing cart page didn't get saved when they decide to go back to the cart. I think it's a pretty safe assumption that if you click on something in the cart to make further changes to it, that anything you've done on the existing cart page would be saved. It'd be no different than clicking continue. So in that regard, I think until at some point the links would properly post the request back to the cart and then redirect the user, not having them as regular links would be ideal.

By default, are these links still GET requests?

That is to say, is having the list render as links still causing integrity problems?

Thanks,
Keith