Export UI - unified ui for exportables

The second link seems to go to ctools module -- didn't see message module in there?

So I haven't fully inxpected the code to follow things through, so there's a lot I'm currently missing. It's also a little hard to follow without some guiding, I think, because I can't actually figure out what message has implemented beyond a Views-like form class, which is nifty, but it seems like there's a lot more in the way of options that need to be set, like menu paths and how you sort and what you list?

My initial thoughts:

1) This shouldn't be an external module. It should probably be something in CTools' includes and take advantage of the menu/theme delegation pieces. See *.menu.inc files in CTools' includes.

2) I'm on the fence about whether or not implementations of this feature should be a plugin. It doesn't quite fit the standard paradigm of a plugin, where generally at some point you've got many to choose from and you're choosing them for some reason. This activates and turns on and you never really manipulate it. On the other hand, having it as a plugin makes for a very easy registration system. Without that we'd end up having to write an additional one. Thus, fence-sitting. Need more thought on this.

Please post patches here, too. Sorry that the mirror I'm currently using on github isn't friendlier to forking/that the public branches get rebased, but I gather that you generated that branch from a tarball, so it's a killer for me to directly compare against.

Some questions/comments so far:

1. There's enough overlap (conceptually, at least) with the Bulk Exporter that I'd prefer to see this effort merged with that. Given that this is a lot more complex than that, though, I'm more than happy to see this effort eat the Bulk Exporter entirely.
2. Is part of the goal here to make some generic tools for generating the table lists of exportable objects? If not, then I think it should be. And any such functionality should live directly as a ctools include (though not in includes/export.inc - perhaps includes/export-ui.inc), not under the module, so that we can use it whether or not the module has been enabled.
3. What's that hook_js_replacements() thing? issue# please?
4. Make sure all import forms are protected by the 'use PHP for block visibility' perm. It's our official-hacky way of ensuring that you should be able to execute arbitrary PHP.
5. These sorts of forms tend towards XSRF vulnerabilities - please make sure confirm forms or tokens are used where appropriate.
6. Please don't use hooks for the plugin implementation. Put them into a separate .inc file that lives next to a class file, then use the $plugin magic var: f.e., http://github.com/sdboyer/panels/tree/dr_obj/plugins/display_renderers/
7. __construct() is PHP5. Until we get to D7, you gotta stick with PHP4 constructors.

With all that said, I'm very excited about this :)

Hmm. it does seem like this system would possibly own the bulk exporter, but perhaps that's a folding in that would come later. I would not combine this with bulk exporter at this time.

> hook_js_replacements()

sorry, I was diffing wrong and it looked like you were changing something with that system. Thought I got rid of that line before I posted, evidently not.

> $plugin instead of hooks

The biggest reason is because we think they're more self-documenting: someone coming in and looking at your project will know immediately, based on the magic file naming, which plugin declaration goes with which plugin. The performance difference is negligible.

Keep it coming! I probably won't have time myself to look at this for a little while, unfortunately... :(

Ok! I downloaded this patch, applied it, and tinkered around with it to get it working. It was a little trickier to get working than I hoped, but I did. Here are some comments:

1) Missing features. Just going to mention these because I'm going to require them anyway, so we should definitely support them. I don't know if they're on your todo list.
- choosing displayable columns. This probably requires an array of columns and ways of determining how they are displayed.
- sort/filter widgets like on the top of the views, pages, styles. Likewise above. Code will need to be given to determine how the filters are actually applied, though some of them can be automatic.

2) Use of $plugin['name'] to match the schema is a nono. $plugin['name'] is a reserved word and it must always match the actual name of the plugin. The schema should be allowed to be different. When I got this working, I changed it to schema and found that it had a couple of assumptions about name vs schema that caused problems.

3) ctools_export_ui_get_plugin_by_menu() is a mess and unnecessary. The menu item should simply have the $plugin key as the first argument all the time. Then you simply load the plugin and go.

