Action includes always loaded using slow file_scan_directory(), even if VBO not used on page [#1235736]

Action includes always get loaded using the slow file_scan_directory() in views_bulk_operations_init(), even if VBO is not used on current page.
With views_bulk_operations_action_info() and views_bulk_operations_theme() loading the files (which already seems too much), why is it needed to do another pass loading the includes on hook_init()?

In any case there should be a function with static cache to keep track of whether each file is loaded or not before trying for an expensive file_scan_directory() call.

Comment	File	Size	Author
#8	vbo_load_actions_init-1235736-8.patch	1.42 KB	hefox
#8
#5	1235736.patch	3.18 KB	bojanz
#5
#1	patch_commit_983966f239ec.patch	2.99 KB	moonray
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

moonray CreditAttribution: moonray commented 1 August 2011 at 14:33

Status:

Active

» Needs review

File	Size
patch_commit_983966f239ec.patch	2.99 KB

And here's a patch.

If there is a god reason to keep loading the action includes in hook_init, let me know. It should probably be documented in code, at that point.

Comment #2

bojanz CreditAttribution: bojanz commented 1 August 2011 at 21:47

Status:

Needs review

» Needs work

What we are doing right now is insane, and your patch is almost perfect in fixing it.

There is no need to do the including in hook_init(). Doing it in our implementation of hook_action_info() is enough because that hook is invoked both when an action is executed (by actions_do()) and when actions are listed in VBO / views admin (because views_bulk_operations_operation_action_list() calls actions_list() which invokes hook_action_info).

The only thing I'd add to your patch is caching (indefinitely) the output of file_scan_directory(). Even with the patch it is guaranteed that file_scan_directory() will be invoked once per (vbo / action execution) request, all in order to scan a directory whose list of files doesn't really change. Having to do a cache clear after adding a new action file (which I guess will happen three to four times during the whole D7 cycle) is perfectly fine.

Comment #3

moonray CreditAttribution: moonray commented 2 August 2011 at 08:47

I ran some (admittedly local) speed tests, and DB reading seemed to be slower than reading the file dir. Of course, it's a small dir.
I had written the caching code, but stripped it when I saw that it was slower.

Thoughts?

Comment #4

bojanz CreditAttribution: bojanz commented 2 August 2011 at 09:34

I spoke to catch about this. His conclusion:
Hardcoding the list of files > variable_get > cache > file_scan_directory().

And I'm fine with just hardcoding the list in an array at the top of views_bulk_operations_load_includes() (though we might want to rename it to views_bulk_operations_load_actions or views_bulk_operations_load_action_includes), along with a comment explaining the situation.