I'm forking the preset API part of #371374: Add ImageCache UI Core off into its own issue so we can focus on it and then look at getting a UI in as a separate patch. Here we'll focus on the actions, presets and generation of images.

I've put the code for this patch into a github repo because it makes collaboration much easier. If you'd like to work on the patch you can either fork the project or ask me for commit access. I'll be exporting the patch occasionally for review purposes once it's a bit more stable.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick’s picture

Ugh. Is there any possible way I can scream out in agony to please, PLEASE not use a git repo for this? Or at least not only a git repo?

We saw what happened with fields in core when an off-site repository was used. The entire community remained utterly clueless about what was going on until the patch was 500K and unreviewable. I don't want that to happen to imagecache as well.

Not using the issue queue for interim patches as things progress means locking out 99.9% of the Drupal community (including both core committers). Please make sure this issue reflects the current state of development in Git.

Anonymous’s picture

subscribe. webchick, that's a good point, we'll try to keep a current patch against HEAD on a very regular basis.

drewish’s picture

Talking with sethcohn at the Design for Drupal Camp about his work with the ComicPlaces project and it became clear that the presets and actions all need to have clear associations with the image toolkits. He's got presets that make extensive usage of ImageMagick but they're very resource intensive, for other things like scaling user photos GD would be fine.

So, each action should have a list of compatible toolkits, if none is explicitly declared then we assume it's compatible with all toolkits. Likewise each preset should have a selected toolkit (if multiple toolkits are installed) and filter the list of actions accordingly.

drewish’s picture

webchick, I'm planning on providing patches but honestly after getting used to the benefits of a distributed version control system the idea of going back to swapping patches back and forth with multiple developers sounds as great as gouging out my eyes. not being able to see what changed between patch to patch or who changed it is a huge limitation.

webchick’s picture

I don't much care about how a patch gets made. I care that it gets cross-posted to the issue periodically, along with a description of the changes between versions of the patch (and not "go look at the Git commit logs to see what we've been up to").

As long as you do that, you can make the thing in Visual SourceSafe for all I care. ;)

Anonymous’s picture

FileSize
46.01 KB

and here's the patch against HEAD right now. drewish, let me know if this is right - i generated it with:

git diff --no-prefix b586424c01659415a4f4a5ae31dea557d5bdd6e1 HEAD > image_cache.patch
Damien Tournoud’s picture

If we are on the "feature request" stage, the possibility to have diverse input and output format would be very nice. At the current stage, imagecache seems to hardcode JPEG as the output format, and I'm not sure it supports having anything that is not JPEG as input.

This is especially useful for very small thumbnails (we have a website with 27x27 thumbnails... and those are either ugly or very big as JPEG, depending on the quality you are using).

lilou’s picture

Status: Active » Needs review
drewish’s picture

Damien Tournoud, actually the format of the original image is preserved. it might make sense to have a transform format preset though.

Status: Needs review » Needs work

The last submitted patch failed testing.

Berdir’s picture

DBTNG review :)