4) It seems like we don't really need to have save(), load(), export() methods on the object. Instead, we should have the schema itself define these functions and provide for simple defaults that just do the basic stuff. The nice thing about that is that it becomes *very* easy to have nearly perfect CRUD which is something export.inc comes close to but does not quite finish. Since this system requires CRUD to be perfect, we should push that into export.inc and let everything use it, not just the UI.

5) Having data wrong in the plugin causes errors and notices. This was the hardest part about getting it working since I had actual whitescreens until I got the naming right.

6) There is a special plugin hook you can use to provide defaults for a plugin. This would be a much better way than is currently used to add the 'has menu' bits. There is one existing for content_types -- you can see that one to give an idea how it works.

7) I would like to see more of the menu item data in the $plugin array itself. This is kind of the master configuration of the plugin, and you can glance at it and see what all the options are. Plus, it'll be a great way for people to learn to use this by following existing examples. Using that plus the defaults hook, you can do quite powerful stuff there and have it be pretty transparent to the rest of the system.

I have a need for this system in the very near future to handle customizable content types, so my participation in this issue is going to increase. Do you know when you'll be able to do another round? If it's not soon enough, I may do another round of this myself, but I don't want to stomp on you if you've got time to work on it right away. (I probably won't work on it until after the weekend; US holiday coming up and I'm booked full for the weekend).

One other comment is that a few other things should be more obviously configurable at the $plugin level:

• The menu path/prefix
• The main title of the menu path
• The operations: enable/disable/export/import should all be optional features that are on by default, but easily removed by setting a flag.
• For stylizer, the main tab will itself be a subtab. This seems like something that could be easily flaggable.

I'm not entirely enamoured of the object in use here. Especially if we pull back the CRUD functions, all the object is going to do for us is to handle hook_menu and the main edit form, which is nice but may be overkill. It will end up working very much like the 'page' plugins which also have hook_menu, but the DX will be different. I realize sdboyer is keen to go OO but I also like consistency. Plus, I prefer to use OO when there's an obvious benefit. Perhaps this one will have good obvious benefit when we get to the list columns/sorting/filtering feature because that one is going to need more interaction with the plugin in order to do specialized sorting/filtering.

One other comment. This doesn't actually need to be part of this patch, but it's something that should probably follow. The filters on the Page Manager page have auto-submit capability and this should be genericized and made into its own javascript widget that can easily just be turned on and have form fields marked appropriately and the included javascript do the right thing. I'm willing to let that wait for a followup though.

Here's my first real review of the code.

+    ctools_include('plugins');
+    // TODO: We include export-ui for ctools_export_ui_defaults().
+    // Maybe ctools_get_plugins() should do it for us.
+    ctools_include('export-ui');
// ...
+    foreach (ctools_get_plugins('ctools', 'export_ui') as $plugin) {

What I usually do here looks like this:

  ctools_include('export-ui');
  foreach (ctools_get_export_uis() as $plugin) {

And then in export-ui there will be two functions: ctools_get_export_uis() which gets all, and ctools_get_export_ui() which gets just one. They're really just convenience to make things more readable but I've been happy with the result, and since it includes plugins for you, you don't have to do it. And you need export-ui to be included whenever you use those plugins anyway, and there's no way for plugins.inc to have any idea what file to include to get data on the plugin.

Also, this foreach seems deadly. Why is the $plugin_name not coming into the function directly?

+      drupal_set_title(t('Add a new @plugin', array('@plugin' => $title)));

Hardcoding a title -- we are better than this.

We can define these titles in the plugin and use sane defaults if not defined.

+  $form['plugin'] = array(
+    '#type' => 'value',
+    '#value' => $plugin_name,
+  );

If you're going to do this, use $plugin, not $plugin_name -- The name can always be gotten via $plugin['name'] and it saves you from having to reload the plugin.

+      $description = t('This action will permanently remove any customizations made to this export.');

+      $description = t('This action will remove this export permanently from your site.');

Hardcoded strings again. I won't point out more of these; we should just do a sweep for strings that should be settable in the plugin and defaulted appropriately.

I'm not really sure those 4 forms should be combined. The amount of switch/case logic you have in there is really doing away with the benefits of trying to reduce the code, I think. Also, I've never bothered with confirms on enable/disable, so only revert/delete really should have a confirm, IMO.

+          $links[$key] = l($value, $url);

We need to be sure to put tokens on these as per recent CSRF patch to page manager.

+      $row['data'][] = array('data' => implode(' | ', $links), 'class' => 'export-links');

I usually use theme('links') for this.

+ 'save callback' => function_exists("$schema[module]_save") ? "$schema[module]_save" : FALSE,
+ 'delete callback' => function_exists("$schema[module]_delete") ? "$schema[module]_delete" : FALSE,
+ 'load callback' => function_exists("$schema[module]_load") ? "$schema[module]_load" : FALSE,

I bet ctools could provide default functions that do the minimal functionality for these three. Perhaps we should do the same for 'export callback' as well. We also probably need one for 'load all' since that is a different operation than simply 'load'.

I'm changing this to 'needs review' to see if we get more eyeballs.

Confirmation forms are an effective defense against CSRF; if you're using them, then there's no need for a token.

Though I also don't think we need confirmation for enable/disable. =)

Here is an interim patch. I decided to go ahead and work on the listing stuff, so I've done a lot:

1) The list is actually an object. I initially implemented this with callbacks and it's a LOT cleaner as an object, so I'm going with this.
2) This includes a generic auto-submit.js that keeps our basic auto submit functionality. As a bonus this'll make it trival to turn views exposed forms into auto submitting ajax forms.
3) Because of the way the listing is done, it is highly geared toward tables with configurable columns.

