Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Working with image toolkits it seems it would be easier to implement toolkits as modules, and leverage the existing module system for dynamic includes.
replacing the file_scan_directory with a module_invoke_all in image_get_available_toolkits.
updating the gd_... namespace to image_gd namespace
adding an image_check_settings function that call the toolkit specific check settings function.
This would allow people to implement custom image toolkits with customized crop and scale functions for their sites with core image.inc hacks.
Comment | File | Size | Author |
---|---|---|---|
#59 | imageapi_140932.patch | 23.1 KB | drewish |
#59 | imageapi_gd_140932.patch | 4.68 KB | drewish |
#54 | imageapi.patch | 17.13 KB | Stefan Nagtegaal |
#47 | 140932_1.patch | 19.78 KB | drewish |
#42 | imagemagick_module.patch | 5.53 KB | drewish |
Comments
Comment #1
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedDopry, are you planning to work on this?
*subscribe*
Comment #2
dopry CreditAttribution: dopry commentedunless you want to...
Comment #3
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedI would love to, but I have no idea what would be the best way to get this in..
If you could make some concept patch. I'll build further upon that.
Comment #4
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedDocumentation about the mentioned function which should be used:
[14:11] Steef: module_invoke?
[14:11] Druplicon: usage module_invoke($module, $hook, $args) - see http://api.drupal.org/api/head/function/module_invoke
[14:11] Steef: module_invoke_all?
[14:11] Druplicon: module_invoke_all is usage module_invoke_all() - see http://api.drupal.org/api/head/function/module_invoke_all
[14:11] Steef: file_scan_directory?
[14:11] Druplicon: file_scan_directory is usage file_scan_directory($dir, $mask, $nomask = array('.', '..', 'CVS'), $callback = 0, $recurse = TRUE, $key = 'filename', $min_depth = 0, $depth = 0) - see http://api.drupal.org/api/head/function/file_scan_directory
Comment #5
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedOkay, I'm on this and it's pretty much working..
A lot of debug code to remove still, but we are getting there!
I'll post some screenshots for anyone who's interested in this..
---
Attached screenshots is the Toolkit selection page, as it is currently implemented. Only now it's using fieldsets to be consistent with the rest of drupal.. It directly shows the settings for the current selected image toolkit..
Comment #6
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedHere we choose to use imagemagick as our toolkit (which is a module now!).
The imagemagick.module isn't having settings (yet)..
Comment #7
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedRemember we have set imagemagick as our default toolkit?
When we do not disable the module, and simply delete it from our modules, the image api automatically falls back to GD support. (and shows it's settings directly!)
Comment #8
drewish CreditAttribution: drewish commentedhow about a patch?
Comment #9
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedWell, here we go..
This patch does the following:
- toolkits are modules now;
- the idea is to have a gd.module in core, which is enabled by default;
- people could use imagemagick.module or netpbm.module as a replcement for the gd.module. They can simply put the new modules is 'sites/modules/$toolkit/$toolkit.module', no fiddling around in includes/ anymore;
- i removed all the gd specific code from image.inc to gd.module, which makes image.inc a reall API and is much cleaner to extend;
- I removed a lot of code which wasn't used, or did not make sense;
- I made the administration part (where you select your toolkit and set your preferences for it) more consistent with the rest of Drupal;
What still needs to be done in this patch;
- upgrade path;
- find out where our default enabled modules get set;
What needs to be done (in separate followup patches);
- the gd.module could use some cleanup as I simply copied all the gd specific functions to the gd.module. I did make some notes in there to make sure I/we don't forget about it;
- write the imagemagick.module, based on the image.imagemagick.inc;
How to test?
- simply apply the patch to your drupal install.
- download the gd.module and gd.info file (from the next issue folowup), and put it in the modules/gd directory or sites/(all|default)/modules/gd;
- goto 'Administer' >> 'Site building' >> 'Modules' and enable it in the 'Image processing toolkits' fieldset;
- Try uploading and resizing images!
Comment #10
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedAttached is the gd.info file which should live under 'modules/gd' or 'sites/(all|default)/modules/gd'.
NOTE: Rename gd.info.txt to gd.info
Comment #11
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedAttached is the gd.module file which should live under 'modules/gd' or 'sites/(all|default)/modules/gd'.
NOTE: Rename gd.module.txt to gd.module
Comment #12
dikini CreditAttribution: dikini commentedThe approach makes includes/ better and slimmer. Makes advanced theming more flexible.
Comment #13
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedNew patch, which adds:
- the upgrade path;
- and made gd.module enabled by default with new installations;
You still have to manually download and rename the gd.info and gd.module files.
Place them inside 'modules/gd'.
(untill I found out how I can do that all automagically in this patch)
Comment #14
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedThere was a bug inside the gd.module, which didnt allow you to resize/crop images.
Comment #15
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedTo test:
- grab the patch, place it inside drupal root and apply it using: 'patch -p0 imageapi.patch';
- make a new directory under 'modules/' called 'gd';
- grab the gd.info and the gd.module files, and place them in the new created 'gd' directory;
- rename the files to gd.info and gd.module!
That's it! have fun!
Comment #16
drewish CreditAttribution: drewish commentedhow's this look? it uses a hook_image_toolkit. it cleans up a lot of the gd.module stuff.
Comment #17
chx CreditAttribution: chx commentedCode style and doxygen fixes, no code changes. I am pretty OK with where this goes, though not requiring gd module would be great...
Comment #18
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedAs Karoly stated, the gd.module shouldn't be enabled by default and not required during install.
So, updated patch which addresses:
- made gd2.module optional;
- added gd2.install which checks for a PHP installation with gd compiled;
- made system_image_toolkit_settings() smarter so it doesn't display unnecessary options;
Comment #19
dikini CreditAttribution: dikini commentedIt is much cleaner now. I like the api - toolkit split. The old behaviour is preserved and the gd.module is optional.
Comment #20
drewish CreditAttribution: drewish commentedStefan Nagtegaal, I don't know what it is but all your patches that add files end up breaking my patch program (both on Windows and FreeBSD):
I tried chx's patch and it worked fine. Eyeballing the two doesn't make it clear what the difference is.
The current patch looks good, the only thing I'm not happy with is the way we get the settings. Having a _setting() hook works but is a bit wonky. We could just let the image toolkit modules form alter the /admin/settings/image-toolkit form... I'm sort of thinking it might be better for the toolkits to each have a menu hook and put themselves on tabs under /admin/settings/image-toolkit, i.e. /admin/settings/image-toolkit/gd and /admin/settings/image-toolkit/imagemagick.
Thoughts?
Comment #21
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedI'm not sure about it.
The tabs are used for 'tasks', and a toolkit is not a task, configuring it is.. But looking through drupal we would create another inconsistency.
How about this:
We have an overview tab (called "overview") at 'admin/settings/image-toolkit' that displays a table:
[Toolkit] [Default] [Operations]
You could pick the default toolkit, with using radio buttons when. Operations has a 'configure'-link that brings you to the toolkits settings.
And we would have a configure tab (called "configure"), which has $toolkits_available as secondary tabs.
Advantages:
- it allows you to setup a new toolkit once the old one is still in use;
- it's consistent with the rest of drupals interface (filter/block/system/locale.modules all implement this kind of layout.)
Just some thoughts...
Comment #22
drewish CreditAttribution: drewish commentedfor reference here's an imagemagic toolkit using this api
Comment #23
drewish CreditAttribution: drewish commentedso i moved the settings into a hook_form_alter and it work pretty well but it gives you validation errors if you submit the form again after changing the toolkit selection. i'm sure it's something simple i'm missing. i'll try to get one of the fapi gurus to look at it.
i fixed one bug in the rotate, there was a bg_color setting that wasn't being passed in correct so i added that to image_gd_rotate() and got it working for both gd and imagemagick.
i included the imagemagick toolkit in this patch. i don't really expect it to get into core but it's handy for testing.
Comment #24
drewish CreditAttribution: drewish commentedI talked to Steef on IRC and he convinced me that the way I was doing the settings wasn't good. I tried using the plan he outlined with "List" being the default tab under "Image Toolkit" and "Configure" with sub tabs for each toolkit.
I defined a hook_menu for each toolkit to add itself in below configure and make itself the default tab if it was the selected toolkit. That worked well until you tried to visit the "Configure" tab. It needs to have the current toolkit's callback info but since the modules define their own settings (heck, they may not even have any settings) the system.module can't put them into the menu.
My fall back plan was to just to have each toolkit define its own settings page: admin/settings/gd or admin/settings/imagemagick. It was the simplest way of doing it.
Comment #25
drewish CreditAttribution: drewish commentedOkay, so if we require every image toolkit to have a modulename_settings_form() function that returns a form, Steef's menus work. For the time being I've left the hook_menus in the imagetoolkit modules.
I also did a bunch of cleanup on the PHPDocs so that Dries wouldn't have any low hanging fruit when reviewing the patch ;)
Comment #26
jvandyk CreditAttribution: jvandyk commentedAfter creating a new site I went to Administer - Site configuration - Image toolkit and was told "There are no image toolkit modules enabled." Either the GD module should be automatically enabled or there needs to be a link here to the modules page.
So I enable the GD module and return to Administer - Site configuration - Image toolkit. Well, there's a single selected radio button for "Built-in GD2 image processing support." If I can't change anything, the page is unneeded. Especially since, after enabling the GD module I see Administer - Site configuration - GD image toolkit.
Maybe a better approach is to have the GD and Imagemagick modules check at install whether another toolkit is enabled, and only allow one enabled at a time.
imagemagick.module found my installation and ran -version successfully.
Removed _imagemagick_check_path() just use is_file() directly instead.
Cleaned up comments and variable initialization.
Comment #27
drewish CreditAttribution: drewish commentedjvandyk, your patch lost the new files. so i just made the changes you described to my copy.
i removed the hook_menu that you weren't into.
cleaned up the UI when there are 0 and 1 toolkits enabled.
also replaced _imagemagick_check_path() with is_file().
Comment #28
webchickI should probably withold comment until I actually test this patch, but most of John's statements have me thinking, "Um. Do modules really make sense for this?"
If you can only have one or the other installed at a time (gd, imagemagick, or netpbm) it seems that this fits much better into our database-XX.inc motif than it does a module motif. I asked drewish about this, and he brought up the point that .inc files can't have hook_menu for settings pages, or hook_requirements for checking run-time/install-time requirements, and they wouldn't work with update status module, for instance. But you can work around the lack of hooks with system.module (which I assume is how this was done in the past), and you don't need update status module integration if you add image-imagemagick.inc to core (and maybe image-netpbm.inc). That seems like a much bigger gain with far fewer UI headaches, and I assume that can be done relatively easily from the imagemagick.module?
Comment #29
webchickAnd what happened to http://drupal.org/node/148346 ? :( That was such a nice, simple approach. :(
Comment #30
drewish CreditAttribution: drewish commentedwebchick, if we could get imagemagick (and possibly a netpbm) into core i'd be happy to move it back to the .inc model (as in #148346). if it doesn't look like that would be committed to core then i think moving it to modules is a much more contrib friendly way to go.
Comment #31
drewish CreditAttribution: drewish commentedimage_get_available_toolkits() now parses the .info file for module info. the only bug at this point is the config menu after you disable or unselect a toolkit.
Comment #32
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedDrewish, what is the way to replicate the bug?
I'm reviewing/checking your patch atm..
Comment #33
drewish CreditAttribution: drewish commentedgoto the configure tab after changing the selected toolkit, or maybe disabling the module, i can't remember which.
Comment #34
drewish CreditAttribution: drewish commentedwhat it boils down to is are toolkits for imagemagick and netpbm going to be commited to core? if not, then it needs to be much easier for image toolkits to be in contrib.
right now users have to copy a .inc file from the image module into their includes directory. once they've done this there's no obvious way to tell if a new .inc file has been released that fixes a bug. if the toolkit was a module the user could use the update_status module to find out about updates. the toolkit could also do a better job of displaying requirements with hook_requirements.
Comment #35
dopry CreditAttribution: dopry commented@webchick:
The reasons I like the hook_toolkit approach to the image toolkits are
The attached patch makes it a little easier for module developers to leverage multiple toolkits at once.
Comment #36
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedThe patch from dopry works very well, and is very nice..
Is there anyone else who could review this patch again, and could set it RTBC?
Comment #37
hunmonk CreditAttribution: hunmonk commentedsub
Comment #38
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedOkay, I tested this quite intensive and did no come up with any problems except that doprys patch includes the settings.php changes. Rerolling the patch in a minute..
Comment #39
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commented- Removed settings.php changes;
- Added the gd module files;
Comment #40
hunmonk CreditAttribution: hunmonk commentedi can't apply the patch:
505 colossus$~/bzr/image_toolkits: patch --dry-run -p0 < ~/Desktop/image-toolkits.patch
patching file includes/image.inc
patching file modules/system/system.module
patching file modules/gd/gd.info
patching file modules/gd/gd.install
patching file modules/gd/gd.module
patch unexpectedly ends in middle of line
patch: **** malformed patch at line 709:
Comment #41
dopry CreditAttribution: dopry commentedHere is a re-rolled version of the patch.
Comment #42
drewish CreditAttribution: drewish commentedi'm attaching a patch with the imagemagick toolkit. it's useful for testing to see how the patch works with multiple toolkits.
Comment #43
drewish CreditAttribution: drewish commentedi've tested the re-roll dopry posted on #41 and it looks good to me. since it's the same thing that Stefan Nagtegaal has tested, RTBC.
Comment #44
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedIn all, this opens up tons of new possibilities for Image handling and even more for the flexibility of it.
IMO we should not ship Drupal 6 without this!
Comment #45
mdlueck CreditAttribution: mdlueck commentedI vote to get this even into an update for Drupal 5.
We host currently with 1&1, and also do development work for clients with other providers. Some interface needs to be supplied to look in an alternate path specified to locate the image toolkit binaries. Gallery does this when you enable ImageMagick, and we have Gallery correctly working with ImageMagick.
There seems to be much FUD about getting Drupal 5 to use ImageMagick. Clean install of Drupal 5 gives no indication of what tool kits Drupal even looks for... just "none found, so using GD." It seems module Image must be installed, a file copied to the /includes dir. That being done, then the system's bin directory is where the Image module expects to find the toolkit - no possibility to specify an alternate path.
Summary - Image Toolkit in Drupal 5 seems buggy at best. I would certainly hope that fixing this in Drupal 6 would not be optional!
Comment #46
drewish CreditAttribution: drewish commentedThe patch on #41 still applies (one hunk has a one line offset)...
Comment #47
drewish CreditAttribution: drewish commentedre-roll to cut down on fuzz
Comment #48
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedUnassigning...
Comment #49
wundo CreditAttribution: wundo commented* subscribing *
Comment #50
Dries CreditAttribution: Dries commentedMoving this to D7.
Comment #51
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedDries, it would be nice if you could tell us what you think about the patch so we could improve it, or keep it up with head so you can commit it as soon as D7 opens up for development.
We (and I in particular ;-)) definatly need an easier and more flexible way to handle images.
Perhaps you can shed a light over the patch and tell us your opinion.
Comment #52
Zothos CreditAttribution: Zothos commented* subscribing *
get this thing in. Would be a great improvement :)
Comment #53
sunComment #54
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedOkay, Drupal 6 is out.. Let's get this sucker in...
And an updated patch for HEAD of today, not seriously tested but should work (or very simple to fix)..
Please test the patch and leave your comments! :-)
Comment #55
drewish CreditAttribution: drewish commentedi can't seem to figure out how to get CVS to let me add the modules/gd directory to the patch so i can't really re-roll. so here's my list of changes that need to be made:
Comment #56
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commented@Drewish: Have a look at: http://drupal.org/patch/create and then especially from the header "Adding/deleting files"..
It works that way, it's exactly how I am doing it...
I would really appreciate your help on this, as time is not my best friend at the moment...
Comment #57
drewish CreditAttribution: drewish commentedStefan Nagtegaal, I did follow those directions but they didn't work. on that page it says that the technique does not work for creating directories... what are you doing differently?
Comment #58
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedHmmm... strange.. I just did hat it says...
Ehm... Can you provide separate patches for core, and attach the module file?
I'll make a patch of it so it could be reviewed/tested and (hopefully and eventually) applied..
Comment #59
drewish CreditAttribution: drewish commentedso here's a patch that addresses the issues i raised in #55. it adds requirements checking to image_get_available_toolkits() but
system_image_toolkit_settings() is a hack right now. i couldn't think of a clean way to distinguish between the toolkits that have okay requirements and those that don't.
because of my above mentioned cvs problem, i split this into two patches.
Comment #60
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commented@drewish: Have a look at how dopy implemented the imageapi_get_available_toolkits() in his imageapi.module. That one looks clean and not as hackish as this implementation..
this is getting there, I'm feeling it... :-)
Comment #61
drewish CreditAttribution: drewish commentedStefan Nagtegaal, not a very helpful comment. i just checked and it looks like the imageapi code was identical to what the function looked like before i added requirements checking on. what's "hackish" about my approach? criticism is fine as long as it's constructive.
Comment #62
Jaza CreditAttribution: Jaza commentedMoving.
Comment #63
sunThis probably conflicts with #1561832: Replace Image toolkit API with Imagine
Comment #76
smustgrave CreditAttribution: smustgrave at Mobomo commentedWonder if this is still a desired feature?