+    while ($preset = db_fetch_array($result)) {

Just use foreach ($result as $preset), see http://drupal.org/node/310072. To change it to an array, you can use the second argument to db_query(): array(':fetch' => FETCH_ASSOC). It might be worth thinking about using objects though, as that allows for example to use by reference hooks withouth & and is what we do with nodes, users, taxonomy terms/vocabs and other things.

+  db_delete('image_actions')->condition('ipid', $preset['ipid'])->execute();

2+ chained method calls should each be on their own line, see http://drupal.org/node/310081 and http://groups.drupal.org/node/17906#comment-77003 about generic DBTNG coding style examples. (Not that there is a wrong example, see the follow-up comments.

+    $result = db_query('SELECT * FROM {image_actions} where ipid = %d ORDER BY weight ASC', $preset['ipid']);

Use named placeholders, :ipid instead of %d and then array(':ipid' => $preset['ipid']). Again, see http://drupal.org/node/310072

drewish’s picture

committed changes to github for Berdir's suggestions.

eojthebrave’s picture

subscribe. Will be back to review this in the next day or two.

mcrittenden’s picture

Subscribe.

eojthebrave’s picture

@drewish it looks like your most recent commit to the git repository included the UI parts of this as well. Unless I'm totally confused. Either way, all the UI stuff appears in the latest version in the git repository. Is this intentional?

RobLoach’s picture

I really like what you've done with this, Andrew. I can see that we use system/image for the generation process. Will this eventually be moved to [files]/image so that Drupal doesn't get the bootstrap for each image display? Other then that, the API looks pretty solid.

In hook_menu(), you might be able to turn the system/image item into something like system/image/%image_preset. Since you already have a image_preset_load() function, it seems rather fitting.

quicksketch’s picture

drewish, please reroll patches after making changes to the Git repository. When just doing a line-by-line review, I don't know what's been changed without doing a checkout from git then rolling the patch myself. This is the EXACT concern webchick had in #1. We HAVE to reroll after EVERY change for this to work or not use Git at all.

quicksketch’s picture

I confirm what eojthebrave says in #16, the latest version of the git repo actually contains parts of the UI for ImageCache and doesn't make the DBTNG changes. I'm working on a reroll against #6. I could potentially make another git repo and tell people to look at it, but that just seems detrimental to actually getting these changes reviewed.

egfrith’s picture

Issue tags: +ImageCacheInCore

Subscribing (and tagging).

eojthebrave’s picture

Documentation for system_image_actions() mentions returning an array with 3 possible attributes but then lists 7 possible attributes.

Suitale should be Suitable in the docs for system_image_preset_request()

+ // Generate a callback path for an image.

Should probably read "Generate a callback path for the image."

+/**
+ * Menu callback. Given a preset and image path, generate a derivative and
+ * serve transfer it to the requesting agent via file_transfer().
+ */

Could probably loose either the word serve or the word transfer, it does not need both.

+  if (!$preset || !isset($_GET['token'])) {
+    drupal_access_denied();
+    exit();
+  }
+
+  if ($_GET['token'] != drupal_get_token($path)) {
+    drupal_access_denied();
+    exit;
+  }

Could be combined into a single if statement. If not that, the "()" can be removed after the exit in the first one.

quicksketch’s picture

Assigned: drewish » quicksketch

I've got a large reroll in progress that updates for the new static caching mechanism and accounts for anonymous users. I'll update shortly.

plach’s picture

subscribe

quicksketch’s picture

Assigned: quicksketch » Unassigned
FileSize
40.9 KB

Here's the reroll with my latest progress.

- Makes changes eojthebrave suggests in #21
- Changes "Implementation of" to "Implements" per #472642: Remove "implementation of" nominalizations from Docblocks
- Updates to DBNTG queries
- Removes t() from around hook_schema() image preset tables
- Removes $_GET['token'] approach since that no longer works with anonymous users, see #506500: Make drupal_get_token() work without a session (anonymous users)
- Adds access control to image presets via the image_cache table (note I'd still like to have the $_GET['token'] approach, but right now that won't work since anonymous users no longer get a session).
- Makes it work for anonymous users.

Note this patch doesn't do much of anything without the UI component in #371374: Add ImageCache UI Core.

eojthebrave’s picture

Here's an updated patch with the following modifications. I kept these notes as I was reviewing it so sorry if they're a little sloppy. Still doesn't address drewish's concerns from #3.

Path applied with offset. re-roll.

Also fixes function name collision in system.install as a result of http://drupal.org/cvs?commit=232196

Removed

files[] = system.admin_image.inc

From system.info, file doesn't exist in this patch. But is part of http://drupal.org/node/371374#comment-1765058

There is reference to a preset array, e.g) "An image preset array." in a lot of the function documentation, but I'm not sure we ever really define what that is. Should we? And if so where does it belong?

Fixed a few minor typos in documentation.

Fixed some issues with comment wrapping.

Fixed concatenation spacing issues. e.g.) '. $var .' should be ' . $var . '

Changed

 $preset = image_preset_load($preset_name);

if (empty($preset)) {
  watchdog('image', 'A derivative of the image %path could not be generated because the preset %preset does not exist.', array('%path' => $path, '%preset' => $preset_name), WATCHDOG_ERROR);
  return FALSE;
} 

to

 if (!$preset = image_preset_load($preset_name)) {
  watchdog('image', 'A derivative of the image %path could not be generated because the preset %preset does not exist.', array('%path' => $path, '%preset' => $preset_name), WATCHDOG_ERROR);
  return FALSE;
} 

Since image_preset_load() returns FALSE if a preset is not found not an empty array.

Fixed some whitespace issues in image_preset_create_derivative()

Changed
$definition_cache = drupal_static(__FUNCTION__); to $definition_cache = &drupal_static(__FUNCTION__);

Fixed documentation for system_image_actions() which still referenced 3 possible attributes. Now written so that it does not matter how many attributes there are so we don't have to keep updating it if the number changes.

Rewrap comments for system_image_preset_request()

All of the system_image_*_image() action functions had watchdog messages that say something like 'image: %image' and the passed in an array of replacement options with %path, and %data but not %image. Fixed.

