655816 - Add new hook: finalize comment

Status: NEW

(Bugzilla :: Extensions, enhancement)

Description

[GavinS]

Discussed a bit in bug#330707.

Comment 1

[GavinS]

Attachment: Adding finalize comment hook

Comment 2

[Frédéric Buclin]

Shouldn't the hook be before

    $text =~ s/\0\0(\d+)\0\0/$things[$1]/eg;
    $text =~ s/$chr1\0/\0/g;

to allow more linkifications? See also bug 652663 (which will probably already add another hook near the end of the subroutine).

Comment 3

[GavinS]

(In reply to comment #2)
> Shouldn't the hook be before
> 
>     $text =~ s/\0\0(\d+)\0\0/$things[$1]/eg;
>     $text =~ s/$chr1\0/\0/g;
> 
> to allow more linkifications? See also bug 652663 (which will probably
> already add another hook near the end of the subroutine).

I'm not sure about moving it to just before the code you include -- I think it may have to move even earlier than that.

I could not see any change in behaviour with the hook before (as you suggest) or after (as in the patch)  in respect to simple markup like bold text or bullet lists.

Both those locations fail when trying to mix Markdown markup for things like headings and bug links, so I'm currently looking at moving this hook earlier in the subroutine, to just after the:

        $text = html_quote($text);

That seems to work better for me.

Comment 4

[Max Kanat-Alexander]

Comment on attachment 531119 [details] [diff] [review]
Adding finalize comment hook

Okay. Looks good to me.

Comment 5

[Max Kanat-Alexander]

Comment on attachment 531119 [details] [diff] [review]
Adding finalize comment hook

Ah yeah, looking over the above comments, we should actually investigate where this hook should really go.

If you want to understand more about what's going on here, feel free to ask me.

Comment 6

[Max Kanat-Alexander]

Probably another solution here is to put the link formatters into the loop above, as though they were themselves injected by a hook, and then allow the hooks to decide where they want their stuff to be in relation to the stuff already in the array.