330707 - Add optional support for MarkDown

Status: RESOLVED FIXED

(Bugzilla :: User Interface, enhancement, P3)

Description

[Tom Emerson]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en) AppleWebKit/417.9 (KHTML, like Gecko) Safari/417.8
Build Identifier: 

MarkDown (http://daringfireball.net/projects/markdown/) is a text-to-html converter written in Perl that is used by several blogs (e.g., WordPress). It can be used stand-alone on the command-line or embedded in a larger application.

Integrating MarkDown into BZ would be relatively straight forward, and could provide some improved formatting capabilities in comments. I expect that a BZ customized version would be needed to handle the existing comment pattern capabilities (e.g., bug xxxx, comment xxxx), but these would be trivially added.


Reproducible: Always

Steps to Reproduce:

Comment 1

[timeless]

normally people give reasons for enhancements.

what problem are you trying to solve? why do you think you have this problem?

we have reasons for generally not implementing such a thing as it leads to increased security risks.

Comment 2

[Tom Emerson]

The reason to add this kind of functionality is allow better formatting (e.g., lists of sundry types) in comment fields: it is an aesthetic concern. Using MarkDown means that the text that is stored in the database is still plain text --- markup generation is done at display time. There would be no more security issues than you currently have with the existing link generation done with comment text.

Comment 3

[Frédéric Buclin]

<LpSolit> justdave: isn't this bug WONTFIX as injecting HTML into comments is what we want to avoid?
<justdave> that sounds suspiciously like things we keep trying to avoid, yeah.

Comment 4

[Max Kanat-Alexander]

Actually, I think this is the best option for HTML comments I've ever seen suggested.

Comment 5

[Tom Emerson]

(In reply to comment #3)
> <LpSolit> justdave: isn't this bug WONTFIX as injecting HTML into comments is
> what we want to avoid?
> <justdave> that sounds suspiciously like things we keep trying to avoid, yeah.

You already inject HTML into comments...

Comment 6

[Frédéric Buclin]

(In reply to comment #5)
> You already inject HTML into comments...

We don't let users decide what to inject.

Comment 7

[Max Kanat-Alexander]

What I'd like to see is somebody to first implement this as an extension, and then if the extension works well, integrate it into Bugzilla core.

Comment 8

[GavinS]

Attachment: wip markdown extension

(In reply to comment #7)
> What I'd like to see is somebody to first implement this as an extension, and
> then if the extension works well, integrate it into Bugzilla core.

WIP in case anyone wanted to review or comment on the proposed new hook location, that I think is needed.

Comment 9

[Max Kanat-Alexander]

Comment on attachment 471302 [details] [diff] [review]
wip markdown extension

Cool! :-)

You shouldn't need the new hook, actually. You could simply replace do a regex that replaces the entire comment text in bug_format_comment, and then nothing else afterward will matter.

Comment 10

[GavinS]

(In reply to comment #9)
> You shouldn't need the new hook, actually. You could simply replace do a regex
> that replaces the entire comment text in bug_format_comment, and then nothing
> else afterward will matter.

The problem I had using that hook is that the entire comment is then stored in the list of replacement texts, and so is not subject to the rest of the quoteUrls()  processing, including quoting and linkification.

Though the extension could run the url quoting filter, I don't think it can do the rest of the useful linkification.

Thoughts?

Comment 11

[Max Kanat-Alexander]

Oh, hmm, that's interesting.

Perhaps we should factor out the normal linkification into its own subroutine, so that you could call it from your own extension as necessary. Of course, that would break compatibility with other extensions, so I'm not sure how good of an idea that would be....

Okay, so you're probably right that we need a new hook there. Perhaps call it bug_format_comment_end?

Comment 12

[Frédéric Buclin]

(In reply to comment #11)
> Okay, so you're probably right that we need a new hook there.

The 2nd hook is definitely needed. I had the same problem with GCC Bugzilla.

Comment 13

[Max Kanat-Alexander]

FWIW, there's a new-ish Markdown module on CPAN called Markdent:

  http://search.cpan.org/dist/Markdent/

It does use Moose, though.

Comment 14

[GavinS]

I've created bug#655816 to try and get the hook added

Comment 15

[Shawn Wilsher :sdwilsh]

The more I use github, the more I wish bugzilla had support for markdown too.

Comment 16

[KARASZI István]

+1 I would love it too.

Comment 17

[Sarah Gerweck]

This is indeed a real problem. There's a reason that virtually every other defect-tracking system has the ability to do formatted text these days.

Bugzilla is used by lots of us for long threads of documentation and discussion on an artifact. It is painful to have to create a wiki page somewhere just because you want your bulleted list to be readable without having to hard-wrap it. 

Honestly, monospaced plaintext just doesn't cut it these days. Even Project Gutenberg has moved to HTML….

You really don't need very much though: em, strong, code, pre, blockquote, links, and ordered & unordered lists cover 95% of the use cases. (And of course a proportional font for formatted comments.) 

If you want to get fancy, the candidates are probably the XHTML 2 list, hypertext and text modules, plus a couple key things from the structural module. Tables are a nice-to-have feature but less important than lists and emphasis.

Comment 18

[Paul Rouget [:paul]]

What should be the next step to see this happening?

Comment 19

[David Lawrence [:dkl]]

Just a matter of resources. After this quarter is over, I could look into creating a small extension that would allow this. There is a perl module available called Text::Markdown that converts markdown style text to HTML and we have the hooks in Bugzilla to manipulate comments in whatever way we choose. We would of course need to make sure this could be done in a secure manner and also not impact performance on Bugzilla rendering. Especially if a lot of the comments for a single bug have complex markdown.

dkl

Comment 20

[GavinS]

(In reply to Paul Rouget [:paul] from comment #18)
> What should be the next step to see this happening?

Sorry - I'm spectacularly failing to finish the WIP extension attached **so** long ago.

(In reply to David Lawrence [:dkl] from comment #19)
> Just a matter of resources. After this quarter is over, I could look into
> creating a small extension that would allow this. There is a perl module
> available called Text::Markdown that converts markdown style text to HTML
> and we have the hooks in Bugzilla to manipulate comments in whatever way we
> choose. We would of course need to make sure this could be done in a secure
> manner and also not impact performance on Bugzilla rendering. Especially if
> a lot of the comments for a single bug have complex markdown.

DKL: As per earlier comments here - I'm not sure the existing hooks are enough.

For the record, the WIP attached (with maybe a few minor updates I'll try (!) to attach) mostly works.

What works:

 - most of the Markdown formatting
   - in isolation
   - and in conjunction with the BZ linkification
   - in both bug comments and HTML email

The main (blocking) problem at the moment is:

  - the conflict between Markdown comment formatting and BZ comment formatting
    - may need an additional hook to disable or control the builtin-BZ formatting

On top of fixing that issue, the TODO list is:

  1. add user pref
  2. more testing (hopefully via repeatable via t/ or xt/)
  3. find somewhere to host (external or in bz/extensions)
  4. look at where-else it may be used
       - component/keyword etc. descriptions
       - displaying text attachments (all, or via content-type or inspection)

Comment 21

[PhoneixSegovia]

Is the extension still valid? Can I test it? Is any progress on comment 20 problems?

Comment 22

[GavinS]

(In reply to PhoneixSegovia from comment #21)
> Is the extension still valid? Can I test it? Is any progress on comment 20
> problems?

I have uploaded the source for the latest version to GitHub. Please test it if you can. The quoted comment conflict still exists resulting in slightly messy quoted comments.

https://github.com/columbusmonkey/Bugzilla-Extension-Markdown

Comment 23

[PhoneixSegovia]

I have tested it.

It run well except for links and other few used things (I have created issues in github to control them).

Comment 24

[David Lawrence [:dkl]]

I would like to see this integrated with BMO, providing we can work on the remaining issues and guarantee current level of security and performance. We are neck-deep in 4.2 roll out at the moment as well as some other quarterly goals, but hopefully we can take a look at this not too long from now.

dkl

Comment 26

[Dan Dascalescu]

Any updates as of March 2014? I see a "Preview" tab at https://bugzilla.mozilla.org/show_bug.cgi?id=330707, but it's unclear what the point of the Preview is, if there's no formatting?

Is there a formatting help page that should be linked to?

Comment 27

[Andre Klapper]

Dan: See bug 40896 to discuss and understand "Preview".

Comment 28

[Manish Goregaokar [:manishearth]]

It previews magic links (eg: bug 330707 , bug 12345 comment 2, etc) , as well as


> blockquotes

Comment 29

[Sudheesh Singanamalla (:ShellHacker)]

I find myself using the markdown syntax everytime I file a bug or comment on one. Would really love if this feature is implemented. Makes things so much easier.
+1

Comment 30

[Ryan]

I wrote an extension to provide markdown rendering for bugzilla 4.4.4. It renders all markdown client side using javascript.

[https://github.com/ryan-sandy/bugzilla-markdownjs](https://github.com/ryan-sandy/bugzilla-markdownjs)

...it's my first bugzilla extension, so constructive criticism would be nice.

Comment 31

[Koosha KM]

Attachment: GSoC Patch - Code part v1.0

Comment 32

[David Lawrence [:dkl]]

Comment on attachment 8469324 [details] [diff] [review]
GSoC Patch - Code part v1.0

Review of attachment 8469324 [details] [diff] [review]:
-----------------------------------------------------------------

::: Bugzilla/Install/DB.pm
@@ +708,5 @@
>      # 2013-01-02 [e-mail] - Bug 824361
>      _fix_longdescs_indexes();
>  
> +    $dbh->bz_add_column('longdescs', 'needs_markdown',
> +                        {TYPE => 'BOOLEAN', NOTNULL => 1, DEFAULT => 'FALSE'});

Nit: Move this down past the last DB update and before the "*** A B O V E ***" commented lines as other changes have been made since. 

Also add a comment above your update such as 

# 2014-08-08 [e-mail] - Bug 330707

::: Bugzilla/Template.pm
@@ +301,4 @@
>      return $text;
>  }
>  
> +sub processComment {

Nit: Lets change this to filterComment instead as that more reflects what is happening.

@@ +303,5 @@
>  
> +sub processComment {
> +    my ($text, $bug, $comment) = @_;
> +
> +    return $text unless $text;

return unless $text;

@@ +308,5 @@
> +
> +    if (defined $comment && $comment->needs_markdown && Bugzilla->feature('markdown')) {
> +        require Bugzilla::Markdown;
> +        my $m = Bugzilla::Markdown->new();
> +        return $m->markdown($text);

This will call require and new() potentially very many times for bugs with a lot of comments. Lets create a method in Bugzilla.pm called markdown() that caches the markdown object.

Bugzilla.pm:

sub markdown {
    require Bugzilla::Markdown;
    return $_[0]->request_cache->{markdown} ||= Bugzilla::Markdown->new();
}

And the code in Template.pm will be:

sub filterComment {
    my ($text, $bug, $comment) = @_;
    return unless $text;
    if (Bugzilla->feature('markdown')
        && defined $comment
        && $comment->needs_markdown)
    {
        return Bugzilla->markdown->markdown($text);
    }
    else {
        return quoteUrls(@_);
    }
}

@@ +814,5 @@
> +            processComment => [ sub {
> +                                    my ($context, $bug, $comment, $user) = @_;
> +                                    return sub {
> +                                        my $text = shift;
> +                                        return processComment($text, $bug, $comment, $user);

the code in processComment() could actually just be inside here and not need the extra subroutine as it is not that much extra code and isn't called from anywhere else that I can tell.

::: template/en/default/bug/comment.html.tmpl
@@ +36,5 @@
>    </div>
>  [% END %]
> +
> +[% IF feature_enabled('markdown') AND user.settings.use_markdown.is_enabled %]
> +  <br>

Nit: remove <br> and replace with <div> with 0 margin and padding.

::: template/en/default/bug/edit.html.tmpl
@@ +181,4 @@
>        [% IF bug.alias != "" %]
>          (<span id="alias_nonedit_display">[% bug.alias FILTER html %]</span>) 
>        [% END %]
> +      - <span id="short_desc_nonedit_display">[% bug.short_desc FILTER processComment(bug) %]</span>

Leave this as quoteUrls as we should never need markdown in the summary of a bug.

::: template/en/default/global/setting-descs.none.tmpl
@@ +44,4 @@
>     "requestee_cc"                     => "Automatically add me to the CC list of $terms.bugs I am requested to review",
>     "bugmail_new_prefix"               => "Add 'New:' to subject line of email sent when a new $terms.bug is filed",
>     "possible_duplicates"              => "Display possible duplicates when reporting a new $terms.bug", 
> +   "use_markdown"                     => "Use Markdown engine when I submit a new $terms.comment (requires Text::Markdown)",

Nit: Allow use of Markdown syntax when submitting a new $terms.comment.

After more thought on this, the end user does not need to know it is an 'engine' or what the Perl module is named. The admin will know to install Text::Markdown when they run checksetup.pl.

::: template/en/default/setup/strings.txt.pl
@@ +102,4 @@
>      feature_xmlrpc            => 'XML-RPC Interface',
>      feature_detect_charset    => 'Automatic charset detection for text attachments',
>      feature_typesniffer       => 'Sniff MIME type of attachments',
> +    feature_markdown          => 'Markdown text engine',

Nit: Markdown syntax support for comments

Comment 33

[Koosha KM]

Attachment: GSoC Patch - code part v1.1

https://github.com/koosha--/bugzilla/commit/0fe0f9a17493713f3513a3fea62b7ac5e74ae0b3

Comment 34

[David Lawrence [:dkl]]

Comment on attachment 8470292 [details] [diff] [review]
GSoC Patch - code part v1.1

Review of attachment 8470292 [details] [diff] [review]:
-----------------------------------------------------------------

I wanted glob to also look over this as he originally wrote the proposal and some of the comments are based on a meeting we had recently.

Email rendering also needs to use Markdown when appropriate similar to the UI so change quoteUrls to filterComment in
template/en/default/email/bugmail.html.tmpl.

The "Preview" tab also needs to work with means some minor changes to Bug.render_comment in Bugzilla/WebService/Bug.pm to call filterComment
instead of quoteUrls. Also filterComment in Template.pm will need some adjustment to deal with allowing markdown to comment text that do
not yet have an associated comment object which Bug.render_comment will not. Ideally the ajax call to Bug.render_comment should include the
markdown checkbox value in the parameters, then Bug.render_comment can call Bugzilla->markdown->markdown or alter filterComment in Template.pm
to work also for text where $comment is undefined. Meaning if $comment is undefined, and the user wants Markdown and/or the system default is
Markdown to be enabled, then allow Markdown rendering. If $comment is defined then we of course look at $comment->is_markdown.

Maybe you can think of a better solution for the above issue.

Also glob mentioned we should probably tie the Text::Markdown to a specific known working version in Bugzilla/Install/Requirements.pm
such as version 1.000031 which I have successfully tested with.

Sorry for the long review but we still have a little time to make the best it can be. Everything else is working great.

::: Bugzilla.pm
@@ +396,4 @@
>      # there. Don't rely on it: use Bugzilla->user->login instead!
>  }
>  
> +sub markdown {

t/011pod.t ........... 175/363 
#   Failed test 'Bugzilla.pm POD coverage is 97%. Undocumented methods: markdown'
#   at t/011pod.t line 104.

Add this to the POD docs below such as:

=item C<markdown>

The current L<Markdown|Bugzilla::Markdown> object, to be used for Markdown rendering.

::: Bugzilla/Bug.pm
@@ +2372,5 @@
>          # there are lots of things that want to check if we added a comment.
>          $self->add_comment($params->{'comment'}->{'body'},
>              { isprivate => $params->{'comment'}->{'is_private'},
> +              work_time => $params->{'work_time'},
> +              needs_markdown => $params->{'comment'}->{'needs_markdown'} });

s/needs_markdown/is_markdown/

::: Bugzilla/Comment.pm
@@ +42,4 @@
>      already_wrapped
>      type
>      extra_data
> +    needs_markdown

s/needs_markdown/is_markdown/

@@ +177,4 @@
>  sub bug_id      { return $_[0]->{'bug_id'};    }
>  sub creation_ts { return $_[0]->{'bug_when'};  }
>  sub is_private  { return $_[0]->{'isprivate'}; }
> +sub needs_markdown { return $_[0]->{'needs_markdown'}; }

Change to 'is_markdown' to be more uniform with other boolean values used in other parts of the code. Such as attachments have is_obsolete, is_patch, etc.

@@ +275,4 @@
>  sub set_is_private  { $_[0]->set('isprivate',  $_[1]); }
>  sub set_type        { $_[0]->set('type',       $_[1]); }
>  sub set_extra_data  { $_[0]->set('extra_data', $_[1]); }
> +sub set_needs_markdown { $_[0]->set('needs_markdown', $_[1]); }

Change to 'set_is_markdown'

t/011pod.t ........... 175/363 
#   Failed test 'Bugzilla/Comment.pm POD coverage is 96%. Undocumented methods: set_needs_markdown'
#   at t/011pod.t line 104.

Add POD below for the setting method.

@@ +526,5 @@
>  
> +=item C<needs_markdown>
> +
> +C<boolean> Whether this comment needs the L<Markdown|Bugzilla::Markdown> engine
> +to be processed.

C<boolean> Whether this comment needs L<Markdown|Bugzilla::Markdown> rendering to be applied.

::: Bugzilla/DB/Schema.pm
@@ +397,5 @@
>                                  DEFAULT => 'FALSE'},
>              type            => {TYPE => 'INT2', NOTNULL => 1,
>                                  DEFAULT => '0'},
> +            extra_data      => {TYPE => 'varchar(255)'},
> +            needs_markdown  => {TYPE => 'BOOLEAN', NOTNULL => 1, DEFAULT => 'FALSE'}

s/needs_markdown/is_markdown/

::: Bugzilla/Install/DB.pm
@@ +726,5 @@
>      # 2014-07-27 [e-mail] - Bug 1044561
>      _fix_user_api_keys_indexes();
>  
> +    # 2014-08-08 [e-mail] - Bug 330707
> +    $dbh->bz_add_column('longdescs', 'needs_markdown',

s/needs_markdown/is_markdown/

::: Bugzilla/Template.pm
@@ +301,4 @@
>      return $text;
>  }
>  
> +sub filterComment {

I do not see where this needed anymore.

@@ +816,5 @@
> +                                        return unless $text;
> +
> +                                        if (Bugzilla->feature('markdown')
> +                                            && defined $comment
> +                                            && $comment->needs_markdown) {

Nit: drop { down to next line.

Change to $comment->is_markdown

As mentioned in my comment for the user preferences text change, we also need to look at the user's preference setting here. If the user has turned 'off' markdown support for comments, we want the non-markdown rendered text to appear. If the user is not logged in, then the system default take effect.

::: process_bug.cgi
@@ +249,4 @@
>      $set_all_fields{comment} = {
>          body       => scalar $cgi->param('comment'),
>          is_private => scalar $cgi->param('comment_is_private'),
> +        needs_markdown => $needs_markdown,

s/needs_markdown/is_markdown/

::: template/en/default/global/setting-descs.none.tmpl
@@ +44,4 @@
>     "requestee_cc"                     => "Automatically add me to the CC list of $terms.bugs I am requested to review",
>     "bugmail_new_prefix"               => "Add 'New:' to subject line of email sent when a new $terms.bug is filed",
>     "possible_duplicates"              => "Display possible duplicates when reporting a new $terms.bug", 
> +   "use_markdown"                     => "Allow use of Markdown syntax when submitting a new $terms.comment"

Change to "Enable Markdown support for comments" as we want this parameter to also enable/disable markdown for comments made by others, not just the bug changer.

Comment 35

[Koosha KM]

Attachment: User doc patch - v1.0

Comment 36

[David Lawrence [:dkl]]

Comment on attachment 8472544 [details] [diff] [review]
User doc patch - v1.0

Review of attachment 8472544 [details] [diff] [review]:
-----------------------------------------------------------------

With docs reviews, one way to do the review is to post some content containing some changes I feel may make sense so here is
my content with those changes. Please take a look and see how it looks to you.

+.. _markdown:
+
+Markdown
+--------
+
+Markdown lets you write your comments in a structured plain-text format and
+have your comments generated as HTML. For example, you may use Markdown for
+making a part of your comment look italic or bold in the generated HTML. Bugzilla
+supports most of the structures defined by `standard Markdown <http://daringfireball.net/projects/markdown/basics>`_.
+but does NOT support inline images and inline HTML.
+
+Additionally, three Github Flavored Markdown features are supported.
+
+- `Multiple underscores in words <https://help.github.com/articles/github-flavored-markdown#multiple-underscores-in-words>`_
+
+- `strikethrough <https://help.github.com/articles/github-flavored-markdown#strikethrough>`_
+
+- `fenced code blocks <https://help.github.com/articles/github-flavored-markdown#fenced-code-blocks>`_
+
+To use the Markdown feature, make sure that ``Enable Markdown support for comments`` is set to ``on``
+in your :ref:`userpreferences` and that you also check the ``Use Markdown for this comment`` option below
+the comment box when you want to submit a new comment.
+

Comment 37

[Koosha KM]

Looks fine to me. Should I submit a new patch for the doc?

Comment 38

[Koosha KM]

Attachment: Patch - Code + Doc - v2.0

Some points:

For the "Preview" tab, I defined a new filter. I intended to make an object of Component on the fly before calling filterComment but, apparently, that is not possible to make an object without persisting it. Also, I couldn't think of a solution for filterComment that honors both user preferences and comment object (even when comment is not defined) and yet be working in all cases.

For Text::Markdown module, I chose version 1.0.26 as it is the current version in Debian. I made a diff of them and saw that the code of this version is exactly the same as 1.000031 but only the version differs!

The patch also includes the updated user doc.

You can see the changes of the code here: https://github.com/koosha--/bugzilla/commit/2499c47f8cdbabf09dca60444485506f6c613c99

Comment 39

[Frédéric Buclin]

Comment on attachment 8473128 [details] [diff] [review]
Patch - Code + Doc - v2.0

>diff --git a/Bugzilla/Template.pm b/Bugzilla/Template.pm

>+            filterMarkdown => [ sub {
>+            filterComment => [ sub {

For consistency with other TT filters, couldn't you name these filters foo_bar instead of fooBar? Also, there is no need to name them filterFoo, as there are already declared as filters. Why not simply 'markdown'? [% foo FILTER markdown %] is better than [% foo FILTER filterMarkdown %]. Also, I think you don't need both filters. filterMarkdown is called only once, from Bugzilla/WebService/Bug.pm. You could simply write:

  my $tmpl = $markdown ? '[% text FILTER markdown(bug, {is_markdown => 1}) %]' : '[% text FILTER markdown(bug) %]';

and refactor your filter a bit to accept both a Bugzilla::Comment object and a hashref. It would be much easier to understand than having two distinct filters.

Comment 40

[David Lawrence [:dkl]]

(In reply to Koosha KM from comment #37)
> Looks fine to me. Should I submit a new patch for the doc?

Yes please for tracking purposes mainly.

dkl

Comment 41

[David Lawrence [:dkl]]

Comment on attachment 8473128 [details] [diff] [review]
Patch - Code + Doc - v2.0

Review of attachment 8473128 [details] [diff] [review]:
-----------------------------------------------------------------

I concur with LpSolit and is partly my fault as I requested the 'filterComment' name. Let's call it simply 'markdown' in all places where 'filterComment' is used.

Unless otherwise noted, the rest looks good to me.

::: Bugzilla/Markdown.pm
@@ +7,5 @@
> +
> +package Bugzilla::Markdown;
> +
> +use 5.10.1;
> +use strict;

add:

use warnings;

::: Bugzilla/Template.pm
@@ +819,5 @@
> +                                },
> +                                1
> +                              ],
> +
> +            filterComment => [ sub {

As mentioned before these should be combined to one 'markdown'.

::: Bugzilla/WebService/Bug.pm
@@ +331,5 @@
>      Bugzilla->switch_to_shadow_db();
>      my $bug = $params->{id} ? Bugzilla::Bug->check($params->{id}) : undef;
>  
> +    my $markdown = $params->{markdown} ? 1 : 0;
> +    my $tmpl = $markdown ? '[% text FILTER filterMarkdown %]' : '[% text FILTER quoteUrls(bug) %]';

text FILTER markdown

::: process_bug.cgi
@@ +239,4 @@
>      $set_all_fields{comment} = {
>          body       => scalar $cgi->param('comment'),
>          is_private => scalar $cgi->param('comment_is_private'),
> +        is_markdown => $is_markdown,

Nit; line up the other =>

::: skins/standard/global.css
@@ +845,5 @@
>  
> +.bz_new_line {
> +    padding: 0;
> +    margin: 0;
> +}

Remove the above and change the #comment css rule to:

#comment {
    margin: 0;
}

With that change, the checkbox looks good underneath the comment textarea. Not sure why it was originally set to 1em margin-bottom but doesn't seem to be needed anymore.

::: t/004template.t
@@ +88,5 @@
>              wrap_comment => sub { return $_ },
>              none      => sub { return $_ } ,
>              ics       => [ sub { return sub { return $_; } }, 1] ,
> +            filterComment => sub { return $_ } ,
> +            filterMarkdown => sub { return $_ } ,

single filter called 'markdown'

::: template/en/default/bug/comment.html.tmpl
@@ +36,5 @@
>    </div>
>  [% END %]
> +
> +[% IF feature_enabled('markdown') AND user.settings.use_markdown.is_enabled %]
> +  <div class="bz_new_line"></div>

I would just remove this as far as I can tell it makes no change visually. Instead put the markdown checkbox inside a <div> like this:

  <div id="comment_markdown">
    <input type="checkbox" name="use_markdown" id="use_markdown" value="1"
    [% "checked=\"checked\"" IF user.settings.use_markdown.value == 'on' %] >
    <label id="use_markdown_label" for="use_markdown">Use Markdown for this [% terms.comment %]</label>
  </div>

Comment 42

[Koosha KM]

Attachment: patch - v2.1 (Code + Doc)

Latest changes: https://github.com/koosha--/bugzilla/commit/f8582ac19648b815e1f5026dac95cda6e985c4c3

Comment 43

[Koosha KM]

Attachment: patch - v2.1 (Code + Doc)

Comment 44

[Frédéric Buclin]

Comment on attachment 8474028 [details] [diff] [review]
patch - v2.1 (Code + Doc)

>diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm

>+    $params->{is_markdown} = 0
>+      unless defined $params->{is_markdown} && $params->{is_markdown} eq '1';

Much simpler to write: $params->{is_markdown} ||= 0; (I guess you don't care what the true value is as that's the job of the checker).

Comment 45

[Koosha KM]

(In reply to Frédéric Buclin from comment #44)
> Comment on attachment 8474028 [details] [diff] [review]
> patch - v2.1 (Code + Doc)
> 
> >diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
> 
> >+    $params->{is_markdown} = 0
> >+      unless defined $params->{is_markdown} && $params->{is_markdown} eq '1';
> 
> Much simpler to write: $params->{is_markdown} ||= 0; (I guess you don't care
> what the true value is as that's the job of the checker).

Yes, you're right. But, I just wanted to follow the conventions of the statements above it to look more consistent.

Comment 46

[Frédéric Buclin]

(In reply to Koosha KM from comment #45)
> Yes, you're right. But, I just wanted to follow the conventions of the
> statements above it to look more consistent.

Then the code will always look oldish. :) Now that we require Perl 5.10.1, we can even write $foo //= 'bar' instead of $foo = 'bar' unless defined $foo. This wasn't possible with Perl 5.8.x, which explains why we didn't use such tricks in the past (including Bugzilla 4.4).

Comment 47

[David Lawrence [:dkl]]

Comment on attachment 8474028 [details] [diff] [review]
patch - v2.1 (Code + Doc)

Review of attachment 8474028 [details] [diff] [review]:
-----------------------------------------------------------------

I feel the below points can be fixed on checkin. Everything looks good to me now. r=dkl

::: Bugzilla/Bug.pm
@@ +691,5 @@
>        unless defined $params->{rep_platform};
>      # Make sure a comment is always defined.
>      $params->{comment} = '' unless defined $params->{comment};
> +    $params->{is_markdown} = 0
> +      unless defined $params->{is_markdown} && $params->{is_markdown} eq '1';

$params->{is_markdown} ||= 0;

::: template/en/default/bug/comment.html.tmpl
@@ +35,5 @@
>      <pre id="comment_preview_text" class="bz_comment_text"></pre>
>    </div>
>  [% END %]
> +
> +[% IF feature_enabled('markdown') AND user.settings.use_markdown.is_enabled %]

Didn't notice this before but *.is_enabled is only relevant to template/en/default/account/prefs/settings.html.tmpl so here we need to use value == 'on' like this:

[% IF feature_enabled('markdown') AND  user.settings.use_markdown.value == 'on' %]

Comment 48

[:glob ✱]

please hold off on committing this; i would like to review the implementation prior to it landing.

Comment 49

[:glob ✱]

ok, let's land this as part of 5.0!

i've filed bug 1059684 and bug 1059685 to track some minor changes i'd like to see prior to this feature's release.


thank-you koosha for the work :)

Comment 50

[David Lawrence [:dkl]]

Thanks again Koosha!

To ssh://[e-mail]/bugzilla/bugzilla.git
   8234603..ec5caa5  master -> master

Comment 51

[David Lawrence [:dkl]]

This has been reverted for the 5.0 branch. As well as the other dependent bugs that were committed to the 5.0 branch all in one patch. I have left everything in trunk however so any bugs found against markdown will be filed separately against trunk.

To ssh://[e-mail]/bugzilla/bugzilla.git
   fc62fd4..f5fc4a6  5.0 -> 5.0

dkl

Comment 53

[Marc Schumann [:Wurblzap]]

The reversal hits installs which had MarkDown (Bugzilla 4.5.6) and now move to 5.0rc: all comments which were created using MarkDown are now flat comments. This wouldn't be so bad if they kept their MarkDown flag, because they'd pick it up again (and look sane again) after upgrading to a later Bugzilla having MarkDown. So, can we please put the table column back in for 5.0 so that the flag is being kept?

Comment 54

[Frédéric Buclin]

(In reply to Marc Schumann [:Wurblzap] from comment #53)
> The reversal hits installs which had MarkDown (Bugzilla 4.5.6)

4.5.6 was a development snapshot. I guess you didn't use it for production, did you? ;)

Comment 55

[Marc Schumann [:Wurblzap]]

Yes I do use development snapshots for production, Bugzilla usually is stable enough so that I can iron out the wrinkles early on. Otherwise little bugs like bug 1073590 or bug 1047405 wouldn't be fixed before 5.0 release, by the way.

Well, anyway, that sounds like a no; fine, I'll keep the column locally.

Comment 56

[Dave Miller [:justdave]]

If you already had people using this, and you weren't being bit by the side effects that caused it to be backed out, I'd suggest re-applying the patch locally when updating.