We added $form and $form_state to the action form in order to support rules actions.
Of course, that killed the possibility to use VBO actions outside of VBO. Not a big waste, considering it isn't exactly pretty anyway.

I'm guessing there should be a drupal.action.inc same as there is rules.action.inc, as well as a generic way of getting the available actions from the individual sources.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bojanz’s picture

We need this before alpha3, it's a BC break so better to do it as early as possible.

And actually, the real problem here was trying to force the Rules integration into the same core-actions mold.
So we could continue to declare VBO actions as core actions (just in case someone finds that useful), but handle the separation between core action / rules action better in code.

bojanz’s picture

Title: Refactor the way action sources are handled (and stop declaring VBO actions as core actions) » Refactor the way action sources are handled.

I've committed a quickfix: http://drupalcode.org/project/views_bulk_operations.git/commitdiff/cdd6d...

It makes VBO actions work correctly outside VBO again. Also removes the get_custom_actions() code that's no longer needed in D7 (since actions_list() invokes hook_actions_info() without touching the DB).

So, now it's no longer broken (which makes it less of a priority), just ugly.

bojanz’s picture

Status: Active » Needs review
FileSize
46.15 KB

This is broken, incomplete, and wrong at places, but it shows where I'm going.

bojanz’s picture

FileSize
56.86 KB

If anyone is following, here is an updated (still broken & untested) version. Makes the codebase much cleaner and easier to understand.
With the rate this is growing, I will have to split it into multiple commits, otherwise the diff will be useless.

bojanz’s picture

FileSize
54.14 KB

Okay, committed some of the cleanups:
http://drupalcode.org/project/views_bulk_operations.git/commitdiff/1125d...
This removed 300 lines from the diff.

Here's the remaining part. Works in my limited testing.

Summary:
1) Operation types are ctools plugins.
2) An operation is now an object. All related code is in one class, instead of being all over the place.
3) Much less throwing of data around, and confusion ($params VS $options VS $callback_arguments VS whatever)

infojunkie’s picture

Great stuff - I'd been wanting to do that for some time!

bojanz’s picture

Title: Refactor the way action sources are handled. » Refactor operation types
Status: Needs review » Fixed

Did some more work and committed:
http://drupalcode.org/project/views_bulk_operations.git/commitdiff/5728c...

Giant step forward!

Status: Fixed » Closed (fixed)

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

bojanz’s picture

Status: Closed (fixed) » Needs review
FileSize
39.63 KB

Time for a sequel!

Last time I did this I forgot to abstract out the admin options form, so we had actions api specific code in the VBO views field handler, which is bad since we might want to embed some rules settings there at some point as well.
Now fixed. Basically all operation-related options are now handled by the operation itself, leaving the VBO views field handler very thin, as it should be.
This change takes up most of the patch, and is pretty straightforward.

Also, performance improvements! As das-peter pointed out in #1319504: Populate operations only if really needed, VBO currently loads all the operations all the time. Bad if you have a lot of Rules (you selected 4 operations out of 200 for the VBO form, but once the user visits the form, VBO loads all 200 of them, for no good reason).
Changed that so we only load all operations when showing the VBO views field settings (where the operations are selected).
For everything else, only the needed operations are actually loaded.
Due to limitations of the core Action API, we still need to load all advanced actions from the DB and invoke hook_action_info() if there is an action used,
but for Rules the situation is much better since each operation (rules_component) can be loaded separately.

Also, all operation_ids are now prefixed with the operation name (action::system_send_mail_action and rules_component::commerce_calculate_tax for example), to prevent possible collisions. There is update code in the VBO view field handler that prefixes all operations, but that requires loading all operations, so if you want the performance boost described above, you will need to resave your VBO views (I will re-export the views shipped with VBO prior to commit).

Also, this patch fixes actions_permissions for advanced actions, that was broken.

TODO:
1) Need to test the Drush integration, might be broken now, I just updated the code blindly.
2) This code

   public function formSubmit($form, &$form_state) {
+    // Some modules (including this one) place their action callbacks
+    // into separate files. At this point those files might no longer be
+    // included due to a page reload, so we call actions_list() to trigger
+    // inclusion. The same thing is done by actions_do() on execute.
+    actions_list();
+

is a bit of a hack, needed because we save the operation in form_state between form steps (config, confirmation / validate, submit...).
Might be a better idea to forget about keeping the operation in form state, and explicitly loading it instead when needed.
Not sure right now which way is better.

EDIT: Okay, yeah, I'm pretty sure the current way is the way to go. We should keep the operation in form state so that it's the same object, persisted through the various validate / submit stages until execution.
EDIT2: Drush works.

bojanz’s picture

Status: Needs review » Fixed

And committed: http://drupalcode.org/project/views_bulk_operations.git/commitdiff/db38f....
It's a big patch, and I've gone many times through it, so I thought better to get it in, and address any issues or criticisms through followups.
So, feel free to speak your mind.

das-peter’s picture

Status: Fixed » Needs review

For now I've just to say awesome! :)

bojanz’s picture

Status: Needs review » Fixed

Great :)

Status: Fixed » Closed (fixed)

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