Move image actions into new hook_imagecache_action()

subscribe

Good stuff gordon, is there any progress on a watermarking feature?

I think it would be good to have default actions factored out as .inc files as well. So people could write (and contribute) .inc files as well as add actions through their cool modules using hook_imagecache_actions().

+1 for the functionality

any chance of a re-roll for imagecache-5.x-1.x-dev version so i can test?

// $Id: imagecache.module,v 1.19.2.27 2007/04/19 22:58:44 quicksketch Exp $

I tried something similar two weeks ago, but was a bit slow about posting it, pending some work on my demo sites.
I've implimented a handful of new actions, demonstrated in this page
http://203.97.100.52:8001/node/101
(that dev URL may vanish/change sometime next month)

Text actions, Imagefilter Colour processing actions, Alpha transparancy actions, and most powerfully, SVG pipelining (really cool, if you have SVG support on your server)
Watermarking is simple in this.
The code changes are pretty big, but I hope logical. Actually simplifies the old core code in places.

Rather than impliment each new action as a module, I've opted to just scan the contents of an imagecache/plugins/*.inc directory and deduced available actions from there, using hook_ type function names.

I'll attach a full (slightly messy) tarball, although I can try rolling a patch.
I'll have a look at gordons hook_imagecache_action API to see if I can/should rename my functions to fit with that instead.
TODO, a bit more error handling, although some problems do now give that 'custom broken image' we wanted.

Have a look at the demo page!

First review of gordons approach -
It's certainly a bit more 'Drupally' with it's use of module_implements() and the way it packs everything into imagecache_imagecache_action($op, $image, $delta)

I didn't try it exactly like that as I figured the individual actions (that I could imagine) were probably not big enough to warrant a full module each, and sorta wanted the development to be kept within the module rather than branching into more add-ons to manage.

I'm personally not a big fan of catch-all functions that then use the $op to switch inside themselves to decide what to do, but prior art within Drupal core certainly tends that way :) . I know block.module, filter.module etc do that all the time, but ...

I guess my current
imagecache_action_hook()
imagecache_action_hook_info()
imagecache_action_hook_form()
plugin API could be scrunched into that method instead.
It's doing exactly the same thing...