$schema['image_presets'] gave the name field a description of 'description' => 'The primary identifier for a node.', changed to 'description' => 'The preset name.', in both system_schema() and system_update_7030()

Rewrap comments in system_file_download()

drewish’s picture

FileSize
40.52 KB

I updated the github with the latest patch. A quick review of changes looked good to me.

Since image_preset_load() returns FALSE if a preset is not found not an empty array.
The previous code was correct since empty(FALSE) evaluates to FALSE as well.

The docs for image_preset_generate() and system_image_preset_request() seemed pretty at odds with their operation. They document returning TRUE which seems totally pointless considering their usage in theme_image_preset().
I'm also not too happy with the way that image_preset_path(), image_preset_url() and image_preset_generate() interact—especially image_preset_path()'s $file_system parameter. I can't see it being used anywhere and it seems to make a mess of the whole public private bit.

I've taken a swing at it to make some changes so that this hopefully operates in a more consistent manner:
- image_preset_path() only returns local file paths and doesn't check what file transfer mode we're using.
- Renamed image_preset_generate() to image_preset_generate_url() and have it always return a URL that will cause the derivative to be generated. I think it's more accurate considering what the function actually does.
- image_preset_url() checks if the file exists and if it does it returns the view URL, if not it calls image_preset_generate().
- Renamed system_image_preset_request() to system_image_preset_generate_url(). I think it's more accurate considering what the function actually does.
- Also tried to be consistent in calling preset arrays $preset and preset name strings $preset_name.

You can see my changes in this commit: http://github.com/drewish/imagecache_d7/commit/61ffcad2054042cf854c747e8...

eojthebrave’s picture

Can we remove the logic from the theme_image_preset function.

  +  $image = image_preset_path($preset_name, $path);
  +  if (!file_exits($image)) {
  +    $image = image_preset_url($preset_name, $path);
  +  }

And just replace the call to image_preset_path to image_preset_url which I think is essentially the equivilant of what is going on here.

drewish’s picture

eojthebrave, no we can't. i explained why in the comment preceding that:

+ // theme_image() can only honor the $getsize parameter with local file paths.
+ // The derivative image is not created until it has been requested so the file
+ // may not yet exist, in this case we just fallback to the URL.

re-reading it now it's not a great explanation but the short version is that if the file exists we need to use the local path, if it doesn't we need to use the url.

quicksketch’s picture

FileSize
41.36 KB

Looks like we're running into another collision with system_update_6029/30. Here's a reroll for head.

quicksketch’s picture

FileSize
41.36 KB

Correcting "function_exits" to "function_exists".

quicksketch’s picture

FileSize
41.42 KB

Updating a cache invalidation in image_action_delete() to new static mechanism.

webchick’s picture

Status: Needs work » Needs review
joshmiller’s picture

Status: Needs review » Needs work

This is a code syntax review for the patch at #31. There are a few off-topic comments mixed in. Also, I did not see any tests.

  1. +  $schema['cache_image']['description'] = 'Cache table used to store information about in progress image manipulations.';
    

    Order of words is confusing. Perhaps, "Cache table used to store information about image maniupaltions that are in-progress."

  2. + *   Array of actions associated with specified preset in the format
    + *   array('ipid' => array())
    

    Missing period.

  3. +function image_preset_flush($preset)
     {
    
    

    Suggestion: Add a drupal_message that links to a sound of a flushing toilet. It would be very "UX". ;-)

  4. +      // Make sure the width and height are computed first so they can be used
    +      // in relative x/yoffsets like 'center' or 'bottom'.
    

    Space between "x/y" and "offsets"

  5. + *   array(
    + *    'action' => Unique name of the action being performed
    + *    'module' => Name of module provoding the action
    + *    'description' => A description of the action
    + *   )
    

    Periods.

  6. +      'description' => t('Convert an image to gray scale.'),
    

    The proper word is "grayscale" not "gray scale." See http://en.wikipedia.org/wiki/Grayscale -- On a side note, do we display these actions based on what tools are available?

  7. +    print t('Error generating image.');
    

    Off-topic: Should there be more effort in stylizing a blank error message? Could we send back a jpg of a druplicon that is not smiling?

  8. + * @param $data
    + *   An array of attributes to use when performing the scale and cropt action.
    

    "scale and crop action"

That's all I could find after reading through the entire patch and checking spelling and code syntax.

Man, that is one helluva long patch.

Josh

quicksketch’s picture

Status: Needs work » Needs review
FileSize
41.7 KB

Thanks Josh! Very much appreciated. I corrected all your findings (except the "swooooosh!" flush message).

Should there be more effort in stylizing a blank error message?

