hook_uc_cart_pane(), hook_uc_checkout_pane() and hook_uc_order_pane() specify unique IDs for each pane, but these are not used as the array key, making it more difficult than should be necessary to find a specific pane from code.

As an example I suggest we change:

  $panes[] = array(
    'id' => 'cart',
    'callback' => 'uc_checkout_pane_cart',
    'title' => t('Cart contents'),
    'desc' => t("Display the contents of a customer's shopping cart."),
    'weight' => 1,
    'process' => FALSE,
    'collapsible' => FALSE,
  );

to

  $panes['cart'] = array(
    'callback' => 'uc_checkout_pane_cart',
    'title' => t('Cart contents'),
    'desc' => t("Display the contents of a customer's shopping cart."),
    'weight' => 1,
    'process' => FALSE,
    'collapsible' => FALSE,
  );
CommentFileSizeAuthor
#12 1168280-hook_uc_order_state-dx.patch5.85 KBlongwave
#11 1168280-hook_uc_line_item-dx.patch6.93 KBlongwave
#10 1168280-payment-methods-and-gateways.patch30.36 KBlongwave
#5 1168280-hook_uc_cart_pane-dx.patch6.71 KBlongwave
#4 1168280-hook_uc_cart_pane-dx.patch6.46 KBlongwave

Comments

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Title: DX: cart, checkout and order panes should be keyed by pane ID » DX: cart, checkout and order panes and payment methods/gateways should be keyed by ID

hook_uc_payment_method() and hook_uc_payment_gateway() could similarly be improved this way.

I don't see why this couldn't be backward compatible, we can just ensure $key == $value['id'] after running module_invoke_all().

tr’s picture

tr commented

hook_uc_shipping_method() implementations have an 'id' key *and* an array key. By convention they are identical . For example:

  $methods['ups'] = array(
    'id' => 'ups',
    'module' => 'uc_ups',
    'title' => t('UPS'),
    'enabled' => $enabled['ups'],
    'quote' => array(
      'type' => 'small_package',
      'callback' => 'uc_ups_quote',
      'accessorials' => _uc_ups_service_list(),
    ),
    'ship' => array(
      'type' => 'small_package',
      'callback' => 'uc_ups_fulfill_order',
      'pkg_types' => _uc_ups_pkg_types(),
    ),
    'cancel' => 'uc_ups_void_shipment',
    'weight' => $weight['ups'],
  );

We could do this same thing for the five hooks mentioned above without having to worry about doing anything for backwards compatibility.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Active » Needs review

Sample patch for hook_uc_cart_pane(). The usefulness of this can be summarized by looking at the changes in the hook_uc_cart_pane_alter() example, from:

  foreach ($panes as &$pane) {
    if ($pane['id'] == 'cart') {
      $pane['body'] = drupal_get_form('my_custom_pane_form_builder', $items);
    }
  }

to

  $panes['cart_form']['body'] = drupal_get_form('my_custom_pane_form_builder', $items);
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
StatusFileSize
new 1168280-hook_uc_cart_pane-dx.patch6.46 KB

Let's try attaching that again.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
StatusFileSize
new 1168280-hook_uc_cart_pane-dx.patch6.71 KB

The static cart pane cache seems useless, so this patch removes that too.

tr’s picture

tr commented 
-  $panes[] = array(
-    'id' => 'uc_google_checkout',
+  $panes['uc_google_checkout'] = array(

So you don't like the idea of just doing

-  $panes[] = array(
+  $panes['uc_google_checkout'] = array(

so we don't have to worry about unintended consequences of removing 'id' ?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

There seems little point in asking developers to specify each id twice in the hook.

  foreach (module_invoke_all('uc_cart_pane', $items) as $id => $pane) {
    // Preserve backward compatibility for panes with no key specified.
    if (is_numeric($id)) {
      $id = $pane['id'];
    }

    // Set defaults.
    $panes[$id] = $pane + array(
      'id' => $id,
      'enabled' => TRUE,
      'weight' => 0,
      'body' => array(),
    );
  }

Old $panes[] = array(...) declarations will have their key explicitly set. New declarations with no 'id' member will be given one, so code expecting the old style will still work.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Active

Committed the above patch plus similar patches for hook_uc_checkout_pane() and hook_uc_order_pane().

Along with payment methods/gateways, I also noticed that hook_uc_line_item() could be improved in the same way, as could hook_uc_order_state() (although that last one is very minor).

tr’s picture

tr commented

Commit http://drupalcode.org/project/ubercart.git/commitdiff/77800bb introduced a bug, now fixed: #1186520: Notice: Undefined variable: i in uc_cart_filter_checkout_panes()

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
StatusFileSize
new 1168280-payment-methods-and-gateways.patch30.36 KB

Committed the attached patchset which changes hook_uc_payment_method() and hook_uc_payment_gateway() to be keyed by ID.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Active » Needs review
StatusFileSize
new 1168280-hook_uc_line_item-dx.patch6.93 KB

hook_uc_line_item patch attached.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Fixed
StatusFileSize
new 1168280-hook_uc_order_state-dx.patch5.85 KB

Committed #11 along with the attached patch for hook_uc_order_state(), which I think means this can be marked fixed.

Status: Fixed » Closed (fixed)

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