I see that running a diff on my code (attached) is a bit hard to read. Mostly because I also ran the module through the code conventions checker and repaired a dozen trivial issues. Also added lots of comments (and a bit of scaffolding)
However, I also added a 'preview' function and adapted the UI forms a bit. Sorry, many changes in one. :(

I don't want to compete with gordons work, as I think our additions are totally compatable. But I'd like input on whether it's better to abstract the actions out into their own inc files (my approach) or require each new action(s) to be provided by a new module.

Well, all patches provided here are good but I wonder what is a destiny of transformer.module? It looks like an API to perform file transformation and there are already similar hooks and several image transformation pluggins developed. Honestly speaking, I thought transformer.module will take care of image transformations for imagecache.module. Now I see people are working on patching imagecache.module directly.

I'm asking because I've just came across with a necessity to develop my own transformation and I don't know which approach will be chosen for the future.

Does anybody have an idea?

I will consider one of these patches of imagecache v2 if anyone is still willing to work on them. They need to be updated to support the scale and crop patch that was committed to head. I will probably not integrate imagecache with transformer until v3, if it is not deprecated in the future.

I've just taken a fresh look at this and I think all module-defined imagecache trasformations (er, actions) and preset definition can be reduced to two hooks:

hook_imagecache_actions() {
  $actions['action_name'] = array(
    'description' => t(''),
    'form callback' => 'callback_function_name',
    'process callback' => 'processing_callback_function_name',
  );
  return $actions;
}

hook_imagecache_presets() {
  $presets['preset_name'] = array(
    'description' => t(''),
    'actions' => array(
      0 => array(
        'action' => 'action_name',
        'weight' => -10,
        'data' => array(
        ),
      ),
      1 => array(
        'action' => 'action_name',
        'weight' => -10,
        'data' => array(
        ),
      ),
    );
  );
  return $presets;
}

These hook names might be problematic moving forward to Drupal 6, though. Does anyone have thoughts on this? I'd be willing to roll it if the structure above looks sensible.

I think you only really need one hook (hook_imagecache_action), not two. Though the idea of module provided presets opens up a world of possibilities. It might also turn out to be a lot of extra work, say over-ridding module defined presets, in which case the hook would be called hook_imagecache__default_presets() I guess. I'd love to see this implemented as hook_imagecache_action for now, then roll the preset patch as a separate issue.

Here's the action hook functionality. I think there's still a solid case to be made for preset hooks, and I think it would be pretty easy as well. (Allowing users to define presets in the DB, then array_merge() in hook-added ones... That would allow user presets to override module-exposed presets pretty easily...)

In any case, though, here's the patch. I included a rough 'image rotation' action as well, to demonstrate how easy it is to stick in a new action with the new system. Hooray!

Gorgeous code Eaton. I like the conditional inclusion of actions only when needed. In a module that could potentially have 20 some actions (plus additional contrib versions), this is a great help.

Some thoughts though... I think I'd like to see each action as a separate .inc file, rather than clumped in one big file. hook_imagecache_actions could live in the module directly, though keeping it separate could make it easier for contrib patches to be rolled, no hunting through the rest of imagecache.module to find it.

I'm not a fan of the dynamic includes. We have the module system for that already. I'd rather see something similar to the transformer module.

http://cvs.drupal.org/viewvc.py/drupal/contributions/modules/transformer...

Where there is an imagecache_actions hook that modules can implement that returns meta data about the available actions, then supplies the necessary functions. Imagecache would only need to implement the basename and title from the transformer spec, without the additional mime, extension, and queue properties.

I really appreciate the work, I just don't want the two modules to diverge too greatly. I've already implemented a functioning pluggable action system for transformer. The model for transformer follows the quicktime plugin model using a register function and then building off of a registered basename for the remaining callbacks.

I went about creating my own version of this before seeing this thread/issue.

http://drupal.org/node/186285

The patch offered here would work just as well for my purposes. How close are we to moving this into HEAD? I have a project that could use this and my rounded-corners module immediately.

Other than being able to specify a file, the differences in hooks aren't too far apart. Eaton's code adds the following structural changes:

   'file' => 'imagecache_actions.inc',
   'form callback' => 'imagecache_resize_form',
   'process callback' => 'imagecache_resize_image',

form and process callbacks could be removed entirely if we wanted, and use the function-pattern method in transformer (or conversely add the callback properties to transformer).

The file property I'd loathe to see go though. Even with the module system, every module is loaded on every page view. If imagecache (or transformer) grew to 20 different actions, it'd be loading those every page view when they're only needed for the admin area and during image manipulations. With Drupal 6, we'll see a lot more conditionally included code in the menu system. I think this is keeping one step ahead of a trend.

Knowing the concept was taken from the menu system in D6 though, I think we'll want to put t() functions around the title and description. They're left out of the menu system because of their dynamic nature, which isn't the case with our actions.

   'name' => t('Resize'),
   'description' => t('Resize an image maintaining the original aspect-ratio (only one value necessary).'),

I kind of put 'file' it in the pile of premature optimization. I reworked imagecache so you can call imagecache_build_derivative or _imagecache_cache from anywhere. The conditional include makes this more difficult.

We're talking about API functions here. The dynamic includes in the menu were meant to avoid loading large amounts of administrative form definitions on each page load. I'm not all in favor of hiding necessary functional code in a dynamic include. I wouldn't mind sticking the forms in seperate includes, but that gets hairy when we have to include everything at runtime, or figure out how to get our includes into hook_menu.

I've take a closer look at eaton's patch...

@eaton: talk to your editor about all the white space changes it likes to make.. its kind of annoying. when reviewing patches ;)

The approach doesn't look so bad. Why the additional pieces of data we have to track for the process callback and form callback. I think we are safe enforcing a naming convention here. I think we can keep a 'file' to be array_merged in hook_menu, but I vote we only put the form definitions and validation in those dynamic includes.