Hopefully the odds of collisions like this occurring are rather low. However, because the image generation method is swappable (it's just a variable), you could indeed make a module that returned a sad Drupalicon. :-) Returning an actual image during processing might cause more troubles than it fixes though, since it would be scaled to whatever the height/width was set on the image.

While working on the UI I discovered that we're also not filtering out special values like "center" and "bottom". This is also fixed.

quicksketch’s picture

I'd like to remove the ability to use percentages in the values like width/height/offset. Does anyone have a reason *not* to remove it? I've never once found the ability useful, since you never know what size the user will upload, percentages just aren't that helpful. After an image has been scaled to a certain size, you can calculate percentages anyway on all subsequent actions and just enter a pixel value. Removing this ability lets us simplify the UI.

joshmiller’s picture

+1 on removing percentages -- I've always felt uncomfortable just giving numbers, I wanted to either be explicitly asked for pixels or be able to say "hey this is a pixel value." I've used imagecache for over 15 projects in the last year, never once used percentages. Can't even think of a use case for it.

I can't recall if I saw it in this issue or not, but will imagecache in core have a pluggable "delivery" that would allow someone to serve the final image as a png or gif?

EDIT: Drewish answered my question in #9, "The format of the original image is preserved. It might make sense to have a transform format preset though."

Josh

quicksketch’s picture

FileSize
41.84 KB

Great, thanks for the followup on percentages. I'll need to check with drewish before making that change.

I can't recall if I saw it in this issue or not, but will imagecache in core have a pluggable "delivery" that would allow someone to serve the final image as a png or gif?

Yes there's a mechanism for this already in D6 ImageCache. This core version also supports it. You can install the ImageCache Actions module then add an action for converting an image from one format to another. I've actually heard of (but never seen) using this with ImageMagick to convert TIFF and BMP files to jpgs for web display. Pretty bad-ass.

Another reroll since I found that the previous patch doubles up on URL strings.

quicksketch’s picture

FileSize
42.03 KB

Adding hooks for hook_image_preset_save() and hook_image_preset_delete(). These are needed to update settings upon deletion or renaming of presets. Fixing a caching issue with image_actions() and consolidating cache clearing routines.

quicksketch’s picture

FileSize
42.16 KB

Adding support for "short" color names such as #ccc instead of just #cccccc. Simplifying API and making $action['form'] and $action['summary'] optional, meaning we don't need these any more for Desaturate (which doesn't have a form or any summary).

eojthebrave’s picture

Status: Needs review » Needs work

I think the definitions of 'summary' and 'form' functions within system_image_actions() probably belong in the UI patch since they reference functions which do not even exist in this patch.

Also need to update the documentation for system_image_actions since 'summary' and 'form' are no longer required.

Suitable is spelt wrong in the documentation for system_image_preset_generate()

Do we need to call module_invoke_all('exit') here:

+  if (cache_get($cid, 'cache_image')) {
+    print t('Image generation in progress, please try again shortly.');
+    exit();
+  }

and or at the end of system_image_preset_generate()

We should probably also document all the hooks this introduces.

hook_image_actions()
hook_image_preset_save()
hook_image_preset_delete()
hook_image_preset_flush()

I can help out with that if need be, though I'm not sure where that documentation should live.

quicksketch’s picture

FileSize
41.18 KB

We've been having a discussion in IRC about the general approach to this patch. We're trying to figure out a way to split some of this (and all of the UI) into a separate module rather than adding to system.module. This should allow for more extensive UI work without bloating system.module further.

Considering we're going to need to rename "preset" anyway (#440468: Bikeshed the word image "preset" , the front-runner right now is "style"), and now we're working on splitting into another module, let's put this on hold until we get the new name/module hammered out.

This patch contains the last changes I had lingering on disk before we do the name change. It adds support for the grid-style selection of cropping and removes percentage support from height/width parameters.

sun’s picture

subscribing

...after a long IRC discussion... to be announced elsewhere.

quicksketch’s picture

After discussing with sun, drewish and others in IRC, we found that the only logical thing to name this "new" module to handle images in core is... "image"! So we've agreed to claim the "image.module" namespace for core, starting with this patch. Image module in its first iteration will consist of this patch (plus the UI patch) to enabling the creation of thumbnails in Drupal using ImageCache's mechanism.

Later patches to image.module will probably consist of adding image formatters and widgets (a la ImageField) to a core file field #391330: File Field for Core.

Announcement posted to the Image module queue: #513096: The Future of Image in Drupal 7.

I have a reroll in progress that begins this new module.

eojthebrave’s picture

FileSize
3.43 KB

Here is a first pass at documentation for hooks defined by the new image module thus far. Wasn't really sure where they should live so if someone wants to provide me with direction I can provide a patch. Otherwise, just take this and do with it as needed.

Also, wasn't 100% sure about style either since I've never written hook documentation before, ripped off developer/hooks/core.php for some ideas.

quicksketch’s picture

Fantastic thanks eojthebrave! I'm putting these docs in the new image.api.php file which will be part of the image.module directory.

Anonymous’s picture

i think this is an awesome decision - a real image module in core? w00t!

quicksketch’s picture

Status: Needs work » Needs review
FileSize
84.14 KB

Here's the first iteration of our new "image.module". :-)

I ran into some caveats versus the system .inc file approach, most notably ALL the inc files in system seem to be automatically loaded, unlike other module .inc files. This meant that our API function for getting a list of image presets or actions no longer worked as a .inc file. I moved them to the image.module file until we find a better way to make them always available.

I also did some renaming in this patch. Per #440468: Bikeshed the word image "preset" and discussions in IRC, the word "style" seemed to be the winner. Then we had the problem that "action" didn't really match with the word "style", so I renamed these to "effects". Unless you have VERY strong objections, please, please, please let's not rename these words again.

I added eojthebrave's hook docs into image.api.php with a few grammatical changes, and I updated the listing page to use the fancy-pants #attached_css and page rendering APIs, though it required a bizarre hack to make the theme function take 2 parameters even thought it only requires 1. See #373201: Enhance drupal_render() themeing. Return renderable array on tracker page. for more info on that. There's a standing TODO in place for now.

We're also in the situation where we need to decide whether to split or merge these patches again. The previous approach didn't require patching on top of patches, but with a single new module, clearly that would make a patch dependency and make managing these changes more difficult.

I'm also still figuring out git, I *think* I updated drewish's git repository correctly, but I'm not entirely sure. Apologies if I broke it.

Status: Needs review » Needs work

The last submitted patch failed testing.

quicksketch’s picture

FileSize
84.12 KB

Clearly I don't know what I'm doing yet with git. Here's a patch that applies without ignoring "a" and "b" branch directories.

sun’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

quicksketch’s picture

Some requirements to get this patch working correctly:

1) You MUST apply #514172: Increase Max Router Parts, which makes it possible to delete an effect.
2) You MUST create a modules/image/images directory and put a file at modules/image/images/sample.jpg. I'm still working on getting a sample image in there that is free from licensing issues.

