509027 - Add a hook in Bugzilla::Attachment::_check_data()

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, enhancement)

Description

[Frédéric Buclin]

Required for bug 480986

Comment 1

[Frédéric Buclin]

Attachment: patch, v1

Comment 2

[Max Kanat-Alexander]

Comment on attachment 393188 [details] [diff] [review]
patch, v1

>Index: Bugzilla/Attachment.pm
>+        # The hook is only allowed to alter the content of the attachment,
>+        # and edit its filename and mime type.


  Hmm. I can imagine that some hooks might want to alter ispatch, etc. I think it'd be safe to pass all of $params. In particular I'm sure some extensions would like to know if they're getting passed a URL, a patch, etc.

  Also, it might be good to also allow extensions to modify store_in_file here, so _check_data might need to be re-worked to allow that.

  (In fact, the "We don't compress BMP attachments stored locally" comment above here doesn't totally make sense--people might want to compress them to save loading time, too.)

>+=head2 attachment-process_data

>+=item C<attributes> - A hashref with two keys: 'filename' and 'mimetype'.

  Those should be explained further. Also, you can wrap them in C<> instead of in single-quotes.

>Index: extensions/example/code/attachment-process_data.pl

>+# Make sure images have the correct extension.

  Note that you've commented things out so that the extension doesn't actually modify anything if enabled. (So that people understand why there are commented-out lines.)

Comment 3

[Frédéric Buclin]

(In reply to comment #2)
>   Hmm. I can imagine that some hooks might want to alter ispatch, etc. I think
> it'd be safe to pass all of $params. In particular I'm sure some extensions
> would like to know if they're getting passed a URL, a patch, etc.
> 
>   Also, it might be good to also allow extensions to modify store_in_file here,
> so _check_data might need to be re-worked to allow that.

I told you on IRC why I didn't pass all params to the hook, and why I didn't care about other params for now. You are requesting something which is not the topic of this bug.

Comment 4

[Max Kanat-Alexander]

I don't recall what you said on IRC about why you're not passing the other params to the hook.

Hooks *are* designed for specific use cases, but if we can make them flexible without some downside, I don't see why not to just make them flexible. For most hooks that are inside of create() or something like that, we pass them everything we can, as long as we're relatively sure it will always be available at that point in the code (even if the code moves elsewhere). Seems like it would be nice to do that here, too, if we can.

Comment 5

[Frédéric Buclin]

(In reply to comment #4)
> I don't recall what you said on IRC about why you're not passing the other
> params to the hook.

I said a hook inside _check_data() was required because the attachment content is not available in create(). You can have a filehandle pointing to the attachment or the content of the attachment itself, it depends.

I also said we should have two hooks. The one I want in this bug, and another one in create(), which is not the topic of this bug. Also, keep in mind that the hook must be called before the validators, as you shouldn't bypass them, which is why I only pass the MIME type and the filename to the hook in _check_data().

Also, I think it's better for now to implement the hook as suggested here and then refactor Attachment.pm a bit if possible to make the hook also available for attachments stored locally than to do the opposite. I first want to see Image::Magick going away. We can update the POD for the hook as soon as the refactor is done.

Comment 6

[Max Kanat-Alexander]

(In reply to comment #5)
> Also, I think it's better for now to implement the hook as suggested here and
> then refactor Attachment.pm a bit if possible to make the hook also available
> for attachments stored locally than to do the opposite.

  Okay. Should we maybe be passing in a "params" and "data" argument instead, then?

Comment 7

[Frédéric Buclin]

Attachment: patch, v2

OK, I managed to refactor Attachment.pm to let the hook interact with all attachments, even if they are stored locally rather than in the DB (an even better proof of concept will come with my updated patch for the BMP -> PNG extension). I now also pass $params to the hook as _check_data() is now the first validator to be called, and so validators will catch illegal changes made by the extension.

Comment 8

[Max Kanat-Alexander]

Comment on attachment 394196 [details] [diff] [review]
patch, v2

Looks basically fine. :-) Go ahead and check it in. :-)

Comment 9

[Frédéric Buclin]

Checking in Bugzilla/Attachment.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Attachment.pm,v  <--  Attachment.pm
new revision: 1.64; previous revision: 1.63
done
Checking in Bugzilla/Hook.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Hook.pm,v  <--  Hook.pm
new revision: 1.28; previous revision: 1.27
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/extensions/example/code/attachment-process_data.pl,v
done
Checking in extensions/example/code/attachment-process_data.pl;
/cvsroot/mozilla/webtools/bugzilla/extensions/example/code/attachment-process_data.pl,v  <--  attachment-process_data.pl
initial revision: 1.1
done

Comment 10

[Max Kanat-Alexander]

Added to the release notes in bug 547466.