Item572: Issues with afterAttachmentSaveHander API

Issues with afterAttachmentSaveHander API

I have encountered several issues with the afterAttachmentSaveHander API. We need to decide how to resolve them. Background: I am developing a plugin that wants to save a companion attachment. It's not possible with the current API. See Store.pm.

In this case, when an image is saved, I want to generate and save a companion thumbnail. I want this to happen automagically whenever an attachment is saved, transparently to the user. (Except that the user gets to use the thumbnail!)

Secondarily, I want default links to images to display the thumbnail, but link to the large image. Issues

• the afterSave handler is called holding the topic lock. So any attempt to call saveAttachment recursively will deadlock.
• saveAttachment closes the temporary file that (may have) used to hold the data just before calling the handler. This guarantees that the handle must reread the data all the way from the repository. This is unfortunate - on performance and atomicity grounds.
• saveAttachment does not pass the new attachment's metadata to the handler - and it hasn't written the topic text back yet either. So the handler really can't know what it's doing.
• there is no hook for the createlink option. It would be useful to be able to modify the returned link to meet coding or functional requirements. For example, to use %ATTACHMENT% or otherwise use an image in the link. (You may have seen the AttachLinkPlugin? appear...I suspect that Attach.pm could export some useful methods for it and the ImgPlugin? , but I digress.)

Options

• Implement a postAttachmentSaveHander callback - right at the final } of saveAttachment. It should pass everyting it knows.
• Provide an "already locked" flag to saveAttachment for recursive handlers - this would prevent the deadlock. But the handler would need the open topic.
• Move the tempfile unlink down to after the if( haveHanderlFor ) block.
• Fill in %attrs before calling the handler, and pass them along.
• Do something really different.
• I can do something local - the prototype simply writes directly to pub/web/topic/file

Thoughts? Takers?

-- Contributors: TimotheLitt - 21 Dec 2008

This API was deprecated in favour of the afterUploadHandler, but there is a risk that this new API may have inherited some of the issues described here, so raising from 'Enhancement' to 'Urgent' as the API must be reviewed before 1.1 is released.

• locking - looks OK to me
• new handlers use streams, and should not close streams (as documented)
• meta is now passed
• createlink is not supported - the handler is called far too late for that.

Marking as releaseable. There is still scope for improvements, but they should go through the feature proposal process.

-- CrawfordCurrie - 12 Jul 2010