eojthebrave’s picture

Whoa, that's a lot of code. Some of these may be unnecessary but here are the notes I kept while reading through it.

Delete action for image style effects doesn't work

Image effects have a 'description', but it isn't ever used anywhere. These used to appear on the page when choosing an effect to add to a style, however now that it has been changed to a select menu these are never used. Show them on the effect add/edit form?

There is at least one occurance where you use @params should be @param

Scale effect should validate that either height OR width is filled in.

This should probably be fixed so that we don't end up with something like.

 function theme_image_scale_summary($data) {
  return theme('image_resize_summary', $data) . ' ' . ($data['upscale'] ? '(' . t('upscaling allowed') . ')' : '');
} 

x100 or 100x or x

Include Anchor information in theme_image_crop_summary.

100x100 Top Left or something like that.

CSS, properties should be in alphabetical order. http://drupal.org/node/302199

Extra leading whitespace on line 109 of image.api.php

; $Id: blog.info,v 1.11 2009/06/08 09:23:51 dries Exp $ in image.info

Do we need all the documentation for image_image_effects() now that it is documented as hook_image_effects()?

Comments on image_style_generate_url need to be re-wrapped, and suitable is spelt wrong "suitale"

Should documentation like this

*   An array of attributes to use when performing the resize effect.
*   array('width' => int, 'height' => int)

be switched to

*   An array of attributes to use when performing the resize effect with the following key value pairs.
*   - "width": An integer representing the desired width in pixels.
*   - "height": An integer representing the desired height in pixels.

and so on. There are a few places where this should change if we decide that the later is more appropriate.

There are a still a number of occurence where "Implements" should be replaced with "Implement". e.g.)

* Implements hook_file_move().

