Image toolkits should not have to be copied into the /includes directory

We should coordinate a little this patch with the broader issue: http://drupal.org/node/259103

minor code suggestion:

instead of doing this:

$path = drupal_get_path('module', $info['name']) . '/' .

maybe you should just select the filename in the query too? since if you trace through what drupal_get_path() is doing, it's just calling dirname() on the filename field from {system}.

Of course, maybe it's beeter to continue to use the API in case that changes.

I don't know (yet) how often _get_toolkits_from_module_info() would get called but it seems like a hook-based approach could yield better performance. Right now, it takes an SQL query and a lot of unserialization -- both are expensive. If _get_toolkits_from_module_info() gets called on every page view, that would be a problem IMO. I guess there could exist situations where _get_toolkits_from_module_info() gets called a lot ... ?

@Dries - currently it is not called often. I discussed with Paul the alternatives between a module hook and the .info file. My suggestion to go this way was based on the idea that we may be moving towards modules that don't require a .module file. http://drupal.org/node/259412 So, I'm not sure what the tradeoff is between load/parse a file vs. this? I guess if you are running with an opcode cache the module hook would be faster?

@paul.lovick - yes, I agree that this problem will need to be solved for all toolkit/framework types. Probably it would make sense to get this patch in and then consider subsequently the question of standardizing the approach used by toolkits/frameworks

I agree that the hook-based approach is better.

I'd be happy to remove the "copy your toolkit into ./includes" mechanism. Is there a good reason to maintain backward compatibility? We can probably count the number of image toolkits on two fingers so I recommend that we try to clean this up as much as we can (if possible). Cleaning things up makes it easier to explain/document in books, workshops, etc -- and will make things a tad faster.

I am about 70% of the way through writing an RFC on a pluggable framework for D7. It will be posted to the planet hopefully later tonight. The image system is one of the subsystems I consider a target. :-)

@Cell - look forward to reading it, though I think a simple re-write with procedural code is going to be a lot less pain than other options...

@Crell, I agree with pwolanin that it is going to be pretty hard to beat this patch in terms of simplicity.

@paul, associating the GD image toolkit with the system module _might_ be a good idea. There are other special cases in system.module. It's worth thinking about some more.

@Crell, Paul, pwolanin: I think we should go ahead with Paul's patch, or a simplification of that. Let's not block this by creating dependencies.

@Crell: propose the pluggable framework as a separate patch on top of this one. It will put us in a better position to compare both.

@Dries: RFC has been posted to the planet and dev list.

We certainly could go forward with this patch for now and convert later when/if it makes sense to. I just wonder if that will be more work in the long run, and if that's worth it. I will not block such an effort, however.

This seems pretty similar to #140932: Improve image-api: Image toolkits should be implemented as modules.. The change would allow you to create an imagemagick module that just implements the hook and points to its .inc file. Then the toolkit could implement hook_requirements and make use of the update status notifications.

That said I think I'd rather have all the toolkits as modules so GD isn't a special case.

One minor comment - looks like you could be using module_invoke_all to simplify this code:

+function _get_toolkits_from_modules() {
+  $output = array();
+  foreach (module_implements('image_toolkits', TRUE) as $module) {
+    $toolkits = module_invoke($module, "image_toolkits");
+    for ($i = 0; isset($toolkits[$i]); $i++) {
+      $path = drupal_get_path('module', $module) . '/' . $toolkits[$i];
+      $file = pathinfo($path);
+      $output[$path] = new stdClass();
+      $output[$path]->filename = $path;
+      $output[$path]->basename = $file['basename'];
+      $output[$path]->name = $file['filename'];
+    }
+  }
+  return $output;
+}

http://api.drupal.org/api/function/module_invoke_all/7

like:

$toolkits = module_invoke_all("image_toolkits");
foreach ($toolkits as $kit) {
  ...
}

That's something I ran into when adding the page split functionality to the D6 menu system. module_invoke_all() is only useful if you don't care what modules returned what. If you do, you have to do it manually. Sounds like that's a worthwhile feature addition of some kind. It's also a separate issue from this patch, so it's probably best to go with the manual method for now and refactor later when/if we have a module_invoke_all_with_name() of some kind. (That would be a bad implementation, but you get the idea.)

I think this can actually be simplified - we can indeed use module_invoke_all(), unless I'm missing something, since drupal_function_exists() will find the include file based on the registry.

One minor question - should we hack the repository to retain the revision history for image.gd.inc?

I'm against hacking the repository. Every time we do that, it makes it harder to ever do anything but CVS, and our specific customized CVS repository in particular. If someone is really tracing back the history of that file (how often does that happen over a long term?), they'll note that it disappeared and reappeared somewhere else between two given revisions and know to start looking there. No big deal, as long as there are no changes to the file at the same time (thus eliminating the need to diff between the two files).

there are no changes - it's a simple move.

Committed to CVS HEAD. I didn't move the file because that would make it impossible to check out a DRUPAL-5 or DRUPAL-6 branch, AFAIK.

Tried this with CVS update and got this message:

warning: require_once(./includes/image.gd.inc) [function.require-once]: failed to open stream: No such file or directory in /Sites/drupal/includes/bootstrap.inc on line 1241.

I got this error while trying to load:
http://devel.drupal.cvs/admin/settings/image-toolkit

@xmacinfo - you need to force a registry rebuild - go to admin/settings/performance and hit the "Clear cached data" button at the bottom

Done. Thanks.

hook_image_toolkits should perhaps be mentioned in contributions/docs/developer/hooks/core.php. That way, it will be documented on api.drupal.org. A hook is only useful if module developers can find it...

Something like this should do it:

/** 
* Define image toolkits provided by this module.
*
* @return
*   An array of image toolkit names.
*/
function hook_image_toolkits() {
  return array('gd');
}

@Arancaytar - looks like you have CVS access. It would be great if you could commit any updates you see as needed - anyone with CVS can alter the developer docs. http://drupal.org/cvs?commit=126084

pwolanin: Oh. I'm afraid I didn't quite realize how the contrib repository works - I thought I only had commit access to the module I'm maintaining. Thanks for the info!

The patch adds a line to the change log file.

That line should probably be descriptive, like the other lines in the changelog.

The change is already in, so you need to say "can now be added by modules without being copied to includes/".

What about this one?

The English is a little off. Maybe something like:

Image toolkits can be added by modules and no longer need to be copied into the includes directory to be used.

Ok, this patch includes Susurrus' suggestion (sorry, English is not my native language)

Yeah, that would be it.

Committed to CVS HEAD. Thanks.