I like the way this turned out; I want to redo editing the same way with a separate object:
1) Some of the edit systems I use don't use a single form, so I want to not impose that. Consider page manager pages, mini panels and Views which all have complex editing forms that combine some form and non-form stuff on the page, plus have lots of other sub pages.

There's a couple of TODO items I left in this patch. I actually undid one thing Amitaibu did that needs to be restored, but it needs to be done in such a way as to make it easy to set the 'enable' and 'disable' operations to use AJAX while other operations use normal clicks.

For testing purposes, I'm currently using this

// Make this panels/panels_stylizer/plugins/export_ui/panels_stylizer.inc
// in order for this to work, the panels_stylizer_ctools_plugin_directory() function has to
// be modified to allow export_ui plugin. To do it quickly just allow all ctools and panels plugins.

// This currently does not implement a proper edit form, so this only tests listing,
// enabling, disabling, exporting, importing and cloning.

$plugin = array(
  'schema' => 'panels_style',
  'menu' => array(
    'menu item' => 'stylizer',
  ),
  'title' => t('Stylizer'),
  'form' => array(
    'settings' => 'panels_stylizer_ctools_export_ui_form',
  ),
  'handler' => array(
    'class' => 'ctools_export_ui',
  ),
);

function panels_stylizer_ctools_export_ui_form(&$form, $plugin_name, $op, $export) {
  $form['markup'] = array('#value' => t('This is the form'));
}

I'm continuing to work so expect another patch later in the day. If you have specific comments I can address them right away.

We absolutely do need XSRF for enable/disable. Though we wouldn't be using this for Page Manager, think about the effect on a site of an XSRF attack that, say, disabled the node_view handler. It's more pesky than anything else, but it's still a realistic and potentially damaging attack vector.

Fortunately, this is exactly the kind of place where it's invaluable do just do it once, and be done - for the enable/disable case, I would rather the use of a token (for smooth workflow reasons). And if we do it right here in the generic, reusable Export UI, everybody gets to benefit :)

Here's a checkpoint patch. I had thought to make separate objects for list and edit, but decided to go back to the original way Amitaibu had it more or less.

Things I've done:

1) Smoothed out the _load() function so it doesn't have to cycle through all the plugins anymore.
2) Got add/edit/clone/enable/disable more or less transitioned and working. The edit form is making a lot fewer assumptions than before. I completely split out some of the logic, too, because I hate _submit functions that also save.
3) Enable/disable are now ajaxy

Still to do:

a) Import, export, delete, revert need to be transitioned. Import is currently saving too early, IMO; I prefer it doesn't do a final save until the very end. That usually means storing the import somewhere. Probably the session for the simple case.

b) The menu stuff needs to move out of the plugin and onto the object to make it a little easier to alter. Right now it's actually hard to alter because you have to make your additions *before* you know what's there rather than after. It'll be much easier if we can simply make changes to the base stuff and add to it if necessary.

c) Still need to revert the operations to a loop.

d) ctools_export_ui_list_items() needs to transition to the smart switcher.

e) export.inc should contain functions that handle the 'load callback' (and others) so you don't have to know how to dig into the schema to do it.

f) ctools_export_ui_export_object() has been pretty much stripped and could be made better.

g) _ctools_export_ui_redirect() is poorly named and kind of ugly. Would like it to be easier to get paths.

h) More flexibility in titles. For stylizer, an object is a 'Style' (though actually there are Pane styles and Region styles) but the administration is Stylizer.

i) Drupal 7 has a nifty method of auto guessing names from titles that is standard. We should integrate that here as a nice standard.

j) CSRF protection on enable/disable

k) Still many calls to ctools_get_plugins() that I'm slowly getting rid of.

l) documentation

Hm. Odd.

If I understand correctly you prefer having all those operations wrapped - What is the benefit of the transitioned functions?

The client has total control over them if it's needed. While I predict it will not be needed for delete/revert/import/export as these operations are unlikely to change from implementation to implementation, it's critical for list/edit/add because when I envision transitioning Page Manager to using this, it needs a MUCH more complex system than this will offer. The best way is to simply hand control over to the client and let it do what it wants. If we wrap everything, then it all works. Also, once we start doing this, we may as well do it all the way in order to apply the principles of encapsulation properly.

The menu stuff needs to move out of the plugin and onto the object to make it a little easier to alter

We actually had it in the object in hook_menu() and moved it to the $plugin. I actually think it might be better in the $plugin -- as it's a configuration, and not an action. If it's in the $plugin implementing modules are not forced to add foo.class.php just for the menu override.

I know. However, I was thinking about stylizer's need to have multiple 'add' tabs -- and modifying the menu from within the plugin is going to be difficult. There's logic in there that the plugin will have to duplicate. I think in the plugin actually making changes to the menu is not going to be very good DX, and if the whole point of having the menu in the plugin is for DX, then we have to question why it is there.

I hate submit functions that also save because I believe that the UI logic should be separated from the storage logic. If the _submit function saves, you can't repurpose the form. The *mess* that is 'node preview' is one really good example of why this fails.

IMO the pattern should always be that the _submit handler builds up an object, and then the caller of the form is responsible for doing what needs to be done with that object. In the preview case, it's rendering it; in the save case, it's storing it.

