don't manipulate images on hook_load

I've also personally had a problem with node access-time regeneration. I have some very large images (about 5Mb) so when regenerating a gallery page I have to endure approximately 3 timeouts before all images are properly regenerated. this is very inconvenient for users will not consider this normal and will quit trying to see the gallery.

Wouldn't it be better to schedule image regeneration in cron job doing it in parts and being able to configure how many images are regenerated each run?

edit: this post is set related to D5 but I'm using D6...

@salvis sorry about the slightly off-topic comment..

I've had the problem in D6 yes. As it called image regeneration for every node during permission rebuilding, the rebuilding batch process did timeout many times.

I confirm this for Image version 6.x-1.0-alpha3 and Drupal 6.5.

The batch permissions rebuild process or an update of permissions for any role in Taxonomy Access Module launches regenerating of derivatives for all image nodes.

There are other scenarios similar to this one which suggest a solution that is as generic/generally availble as possible. How about a small addition to the poormanscron module (?) that can set a temporary flag with some way of measuring the progress, so that for the subsequent period of time, each page load triggers cron for a small set of nodes, until done, and then going back to normal cron schedule again, after the "measure" has been reached?

Should be possible for other modules to hook into this easily.

I posted this idea as a feature request for poormanscron here:
"General-purpose addition for smaller cron runs for better/smarter performance management."
http://drupal.org/node/331396

Bumping because it happened to me as well.

Well, the problem is that image rebuilds derivatives on every hook_load. It also displays messages on the site if a derivative needs to be rebuild.

Perhaps this should be done in image_cron(), instead. The problem is that it would have to check all files in the cron, which would also kill it.
Perhaps we could introduce an additional column to the image table, or add an additional table rebuild_queue, which saves the fids where the derivatives need to be rebuild. In this case we would know all derivatives that need to be rebuilt upfront, and could probably issue something more efficient to rebuild them, along the lines of

mogrify -geometry $derivative_width x $derivative_height $images_in_queue

Marking #516064: Out of memory error due to uploading large file; can't delete node as a duplicate.
That descripes a different problem but with the same root cause: we're doing stuff with images on hook_load.

So: should we move this to hook_view?
What might be the negative consequences?
What relies on image regeneration?

A grep shows that the only thing relying on image_load() regenerating derivatives is the views handler.

So here is a patch:
- moved renerating call to our hook_view, while keeping the logic checking in hook_load
- updated our tests
- changed image_handler_field_image_node_image to call image_view() if needed.

Simpletest reports everything passes :)

+++ image.module	15 Aug 2009 12:16:54 -0000
@@ -436,8 +436,18 @@ function image_form_submit($form, &$form
  * Implementation of hook_view

Missing parenthesis and period (full-stop).

+++ image.module	15 Aug 2009 12:16:54 -0000
@@ -436,8 +436,18 @@ function image_form_submit($form, &$form
+ * Also rebuilds image derivatives if $node->rebuild_images was set by 
@@ -463,6 +473,9 @@ function image_view($node, $teaser = 0, 
+ * Loads image data from the database, and sets $node->rebuild_images if a 
+++ views/image_handler_field_image_node_image.inc	15 Aug 2009 12:16:54 -0000
@@ -107,9 +107,13 @@ class image_handler_field_image_node_ima
+    // than the original fall back to the original. 
...
+    }    

Trailing white-space.

This review is powered by Dreditor.

Additionally, whenever $node->rebuild_images is tested, the code should use !empty() to prevent PHP notices. I recommend verifying that your php.ini contains E^ALL for error reporting.

I fixed all of those issues and committed attached patch.

Thanks for reporting, reviewing, and testing!

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

Thanks!

And argh PHP.... :/

I can mark it as needing to be ported, but I don't have time to roll a patch on D5 -- will apply something that's posted and tested though.

Can someone please verify that since this patch was committed,

• Derivative images are not created when an Image is uploaded via Image Attach functionality to attach to a non-image node. Hence the node view of a node with attached images is broken
• Bulk updates to Rebuild derivative images does not report any messages although the missing derivative image files are generated.

(Tested against Image HEAD).

Sorry, I'm not following you. Are you reporting problems as a consequence of this patch, or things that need to be checked?

Sorry for not being clearer,

My first point is a problem - was this patch tested for Image Attach? It doesn't seem reasonable to require users who upload-and-attach then to have to take further action to cause the creation of the derivatives.

My second point is a question - is it "normal" for a bulk update to complete without any confirmation message?

Point 1:

Image attach -- I think you're right, we have a problem.
Here is the flow:

1. image_attach_nodeapi, $op = load -- loading the attaching node. Get the iid.
2. image_attach_nodeapi, $op = view -- viewing the attaching node. Theme the image with theme_image_attach_{$teaser_or_body}
3. theme_image_attach_body does a node_load on the attached image node (which is HIDEOUS CODE! ARGH! TEH BAD!)
4. a node_load on an image node calls image_load, which determines IF a rebuild is needed but doesn't do one any more.

So, to fix:
Call image_update on the image node after it's been loaded, if regeneration is required.
Patch attached for this.
Please test :)

Subsequently TODO:
- refactor the UGLY UGLY theme functions. this is already a filed issue: #412288: restructure theme_image_attach_body/teaser .
- I really don't like calling image_update($node); to rebuild. However that function is scary and works currently as far as we know, and image_attach is scheduled to die anyway. :)

point 2:

Bulk updates to Rebuild derivative images -- you mean image_operations_rebuild? and do you mean there should be a watchdog message? For each node or for all of them? or do you mean a dsm, and again, for each or all? AFAIK we've removed some message output as it was felt this module is a bit pointlessly noisy. We still watchdog in image_view which IMO is a bit pointless -- I think there may be an issue for this. Or file a new issue for rebuild watchdog and DSMs in general please?

Point 1 - having tested the fix patch, I think there are a couple of situations not yet covered:

Using the Attach button on (for example) a Page form to upload a new file does not result in the Thumbnail being generated at that time, so the form does not show what has just been attached when it is re-displayed via image_attach_image_add_submit().

Also if image_attach_block() gets invoked to display a thumbnail of the attached image, before the main node display content has been built for the first time, the thumbnail file does not exist so the markup for it is not created. For example I have the Image Attach block in my right sidebar, I created a Page, attached an image (using the Attach button so no thumbnail was created yet), then clicked Save. I got the Preview image in my main content area but the side block was missing the thumbnail until I hit refresh on the whole page.

Point 2 - a dsm for "all done" would probably be enough just to reassure the user that the processing happened. I don't mind filing a new issue for that.

Since we can't and shouldn't solve 3) in this issue, this looks ready to fly.

