View Issue Details

IDProjectCategoryView StatusLast Update
0030919mantisbtmarkdownpublic2024-03-27 20:20
Reporterhotzeplotz Assigned Todregad  
PrioritynormalSeverityminorReproducibilityN/A
Status closedResolutionfixed 
Target Version2.26.0Fixed in Version2.26.0 
Summary0030919: Markdown processing code cleanup
Description

Initially reported by @hotzeplotz in 0030791:0066865.

1. cleanup quotings

  • add an external css file, only if process_markdown is enabled.
  • remove the "__qoute()"-part from from MantisMarkdown.php
  • fix the test to match the new output without inline styles.

see https://github.com/mantisbt/mantisbt/compare/master...grummbeer:mantisbt:markdown-cleanup-01-quotings

This is related to 0022190 and https://github.com/mantisbt/mantisbt/pull/1004 (especially the last comment about emails and inline-styles).

For decades the opening arrow > has been the generally accepted character to mark a quote, mail clients take care of this and apply their own formatting, so there is no need to add inline styles to a quote.

2. cleanup tables

  • add note to Test, because the [link](/) thing makes this test highly dependent on "process_urls", which is not recognized at all.
  • remove __doTable()- stuff from MantisMarkdown.php

Parsedown provide a helpfull method where all "marked" elements accessible. Use it.

see https://github.com/mantisbt/mantisbt/compare/master...grummbeer:mantisbt:markdown-cleanup-02-tables

3. cleanup table tests

[link] (/) makes this test highly dependent on "process_urls", but this is not recognized here, "MantisMarkdown::convert_text" does not know anything about it. Testing links should have a separate test, because its not part of the "tableClassDefinition".

  • reduce the sample to match more the intended purpose of this test

see https://github.com/mantisbt/mantisbt/compare/master...grummbeer:mantisbt:markdown-cleanup-03-table-test

4. cleanup the blockHeader part

The Tests testHashLetters, testHashNumberAny and testHashLettersAny failing.

  • invert the logic, do not search for a valid markdown heading, search for a valid "buglink" instead.
  • Change regex to be more generous with the use of colons.
  • Add test for the regular expression.

The Tests testHashLetters, testHashNumberAny and testHashLettersAny are successfully again afterward.

todo: Clarify what a valid issue mention is.

| input            | is issue mention     |
|------------------|----------------------|
| `0000001`          | true                 |
| `#123 `          | true                 |
| ` #123 `         | true                 |
| `#123 `          | true                 |
| `#123 summary`   | true                 |
| `#123: summary`  | true                 |
| `#123:summary`   | true                 |
| `#123:: summary` | true but should not? |
| `#123summary`    | false                |
| `# 123:summary`  | false                |
| `# summary `     | false                |
| `# 123`          | false                |

Modify the regex to meet the mantis requirements.

see https://github.com/mantisbt/mantisbt/compare/master...grummbeer:mantisbt:markdown-cleanup-04-blockheader

5. cleanup formatting process

  • Rearrange the processing. Moving the Markdown part before ON == $s_text, return the result and do not doing further processing.
  • Make MantisMarkdown know about the plugin config and process the links and mentions within the parser context
  • mark all the methods that make use of $this->processAmpersand() as deprecated, because the output is no loger messed up.
  • remove all methods which marked as "deprecated"

Parsedown provides a usefull method called unmarkedText, which make all "unmarked" strings accessible. Seems the right place to process the links.

see https://github.com/mantisbt/mantisbt/compare/master...grummbeer:mantisbt:markdown-cleanup-05-formatting

After that and merging all parts together the whole part looks very nice and its a huge impact for user experience.

The branches are all independent but could make it a cascading series. What do you think about? I am not experienced in contributing, so this may look strange to you, but I have no idea what the right approach is.

todo

  • the tests are now failing because MantisMarkdown.php is getting the plugin config using plugin_config_get, which is not available inside the unit tests. Make it possible to pass a configuration to the MantisMarkdown?
  • emails still not proceed with markdown. hidden deep inside string_insert_hrefs make a new api function to catch them?
  • add prism to raise up the user experience.

Fixing this todos would be a really nice finishing point.

TagsNo tags attached.

Relationships

related to 0022190 closedcommunity Markdown markup should be done with CSS classes, not inline styles 
related to 0030791 closeddregad Allow adding relation type noopener/noreferrer to outgoing links 
related to 0034040 assignedcommunity Markdown processing code cleanup (part 2) 

Activities

dregad

dregad

2022-08-24 10:31

developer   ~0066941

  1. cleanup quotings => PR https://github.com/mantisbt/mantisbt/pull/1841
dregad

dregad

2022-09-03 06:04

developer   ~0066965

Last edited: 2022-09-03 06:08

PR https://github.com/mantisbt/mantisbt/pull/1841 has just been merged (fixing 0022190).

@hotzeplotz, looking forward to reviewing the follow-up PR(s) with the remaining points !

hotzeplotz

hotzeplotz

2022-09-03 21:41

reporter   ~0066967

  1. and 3. Simplify table processing => PR https://github.com/mantisbt/mantisbt/pull/1848
hotzeplotz

hotzeplotz

2023-10-15 11:41

reporter   ~0068218

Last edited: 2023-10-15 13:38

step 4. cleanup the blockHeader part => PR https://github.com/mantisbt/mantisbt/pull/1929

dregad

dregad

2024-03-13 08:05

developer   ~0068649

Fixes for steps 1, 2 and 3 were included in Release 2.26.0, so I'm resolving this issue.

Follow up on remaining points are tracked in 0034040

Related Changesets

MantisBT: master 3faaae32

2022-09-03 05:54

dregad


Details Diff
Markdown processing code cleanup, step 1

This addresses step 1 (cleanup quotings) of issue 0030919.

Merge PR https://github.com/mantisbt/mantisbt/pull/1841
Affected Issues
0022190, 0030919
mod - plugins/MantisCoreFormatting/MantisCoreFormatting.php Diff File
mod - plugins/MantisCoreFormatting/core/MantisMarkdown.php Diff File
add - plugins/MantisCoreFormatting/files/markdown.css Diff File
mod - plugins/MantisCoreFormatting/tests/MarkdownTest.php Diff File

MantisBT: master a3211026

2023-01-07 20:57

grummbeer

Committer: community


Details Diff
Markdown: simplify table processing

Overriding Parsedown's element() method which gives access to all
marked elements makes it easy to change the properties/attributes of
an element just before it is converted into HTML markup.

This addresses steps 2 and 3 (cleanup tables and tests) of issue 0030919.

Merge PR https://github.com/mantisbt/mantisbt/pull/1848

Co-authored-by: grummbeer <kde@streber24.de>
Co-authored-by: Damien Regad <dregad@mantisbt.org>
Affected Issues
0030919
mod - plugins/MantisCoreFormatting/core/MantisMarkdown.php Diff File
mod - plugins/MantisCoreFormatting/tests/MarkdownTest.php Diff File