Change $form[#programmed] = TRUE to $form[#post] = TRUE to be compatible with drupal_prepare_form() -- and for values to appear even if $_SESSION['ctols_export_ui'] isn't set.

Are you sure about this one? I've had the biggest bear of a time getting those forms to work right in the default case -- does that actually do it?

I'm going to go back to work on this, so unless you're here right now, no need to act on this stuff, I'll integrate my comments into the next patch.

-function ctools_export_ui_switcher_page($plugin_name, $op) {
-  $args = func_get_args();
+function ctools_export_ui_switcher_page($plugin_name, $op, $item = array()) {
   $js = !empty($_REQUEST['ctools_ajax']);
 
   // Load the $plugin information
-  ctools_include('export-ui');
   ctools_include('export');
   $plugin = ctools_get_export_ui($plugin_name);
 
@@ -43,9 +41,11 @@ function ctools_export_ui_switcher_page($plugin_name, $op) {
   if ($handler) {
     $method = $op . '_page';
     if (method_exists($handler, $method)) {
-      // replace the first two arguments:
-      $args[0] = $js;
-      $args[1] = $_POST;
+      $args = array(
+        'js' => $js,
+        'input' => $_POST,
+        'item' => $item,
+      );

This change is not a good idea; I used func_get_args() to make sure that whatever was passed to the switcher function gets passed through. This is going to be significantly more flexible as some pages may need additional arguments.

I'm not sure why you remove the export-ui include, but since the next function called requires export-ui that seems necessary.

+function ctools_export_ui_plugin_base_path($plugin_name) {

It seems like most usages we have of this function, we have $plugin already and are passing $plugin['name']. Let's save ourselves a plugin load and just have this take the loaded plugin.

+        if (in_array($op, array('enable', 'disable'))) {
+          $operations[$op]['attributes'] = array('class' => 'ctools-use-ajax');
+        }

This is hardcoding. It's not a big deal, but it makes it impossible for a client to add operations that use ajax.

+   // Add plugin's form definitions.
+   if (!empty($this->plugin['form']['settings'])) {
+     // Pass $form by reference.
+     $this->plugin['form']['settings']($form, $this->plugin, $form_state['op'], $item);
+   }
+

If we're going to do this, we should also include corresponding submit and validate callbacks as well. I'm a bit torn on this, but I do think it's likely a nice shortcut for the very simple case that only needs to add stuff to the form and so doesn't need to create an object at all.

Also, no need to pass $item as an argument, we should just use $form and $form_state -- that's more consistent and includes the other arguments already.

Newest patch. Most of the items on my TODO list are now complete. These remain:

h) Revisit the strings with an eye for flexibility and ease of use for translators. My experience is that trying to build strings up is bad for translators.

i) Drupal 7 has a nifty method of auto guessing names from titles that is standard. We should integrate that here as a nice standard.

l) documentation

The CRUD stuff is something I'm extremely happy with. In simple tests, everything is working quite nicely. I think the next thing I need to do is complete the stylizer implementation and see what rough spots I hit, but other than the items above, I think this is really close to being feature complete and ready to commit.

I'm also attaching the patch to Panels to do my not-complete stylizer test. While it doesn't have the stylizer edit forms (those are going to take some work as it needs the wizard) it does do all the basic functions correctly as far as I can tell.

+    'handler' => array(
+      'class' => 'ctools_export_ui',
+    ),

I might be mistaken but I don't think this works, because it belongs to a different plugin and it needs to be sure to load the plugin file to get this. I don't think it will. The class loader will fall back to the default plugin if no handler is specified.

+  if ($reset) {
+    ctools_export_load_object_reset($table);
+  }
+

Ok the fact that I omitted this by accident makes me laugh. Good catch, thanks.

+    $this->items = ctools_export_crud_load_all($this->plugin['schema'], !empty($input['js']));

$js rather than $input['js']?

-    $class =
-

Yup, must be a leftover from shifting code around.

   $menu_prefix = ctools_export_ui_plugin_base_path($plugin);
-  $form_state['redirect'] = "$menu_prefix/list/$item->{$export_key}/delete";
+  $form_state['redirect'] = ctools_export_ui_plugin_menu_path($plugin, 'delete', $item->{$export_key});

Should be a - on $menu_prefix = ... as we..

I think this is going to get committed today. Would love one other reviewer just to get one more pair of eyeballs to look for stuff.

Er, 'as we..' was supposed to be 'as well'

Since $menu_prefix is now not being used.

I'll update with you changes plus my additional review...we need some documentation before I should commit this, so I'll do some of that today too.

Okay!

I did a LOT of smoothing out of strings.

I changed the form callback to take (&$form, &$form_state) -- that will affect your message module UI, so you will definitely have to update.

I added configurable redirects to the edit/add/clone/import pages that default to returning to the list.

I created documentation for export-ui and documented the new export stuff. I did *not* document auto-submit.js in the advanced help. That probably needs to be done.

This is now committed to CVS. We can file any changes to this as new patches. I assume we'll need to make some adjustments as we get some UIs actually using it, but it'll be easier to make small changes like this than maintaining the big patch like we have been this week.

Thanks so much for getting this rolling, Amitaibu. This was a lot easier to do having a solid base to work from!

Ok, here's Panels Stylizer fully tricked out to use this. A couple bits got complex. That's ok, stylizer has an odd UI because of the split with stylizer.inc

http://drupal.org/node/824530