Add new hook: finalize comment

NEW
Assigned to

Status

()

Bugzilla
Extensions
--
enhancement
7 years ago
2 years ago

People

(Reporter: GavinS, Assigned: GavinS)

Tracking

Details

Attachments

(1 attachment)

(Assignee)

Description

7 years ago
Discussed a bit in bug#330707.
(Assignee)

Comment 1

7 years ago
Created attachment 531119 [details] [diff] [review]
Adding finalize comment hook
Assignee: extensions → bugzilla
Attachment #531119 - Flags: review?
(Assignee)

Updated

7 years ago
Attachment #531119 - Attachment description: Addding finalize comment hook → Adding finalize comment hook

Updated

7 years ago
Attachment #531119 - Flags: review? → review?(mkanat)

Comment 2

7 years ago
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).
(Assignee)

Comment 3

7 years ago
(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

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

Okay. Looks good to me.
Attachment #531119 - Flags: review?(mkanat) → review+

Comment 5

7 years ago
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.
Attachment #531119 - Flags: review+ → review-

Comment 6

7 years ago
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.

Updated

2 years ago
See Also: → bug 726607
You need to log in before you can comment on or make changes to this bug.