Note that stuffing a

// @todo Theme functions shouldn't load data.

before that code would be nice. ;)

// @todo Theme functions shouldn't load data.
Oops forgot. But believe me, I'm aware of this! :)

Committed :)

But a couple more things it seems:

1b: Using the Attach button on (for example) a Page form to upload a new file [...]
1c: Also if image_attach_block() gets invoked to display a thumbnail of the attached image [...]

Here's another patch:

- refactored the checking of $node->rebuild, as it seems we're doing it in lots of places, and checking that every time is getting tedious. So added a function _image_build_derivatives_if_needed() -- basically, call this anytime you do a node_load but not a node_view. It's still messy code but it should work and we don't have time to make pretty.
- fixed 1b in the node edit form.
- fixed 1c in the block display.

If you have time to test, yay. If not, a quick eyeball for sanity would be appreciated before I commit.

And PS, sp3boy, thank you for catching all these hiccups! It's really helpful :D

+++ image.module	26 Aug 2009 08:48:59 -0000
@@ -472,6 +469,22 @@ function image_view($node, $teaser = 0, 
+function _image_build_derivatives_if_needed(&$node) {
+  if (!empty($node->rebuild_images)) {
+    // Possibly TODO: calling a hook implementation here isn't very elegant.
+    // but the code there is tangled and it works, so maybe leave it ;)
+    image_update($node);
+    watchdog('image', 'Derivative images were regenerated for %title.', array('%title' => $node->title), WATCHDOG_INFO, l(t('view'), 'node/' . $node->nid));
+  }  
+}
+++ contrib/image_attach/image_attach.module	26 Aug 2009 08:48:59 -0000
@@ -79,6 +79,7 @@ function image_attach_block($op = 'list'
             $image = node_load($node->iid);
             if (node_access('view', $image)) {
+              _image_build_derivatives_if_needed($image);
@@ -191,6 +192,7 @@ function image_attach_form_alter(&$form,
         $image = node_load($node->iid);
+        _image_build_derivatives_if_needed($image);

ok, please someone tell me why we don't do this directly in image_load() ?

I'm on crack. Are you, too?

Because of mass operations and deletion of nodes with bad images.

ok, after discussion with joachim in IRC, we want to

- revert everything in here

- put the derivative image rebuilding back into image_load()

- exclude the derivative image rebuilding when we assume that some mass operation is happening.

Mass operations happen with Batch API or on admin/content/node (which does not use Batch API when deleting nodes). Hence, we can explicitly check for

- arg(0) != 'batch'

- strpos($_GET['q'], 'admin/content/node') === FALSE

For other, programmatic invocations, we can - albeit wonky - add second argument to image_load(), so developers can force to skip the rebuilding.

Given we've already said somewhere else that overloading hook implementations is a Bad Thing, we shouldn't do it ourselves.
Could test for a $ndoe->dontrebuild prroperty?

First of all, apologies for causing confusion. I have been trying to test this fix in conjunction with #81102: Attach Multiple Images with image_attach using Drupal upload mechanism and so my references to an Attach button are invalid unless that patch was also applied.

So, this time I have tested against Image HEAD without any other patches!

I think it is OK except for a couple of bugs:

In theme_image_attach_teaser() and theme_image_attach_body() of image_attach.module, there is

_image_build_derivatives_if_needed($node);

which I think should be

_image_build_derivatives_if_needed($image);

as it is the $image variable that has just been loaded with the data for the attached image.

Oops, looks like I was sitting with the form in my browser for some time and missed all the latest posts.

I think I've also inadvertently changed the status so I'll set it back. Sorry about that.

I've also lost track of where this is heading, fix-wise.

So:

- 22 -- not applying.
- 18 -- reverted.
- 11 -- reverted.

So CVS HEAD is now back to where it was before any of this issue.
(Patches have come in from elsewhere in the meantime, of course).

So now here's a patch against the new CVS HEAD.

Testing for paths as sun suggested, which is a bit ugly but much less code work than chasing image_load all over.
Also refactored all the rebuilding checking into a helper function to make it more readable.

I think you need another closing parenthesis on the line

if ((arg(0) != 'batch') || (strpos($_GET['q'], 'admin/content/node') === FALSE) {

Having corrected the typo previously mentioned, I just gave it a test against a fresh D6 install with Image HEAD.

Direct creation of an Image node looks OK but derivatives were not created for an Image node created by uploading as an attachment. I had to edit the resultant image node and apply the Rebuild derivative images option to get them to appear. Have run out of time to do any more testing today.

Good catch :)

_image_build_derivatives_if_needed() was still referring to $original_path which is not set in the function, hence $needed_sizes was not being set.

Have corrected in attached patch which tests OK for image node creation and multiple image attachment to a Page node.

o_O ... subscribing.

This patch seems to do something wacky to images that are a certain size -- the node is trying to show the preview size, as there s a link to original, but no preview size exists -- presumably because the original is smaller than preview.

Trying to figure it out, but my concentration is flagging; a bit worn out with moving around trying to find wifi and power at Drupalcon.

Apologies, I must be losing my touch :(

There was a second reference to $original_path which I didn't fix, so as you say, if the original was smaller than a derivative, the derivative path was not being set to that of the original when image_load() ran.

Edit: I was feeling bad about not running the SimpleTest suite on this patch, until I tried it and found that because image.test sets very small dimensions for the scaled down derivative images, the test does not cover the "what if the original is smaller than the preview / thumbnail size" case.

Brilliant :)
Committed.

Hello again,

despite having made the last patch on this, some further testing I'm doing with Image suggests that there's an incorrect test in the patch:

+  if ((arg(0) != 'batch') || (strpos($_GET['q'], 'admin/content/node') === FALSE)) {
+    _image_build_derivatives_if_needed($node);
+  }

this is supposed to avoid rebuilding derivatives if either the q starts with "batch" or "admin/content/node" I think? But the test is written as (NOT A) OR (NOT B) where A and B are by definition mutually exclusive. So as they'll never both occur at the same time, the derivative rebuild is always going to get run?

I'd appreciate confirmation before putting a revised patch together with

+ if ((arg(0) != 'batch') && (strpos($_GET['q'], 'admin/content/node') === FALSE)) {

Simple way to test: stick that block with a dsm('hello') in it in a module's hook_init.

But I think you're right.

(NOT A) OR (NOT B)
is identical to
NOT (A AND B)

This should definitely be an AND, or perhaps to be clearer, a NOT (OR) -- though does that make the evil PHP === condition worse?

I tried the code in hook_init() and got the expected desired results with "&&" rather than "||". So here's a patch to fix that one line, although I'm not sure whether it's better to roll back the last patch and make the whole thing again. Let me know if you'd like me to do that instead.

# php -r "$arg0 = 'batch'; $q = 'batch/run'; var_dump(($arg0 != 'batch') || (strpos($q, 'admin/content/node') === FALSE));"
bool(true)

# php -r "$arg0 = 'batch'; $q = 'batch/run'; var_dump(($arg0 != 'batch') && (strpos($q, 'admin/content/node') === FALSE));"
bool(false)

# php -r "$arg0 = 'admin'; $q = 'admin/content/node'; var_dump(($arg0 != 'batch') && (strpos($q, 'admin/content/node') === FALSE));"
bool(false)

# php -r "$arg0 = 'batch'; $q = 'batch/run'; var_dump(!(($arg0 == 'batch') || (strpos($q, 'admin/content/node') !== FALSE)));"
bool(false)

# php -r "$arg0 = 'patch'; $q = 'patch/run'; var_dump(!(($arg0 == 'batch') || (strpos($q, 'admin/content/node') !== FALSE)));"
bool(true)

True. Not sure whether the second approach of a positive, but overall negated test would be easier to grok - probably not.

Hey nice to see a RTBC :)

#226121 by sp3boy: Fixed bad logic from previous patch to this issue.

Sorry to reopen this can of worms, but I have a suggestion.

I'm learning a little bit about Drupal development, and it seems like rebuilding derivative images would be a perfect candidate to be a triggered action. It could be set to hook_view() by default, but users could configure it to be triggered on hook_update() and/or hook_insert(). This would be through an implementation of hook_action_info().

Is this a good idea, or am I just on crack?

Also, since I haven't had any problems with the current code, I'm switching this to a low-priority feature request.

Sorry 'bout that, this seemed like the proper place to ask, since it would require changes in the same code.

@Karlheinz It's an interesting idea, but it doesn't solve the core problem which is when to do the rebuild.