At first I was blaming myself for careless cleaning up the database or deleting the wrong files in the filessystem. Then I though its some inconsistency or bug in core or some module.

Now I know it's a feature (kind of). Well, I hope it isn't. It's a bug in Image crop (kind of).

I had some images with Image crop in my site. While working with it for a while I discovered that the original files that Image crop uses as source for cropping are not available anymore after a while (e.g. next day).

Here is why:

When a file is uploaded to a field controlled by Image crop, it will be saved to the filesystem and a database record is added to the file_managed database table.

Then a copy is made of that image with a new name (suffix counter) and a record is added for it to the database as well. This copy is then used as the original image for future recropping.

Finally a record is created in the file_usage table for the copied image. The initial image record is set as the referencing image.

This does all make logical sense. What it results in though, is two records in the file_managed table, one has status of 1 and the other a status of 0.

Now read the comment for the field 'status' in file_managed in in the database schema:


A field indicating the status of the file. Two status are defined in core: temporary (0) and permanent (1). Temporary files older than DRUPAL_MAXIMUM_TEMP_FILE_AGE will be removed during a cron run.

DRUPAL_MAXIMUM_TEMP_FILE_AGE is set to 21600 seconds (at system.module) which is 6 hours.

That means Image crop's original images get inevitably wiped after 6 hours (if cron is running properly).

If this is a feature, then I think it should be documented. And optional.

After some investigation I found out that status 0 is the default for images and it's node_save() that sets the status of attached images to 1. So the responsibility should lie at Image crop to care about the original image's status. This can be done by explicitly setting it to 1.

In imagefield_crop_widget_value():342 add this line:

      $orig->status = 1;

berfore this one

      $orig = file_save($orig);

I have tested it and it seems to solve the problem.

Patch follows.

CommentFileSizeAuthor
#6 save-what-can-be-saved-1219208-6.patch575 bytesjox
#4 save-what-can-be-saved-1219208-4.patch575 bytesjox
#1 original-image-gets-wiped-1219208-1.patch476 bytesjox

Comments

jox’s picture

jox commented
Status: Active » Needs review
StatusFileSize
new original-image-gets-wiped-1219208-1.patch476 bytes
yhager’s picture

yhager commented
Status: Needs review » Closed (fixed)

Thanks for the detailed and informative report, and patch submission.
Committed.

jox’s picture

jox commented

Thanks for the quick attention.

jox’s picture

jox commented
StatusFileSize
new save-what-can-be-saved-1219208-4.patch575 bytes

While this might already have messed up many configurations, there could still be potential to rescue some data.

It would make sense to fix the database of installations that have existing Image crop images. Because the patch does not apply for previously stored images.

So my proposal is to add an imagefield_crop.install file with an update function like this:

/**
 * Update files in the database that were previously stored with status=0.
 */
function imagefield_crop_update_7000() {

  $fids = db_query("SELECT fid FROM {file_usage} WHERE module='imagefield_crop' AND type='file'")->fetchCol();

  db_update('file_managed')
    ->fields(array('status' => 1))
    ->condition('fid', $fids, 'IN')
    ->condition('status', 0)
    ->execute();
}

Here is a patch that adds that function. (I hope the patch works, I had to use 'git diff --cached'.)

jox’s picture

jox commented
jox’s picture

jox commented
Status: Closed (fixed) » Needs review
StatusFileSize
new save-what-can-be-saved-1219208-6.patch575 bytes

Modified the patch so status gets checked before fid. Minor but should be better.

jox’s picture

jox commented
Priority: Major » Minor

After thinking about it, I admit the update function seems kind of pointless. If everthing is working as expected there will not be much to rescue. I can not really estimate if there is cases where this actually would make sense. So if you decide to discount the idea then I'll agree.

jox’s picture

jox commented

Well, one case would be a site that has cron execution disabled (for whatever reason). Without the update function, all original images that were uploaded while cron was disabled would be deleted as soon as cron is enabled anytime in future.

Taxoman’s picture

Taxoman commented

#8: Yes, this should be done "just in case". Several sites may have cron disabled. Nice to let those avoid loosing more than "necessary".

Btw, is this relevant/committed to the 2.x branch too?

jox’s picture

jox commented

@Taxoman: I also think the potential benefit overweights the disadvantages (is there any significant at all?).

Yes, part 1 (the main fix) is incorporated into the 2.x branch too. Part 2 (the update function) was not yet considered in any version.

Taxoman’s picture

Taxoman commented
Priority: Minor » Major

I guess that means that normally part 2 should be fixed in 2.x first and then backport to 1.x(?), but in this case, with 1.0 recently released and no stable 2.x yet, I think a 1.1 version should get priority, since this is about actually loosing data, and people would now start using this module because of the recent stable release?

I think this deserves "major" status to catch newcomers' awareness meanwhile.

yhager’s picture

yhager commented
Status: Needs review » Closed (fixed)

@Taxoman, #1 is present in current 7.x-1.0, and I agree with @jox there is not much point in committing #6, so I am not sure what you mean here. If someone needs #6 for her site, she can apply the patch from here temporarily.
2.x is completely experimental at the moment anyway.

Closing.