Abstract administration to be as generic as possible [#845602]

Non-form logic should NOT be present in administrative form calls. If this logic is necessary, then an extra layer of abstraction needs to be present to invoke that logic separately from the form. This should be a wrapper function.

Example:

commerce_product_ui_product_form() makes a call to commerce_product_ui_set_breadcrumb(), this call (and the associated logic for it) should be moved to a wrapper function for commerce_product_ui_product_form() so that the administrative form function(s) can be utilized in situations we haven't yet anticipated.

Comment	File	Size	Author
#8	845602.product_types.patch	22.41 KB	rszrama
#6	845602.delete_form_altering.patch	11.19 KB	rszrama
#5	845602.edit_form_altering.patch	8.68 KB	rszrama
#4	845602.cleanup_1-4.patch	12.97 KB	rszrama

Comments

Comment #1

damien tournoud commented 5 July 2010 at 16:28

Agreed. Patch? :)

Comment #2

eclipsegc commented 6 July 2010 at 22:54

I've been working on this, but I wanted to place it where others could take a look at it and participate/collaborate and hopefully give me some feedback.

I've branched specifically for this issue:

http://github.com/EclipseGc/drupalcommerce/tree/845602

I believe I have products and orders both largely done:

What's the reasoning behind this?

Good question, essentially any administrative functionality that didn't previously stand on its own, I've attempted to make it stand alone. This has largely revolved around forms of various sorts. Edit and delete functionality in most cases was making additional calls to set the breadcrumbs. There's at least one instance of a title being set (thus far), etc etc. If an administration function already stood alone as it was, I left it alone, if it didn't, then I've migrated the various form/validate/submit functions as necessary. This has gone pretty smoothly thus far, but I could really use someone walking behind me and testing to make sure I didn't break anything.

Please let me know what you come across, and I'll keep working against the additional modules that I haven't touched yet.

Eclipse

Comment #3

rszrama commented 8 July 2010 at 14:33

Status:

Active

» Needs work

We discussed a few of these things in IRC:

The page callbacks in the UI modules should be *_form_wrapper() instead of just *_form(), and the comments need to be updated to match (one form builder function was called a form wrapper... I think commerce_order_order_form()).
You're fiddling with the $form_state in the order form but not the product form - not sure if that needs to be duplicated for product or removed for order, but you mentioned needing it for something w/ the Field Attach API.
The $breadcrumb argument on the UI wrappers is no longer necessary, as we'll always be wanting the breadcrumb when we call those functions. You can get rid of the argument in the function definitions and remove the related if statement.
I don't see the //$Id$ header or @file docblock in the form.inc files.
We discussed what to do about form submission and redirection briefly on the phone. If you want to do this as a two stage commit and open a second issue for this point, that'd be fine. Basically, I think we should pursue having a separate form ID for the UI forms. The base form would just have a single submit button with no cancel link, and upon submission would just remain on the same page. The UI form would need to alter in Cancel links and potentially other buttons (like the "Save and add another" button for products) and add submit handlers to the forms to set the proper $form_state['redirect'].
The separate form ID can be accomplished by a simple implementation of hook_forms():
```
/**
 * Implements hook_forms().
 */
function commerce_product_ui_forms($form_id, $args) {
  $forms = array();

  if ($form_id == 'commerce_product_ui_product_form') {
    $forms['commerce_product_ui_product_form'] = array(
      'callback' => 'commerce_product_product_form',
    );
  }

  return $forms;
}
```
I believe the advantage of this is going to be that from commerce_product_ui_product_form_wrapper() you could then call drupal_get_form('commerce_product_ui_product_form') and be able to alter the buttons in properly. What I don't know is what this would do for other modules altering the product form... does this mean we'd need to instruct modules altering these forms to detect $form['#base'] or something? Looking at cart as an example.