I'd like to stick this in HEAD and get 2.x ready for release.

Eaton's already done the dynamic includes in this patch. When imagecache_cache is called, it runs _imagecache_apply_action();

function _imagecache_apply_action($action, $image) {
  $actions = imagecache_action_data();
  if ($definition = $actions[$action['data']['function']]) {
    if (!empty($definition['include file'])) {
      require_once($definition['include file']);
    }
    call_user_func($definition['process callback'], $action, $image);
  }
}

Meaning _imagecache_apply_action (almost the same functionality as imagecache_build_derivative in HEAD), does the loading of necessary files. It can still be called from anywhere just as easily. If the patch were changed to use the same function setup with imagecache_build_derivative, just adding the file support, would that be any more palatable?

Oh doh, you beat me to my own rebuttal. :)

of course it would be more palatable. I'd just rather leave off on the dynamic includes until D6, then leverage hook_menu for that instead of reinventing the wheel. It would be even more palatable if the dynamic includes were only for form stuff right now instead of in apply action.

then the rest of the admin form stuff can go there for D6 instead of in an imagecache_admin module.

of course it would be more palatable. I'd just rather leave off on the dynamic includes until D6, then leverage hook_menu for that instead of reinventing the wheel. It would be even more palatable if the dynamic includes were only for form stuff right now instead of in apply action.

Well, the menu related stuff won't actually give us anything in D6 other than the forms themselves. And since the snippets of form 'stuff' are relatively minor I don't know whether it would even be worth pursuing. I had thought that the ability to include everything -- the form and the implementation of the graphical code -- in a single self-contained .inc file would be the benefit.

The current code makes use of the include files transparently when an imagecache action is called -- that's why the information on where the file is stored gets cached in the action metadata. Callers who want to process images don't need to know whether the actions are in external files, modules, or -- heck -- themes. Or am I missing something? They're also an optional mechanism that doesn't have to be used -- it could be really useful for some of the more code-heavy actions, though, like drop shadowing, watermarking, and so on.

In any case, I'll try to get some of the suggestions implemented here and roll a fresh version of the patch.

subscribe

so is #15 the thing to test this week?
I'd like to chip in and have a go. I've got an SVG transform, plus a few configurable colour filters (for greyscale,sepia etc) that I'm using as transform actions.

I really like the actions to be their own include file.

Can we find a way to pass in extra arbitrary arguments? I've used my experiments in imagecache to replicate banner.module functionality by passing in strings to one of my action filters.

Currently it assumes that we always start with a source image, then apply changes to it. I found that a bit restrictive when building the source image from scratch. I guess what I want may be out-of-scope for what imagecache is intended to do, so possibly a sister module that allows these extra complicated settings, but has access to the imagecache transform libraries is what I should do...

+1 for this and subscribe....

@eaton, have you had a chance to revisit or shall I?

@dman, find another issue for your scope creep. I like the idea, but passing arguments in the url in this fashion exposes a denial of service attack... Image processing can consume a lot more resources than just answering a web request on a shared server... note the original imagecache had urls like imagecache/scale/320/200/crop/160/160/80/80/file/filepath/filename.ext I found I could bring hefty servers to their knees pretty quickly with a shell script and wget.. so I setup the preset system. please expand on the idea in another issue.

Fair enough. I hadn't considered DOS. I guess it would need throttle integration or something.

Please move ahead. I just want to see the new stable version out and about.

Yeah, I hadn't had a chance to tackle any of the additional changes since I rolled that patch. I still think the ability to stick things in external files is a big boon, but just defining things in hooks will be enough of a boost that I think it's worth doing even if the external file support doesn't get in..

I dug into this tonight.. it has been committed to imagecache head... I also implemented backwards compatibility for the old scale preset.. There are now 3 scale presets ack! Deprecated Scale, Scale, and Scale and Crop.... Warning rotate is busted

@eaton you wanna fix rotate so it's not busted? If not I'll remove it...