Fix for vips intervention image backend

[forsdahl]

The vips intervetion image backend isn't working right now, because InterventionBackend unlinks the temporary file too early. It is removed before the actual libvips commands are run against it, which makes them fail. The temporary file is removed in the destructor of InterventionBackend anyway, so this section is unnecessary.

[GuySartorelli]

The temporary file is removed in the destructor of InterventionBackend anyway, so this section is unnecessary.

Could this cause storage problems if a single request (e.g. some dev/task) affects a large number of very big images?

[michalkleiner]

Should be fine as long as we can be sure we have a new instance for every image.

[forsdahl]

The only case I can see where this might be a problem is if the same image backend instance is used for several manipulations on different images (alhtough I don't see exactly the use case where that would happen). One thing that could be done is to make sure we unlink the temporary file if the image resource of the image backend is set to null (which happens at the end of an image manipulation), i.e. in setImageResource in InterventionBackend.

[forsdahl]

Alternatively, PR #527 also fixes the issue with the vips backend. That PR removes the temporary file creation entirely, and always creates the image resource from the stream, which also works well for the vips driver. @christopherdarling can you have a look at that pull request and see if can be made to pass all tests?

[christopherdarling]

@forsdahl yep - it's on my list just don't have much capacity right now

[kinglozzer]

I’ve had a look at this PR and I’d be okay with merging it with the amend to setImageResource() described above.

The only case I can see where this might be a problem is if the same image backend instance is used for several manipulations on different images (alhtough I don't see exactly the use case where that would happen)

There’s certainly nothing out of the box that would do this, the ImageManipulation trait will create a new image backend for each image, nothing in there is static or shared between Image instances.

One thing that could be done is to make sure we unlink the temporary file if the image resource of the image backend is set to null (which happens at the end of an image manipulation), i.e. in setImageResource in InterventionBackend.

I think that’s a good idea. I’d also tweak setAssetContainer() to call setImageResource(null) just in case anyone is using the API directly.

We’re not doing any more minor releases of Silverstripe 4 and I don’t think we can really sneak this into a patch release so this will need to be retargeted. @GuySartorelli what do you think with regard to targeting this to 5? Target 2.0 and the 5.0.0 release, or target 2 and delay until 5.1.0?

[GuySartorelli]

Should probably target 2 - we're hoping to target the release candidate for 5.0 next week so it'd be cutting it a bit close for that I reckon.

[forsdahl]

Added the changes discussed above, namely to unlink the temporary file whenever the image resource is set to null.

[GuySartorelli]

Thanks! Just one small change to make. Please also retarget this PR to the 1.13 branch (I said 2 above, but I'm okay with this being in a patch after further consideration. It is effectively a bug fix, and doesn't affect APIs or have any obvious regressions)

[GuySartorelli]

Suggested change

	unlink($this->getTempPath() ?? '');
	unlink($this->getTempPath());

If the file exists, it can't be null. I suspect you copied this from __destruct() but for context, the null coalescing operator there was added by a naive automated approach to PHP 8.1 compatibility. We don't need or want to add unnecessary null coalescing operators in manual changes.

[forsdahl]

Requested change made, and PR retargeted to 1.13

[GuySartorelli]

Thank you, looks good.

[GuySartorelli]

This will be automatically tagged as 1.13.5 once CI has finished running on it

[lerni]

I was excited when I saw this got merged. Especially since AVIF 'll be available in Edge 117, which 'll make it broadly available soon. What I noticed is, that I still get [Emergency] Uncaught Jcupitt\Vips\Exception: libvips error: /var/www/html/silverstripe-cache/www-data/interventionimage_VO3qQo.jpg: unable to open for read unix error: No such file or directory when using libvips with jonom/focuspoint. Libvips also works with FP if I comment-out unlink in __descruct() but not otherwise.

[lerni]

I've made an attempt to make focuspoint work with vips and came up with a suboptimal solution 😁 The problem is similar to this issue - the temporary file is unlinked after the first operation but focuspoint chains operations like $backend->resizeByWidth($width)->crop($cropOffset, 0, $width, $height). I would appreciate ideas on how this could be tackled other than just cloning.