function image_style_load($name = NULL, $pid = NULL) {

$pid should probably be $isid, and updated in the function as well.

 * @return
*   Array of styles array($pid => array('id' => integer, 'name' => string)). 

$pid should again be isid

*   The absolute URL to a style image. If the site is using the default
*   method for generating images, the may not yet and will only be created
*   when a visitor's browser requests the file.

should be

*   The absolute URL to a style image. If the site is using the default
*   method for generating images, the image may not exist yet and will only be
*   created when a visitor's browser requests the file.

the function image_style_generate_url() needs to be in image.module or it is not available to functions like theme_image_style which calls it via image_style_url().

The call to image_style_generate_url() in image_style_url() is wrong as it is passing in a string but the function needs an image style array. Replacing with with something like return image_style_generate_url(image_style_load($style_name), $path); should do the trick. That will also fix two of the failing tests.

Not sure about the third failure which is at line 566 of user.test. The issue is that the call to theme('image' ... returns the real path to the image while the page that it is asserting against contains the url returned by image_style_generate_url(), as far as I can tell.

drewish’s picture

Status: Needs work » Needs review
FileSize
42.04 KB

I've sorted quicksketch's code back into two branches so that API and UI are separate again.

Delete action for image style effects doesn't work

see quicksketch's comment in #52 about applying #514172: Increase Max Router Parts

There is at least one occurance where you use @params should be @param

Fixed.

CSS, properties should be in alphabetical order. http://drupal.org/node/302199

Fixed.

Extra leading whitespace on line 109 of image.api.php

Fixed.

; $Id: blog.info,v 1.11 2009/06/08 09:23:51 dries Exp $ in image.info

Fixed.

Do we need all the documentation for image_image_effects() now that it is documented as hook_image_effects()?

No, I don't think so. Dropped it.

Comments on image_style_generate_url need to be re-wrapped, and suitable is spelt wrong "suitale"

Fixed.

Should documentation like this ... be switched to ...

Yeah excellent idea. Fixed that.

There are a still a number of occurence where "Implements" should be replaced with "Implement". e.g.)

Fixed.

$pid should probably be $isid, and updated in the function as well.
...
$pid should again be isid

Fixed, as well as a comment in there.

the function image_style_generate_url() needs to be in image.module or it is not available to functions like theme_image_style which calls it via image_style_url().

The call to image_style_generate_url() in image_style_url() is wrong as it is passing in a string but the function needs an image style array. Replacing with with something like

Not sure about the third failure which is at line 566 of user.test. The issue is that the call to theme('image' ... returns the real path to the image while the page that it is asserting against contains the url returned by image_style_generate_url(), as far as I can tell.

I think I'd fixed those already.

Status: Needs review » Needs work

The last submitted patch failed testing.

drewish’s picture

FileSize
47.95 KB

Changes since the last patch:
* Pulled in some of quicksketch's help changes.
* Adding some basic unit tests for the image.module path and URL functions.
* image_install() now tries to create the 'styles' directory if it doesn't exists.
* Renamed _image_strip_file_directory() to _file_strip_files_directory() and moved it to file.inc where it can be re-used by file_create_url(). Might need to fork this off into its own issue.
* Removing the $toolkit parameter from image_effect_definitions() since we are not doing anything right now. We can add it back later.
* Moving the existing Image API tests into the Image group for consistency.

Things to do:

Wondering why we're trying to create the directory twice in image_style_create_derivative():

  if (!file_check_directory($directory, FILE_CREATE_DIRECTORY) && !mkdir($directory, 0775, TRUE)) {

seems to me like some left over cruft from imagecache.module that we should remove...

in image_style_create_derivative() we document a NULL return status but we never return that. docs or code should change.

Can we drop image_filter_percent() and image_filter_keyword() yet?

drewish’s picture

FileSize
49.12 KB

Over on the UI issue eojthebrave pointed out:

I believe there is also supposed to be a widget that allows you to choose which image style to use when rendering a user's profile picture at Administer > Site Configuration > Users, don't have time to poke around at the moment though. (drewish? quicksketch?)

I just added a basic one. The help text links to the URL provided in the UI patch but I think that's tolerable. I also tried to work on the help to make it clear that the picture maximums are enforced first. We should probably make it clearer that if you're using a style you might want to increase the values (only really an issue if you did the minimal install profile).

Also fixed the hook_perm -> hook_permission that eojthebrave also noticed.

quicksketch’s picture

FileSize
49.94 KB

Latest update from the Git repo:

- Striping the file directory prefix has been put into file.inc as file_directory_strip()
- Removed unnecessary access callback that is no longer used.
- Utilizes #515280: file_check_directory() should create recursively so we don't have to use that funky extra mkdir() call.

yched’s picture

Minor: I think core tries to settle on hook_*_info() for things like hook_image_effects()

quicksketch’s picture

FileSize
55.16 KB

This patch makes several updates to the API:

- Changes hook_image_effects() to hook_image_effect_info()
- Significantly cleans up PHPdoc
- Moves a few functions around between .inc files
- Code style cleanups

Note that tests do not pass, not because things are broken but because we need to update the tests. I'm posting regardless so everyone can check the latest progress.

drewish’s picture

Status: Needs work » Needs review

actually i'd like to see the testbot's feedback. i can't get any of the user tests to run at all on my machine.

Status: Needs review » Needs work

The last submitted patch failed testing.

quicksketch’s picture

Status: Needs work » Needs review
FileSize
56.07 KB

With the user test fixed.

webchick’s picture

Ok, spent about two hours code reviewing this patch tonight. Haven't tested it out yet w/ the UI stuff. Here's what I found:

+1 for file_directory_strip(). It's nice to have that scary code stashed away somewhere so we don't have to look at it. ;)

+100 for the name change from "preset" and "action" to "image style" and "image effect." I think that will go a very long way toward helping new users figure out the power of this feature, and also will avoid weird collisions with core's Actions.

+function hook_image_effect_info() {
+  $effects = array();
+
+  $effects['mymodule_resize'] = array(
+    'name' => t('Resize'),
+    'help' => t('Resize an image to an exact set of dimensions, ignoring aspect ratio.'),
+    'function' => 'mymodule_resize_image',
+    'form' => 'mymodule_resize_form',
+    'summary' => 'mymodule_resize_summary',
+  );
+
+  return $effects;
+}

Elsewhere we call 'functions' 'callbacks', so let's be consistent here. Since 'function', 'form', and to some extent 'summary' are all callback functions we should probably name-space these like:

'effect callback' => ...
'form callback' => ...
'uh. not sure for summary yet' => ...

'form' and 'summary' don't make sense without the UI patch (their callbacks aren't even in this patch) so they should be moved to the other patch, where we can discuss those in context. That means removing the summary functions from hook_theme() as well.

+ * Respond to style updating.
...
+ *   The style being updated.

Let's make sure to properly refer to 'image styles' so that these aren't confused with other types of styles: CSS, Views/Panels, etc.

+ * @return
+ *   None.

We don't need that if the function isn't returning anything. See #516254: Remove @return PHPDocs where nothing is returned.

Ok, I'm very tired and luckily quicksketch is the man and kept track of all my other comments, so here they are in abbreviated form. THANK YOU QUICKSKETCH!

- Move 'form' and 'summary' to UI
- Move hook_theme summary* items to UI
- Fix watchdog errors on effects not to use print_r
- Remove the word "default" from effect PHPdocs

$schema['cache_filter'] = drupal_get_schema_unprocessed('system', 'cache');
$schema['cache_filter']['description'] = 'Cache table for the Filter module to store already filtered pieces of text, identified by text format and md5 hash of the text.';

- Add "weight" index to image_effects
- Rename "effect" column in image_effects to "name"
- Define foreign keys in image_effects:
'foreign keys' => array(
  'cid' => array('aggregator_category' => 'cid'),
),


- Reference foreign key relationship in description for image_effects.isid.
'description' => 'The primary identifier for an image style.',


- Code comment cleanup:
array_shift($args); // Remove the "styles" item.
$style_name = array_shift($args);
TO
// Discard the first part of the path, then get the style name from the second part.
array_shift($args);
$style_name = array_shift($args);

- Code style:
foreach (image_styles() as $style) {
TO
$styles = image_styles();
foreach ($styles as $style) {

- Conditionally clear block_cache table:
cache_clear_all('*', 'cache_block', TRUE);
TO
if (module_exists('block')) {
  cache_clear_all('*', 'cache_block', TRUE);
}

- function testEffects() needs PHPdoc
- * FUNCTIONS STILL TO TEST, change to TODO:
Dries’s picture

First review:

- The API looks great, and the documentation is great too. It was relatively straightforward to review, which is a good sign. Excellent work.

- I installed the patch. The user avatar help text reads: "Image styles may be configured in the Image handling administration area." but the link to the Image handling administration area is broken. There is the "Image toolkit" section but that doesn't show image styles.

- User module seems to have a fall-back when image module is not enabled. Is that useful? Should user.info have a image.module dependency and always be enabled?

- file_directory_strip() only strips the first directory of the path, it seems. For example, abc/def/gef.jpg becomes def/gef.jpg not gef.jpg -- I think. Might be worth clarifying by saying it strips the first part? Or we should change the behavior and use strrpos().

- The hook_image_effect_info() has a 'help' parameter -- wondering if that should be 'description' for consistency with the rest of core.

- In image_image_effect_info(), the image_scale_and_crop effect has the wrong summary function.

- In your watchdog message of the format: Image crop failed. image ..., let's change that . to a : as in Image crop failed: image ....

- I found the function name image_style_create_derivative not to do what I expected. It creates an image, not an image style. Maybe we can find a better name? Or maybe I'm mistaken.

Hot stuff! Great work. Let's get this in so we can make progress with filefield and imagefield. Would love to test the UI first though.

quicksketch’s picture

Status: Needs review » Needs work

Thanks Angie and Dries! I'll work on rerolling all the suggested changes. Most are completely valid, a few rebuttals:

- User module seems to have a fall-back when image module is not enabled. Is that useful? Should user.info have a image.module dependency and always be enabled?

If we make User depend on Image, that'll make Image a "required" module. Although the number of sites that *wouldn't* use Image is very, very low (say NowPublic, which has a dedicated Image resizing server), I'm hesitant to make Image absolutely required in case a better solution does come along (say like the ImageCache/ImageField duo displacing the current Image). But I'm just anti-required modules, maybe I'm just be speaking my bias.

- file_directory_strip() only strips the first directory of the path, it seems. For example, abc/def/gef.jpg becomes def/gef.jpg not gef.jpg

This function strips just the file directory path, not the entire path, we could just use basename() for the filename alone.

The hook_image_effect_info() has a 'help' parameter -- wondering if that should be 'description' for consistency with the rest of core.

In the UI patch, we literally use the contents of $effect['help'] as part of hook_help(), so it might make sense to keep it different from other implementations that use description.

Thanks for the excellent reviews! Dries, if you haven't already, you should definitely try out the UI patch. It's completely functional but isn't passing it's own tests yet.

quicksketch’s picture

To quote Eaton:

We've gone to a lot of effort in D7 to make previously required modules (like block) optional -- I'd prefer that image not become required just because one feature in user benefits from its presence.

Which I think might be better stated than my reason above. :-)

drewish’s picture

Status: Needs work » Needs review

Been working my way down the list and committing to github as I go. I'll roll a patch when I'm all finished up.

Some stuff I have done so far:
* fixed up some of the PHPDocs, removing "@return None." and making it clear that $image parameters come from image_load().
* Fix watchdog errors on effects not to use t() or print_r().
* Reusing core's cache schema.
* Renamed the effect's 'function' to 'effect callback', I moved the 'summary' and 'form' over to the UI patch but didn't mess with renaming any of that. We can bike shed that when we get to the UI patch.

One of webchick's comments was:

foreach (image_styles() as $style) {
TO
$styles = image_styles();
foreach ($styles as $style) {

In this case I don't think gain anything from the change. I think the precomputing of loops you were describing is more effective on code like this where you're running the function count() on every pass through the loop:

for ($i=0; $i<count($bar); $i++) {
}

so I'll be ignoring that bit.

Dries had commented:

- I installed the patch. The user avatar help text reads: "Image styles may be configured in the Image handling administration area." but the link to the Image handling administration area is broken. There is the "Image toolkit" section but that doesn't show image styles.

Yeah, I knew about that when I committed it, it just seemed easier to put the link into the API patch knowing that the URL would be valid once the UI gets in. If it's a big deal we can move it over to the other patch.

drewish’s picture

FileSize
54.63 KB

I don't fully understand the impact of

- Rename "effect" column in image_effects to "name"

so I'll leave that to quicksketch.

ended up moving the user picture style description to the UI. we can deal with it there.

quicksketch’s picture

FileSize
55.5 KB

One last pass to address the Dries/webchick reviews with the following changes:

- Renamed the "effect" column to "name".
- Further clean up on the PHPdoc for effect callbacks.

Status: Needs review » Needs work

The last submitted patch failed testing.

quicksketch’s picture

Status: Needs work » Needs review
FileSize
55.5 KB

Missed one "effect" to "name" change in the install profile.

webchick’s picture

Status: Needs review » Reviewed & tested by the community

Awesome work, guys!

I re-read through the patch tonight and it looks great to me! I think all of the previous feedback has been incorporated: the help text has been moved to the UI patch, and the case has been made for image module to remain optional.

I have nothing else to complain about. :) It sounded though like Dries wanted another peek at this before committing, so marking RTBC.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

Committed to CVS HEAD! Great, great job all.

This requires a CHANGELOG.txt so let's follow-up with that.

Let's keep rollin' ...

quicksketch’s picture

WOOOOOHOOOOOOO!!!

I'm rolling hard on #391330: File Field for Core, which should be followed by Image Field in core.

yched’s picture

webchick’s picture

Status: Needs work » Fixed

quicksketch wrote up a general CHANGELOG.txt entry for all this image stuff at the UI patch. Closing this one as fixed. :)

Status: Fixed » Closed (fixed)

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

grendzy’s picture