Support additional extensions

I'm migrating my personal site from Drupal 7 to 11, and adopted this module instead of https://www.drupal.org/project/markdown → .

Footnotes not being supported is a massive surprise. Per https://commonmark.thephpleague.com/2.4/extensions/footnotes/, this is trivial to support.

The absence of this makes me want to switch back to the Markdown module. But then there's 🐛 Subformstate incorrect interface error Active and 📌 Automated Drupal 11 compatibility fixes for markdown Active that are ostensibly even bigger problems to tackle 😬🫠

(#3 allows me to gradually move all my writings over to Standard's Basic HTML/Full HTML combined with https://www.drupal.org/project/footnotes → ^4. I'll keep using this module for user comments though.)

@wim-leers check out my MR, I've added optional footnote support.

I agree with you that we should allow the user to access CommonMark's additional extensions. As I've done it with the footnotes, it would be easy to add.

It would also be good for markdown_easy to allow you to configure CommonMark's settings. For example, changing the default footnote hrefs (which I had to do because drupal breaks the footnote linking, since it filters out id's with colons in them).

I've update the MR to address the things I noted above as well as I improved one of the functional tests to actually test the footnote extension.

I'm still on the fence about whether or not the Footnotes extension should be enabled by default or not seeing how there are additional tags that have been added to the "Limit allowed HTML tags and correct faulty HTML" configuration to support footnotes.

Regardless, once this is merged, I'll need to remember to add something to the release notes letting folks know that if they're upgrading and enable the Footnotes extension, they'll need to add the following to the "Limit allowed HTML tags and correct faulty HTML" config: id to the existing <li> as well as <sup id>.

I am amenable to adding additional extensions, but I really want to keep this module dead-simple to use - perhaps that means enabling and configuring extensions by default without giving users the option to enable/disable them in the filter's settings form.

Thoughts?

-mike

Personally, I use markdown a lot for other stuff, and I found it quite jarring that I couldn't use footnotes out of the box (hence why I made the MR). I'm not fussed whether its enabled by default or not though.

In terms of keeping the module easy to use, how about keeping the additional extensions hidden behind an "advanced" section in the settings form?

I talked this over with some folks during DrupalEasy Office Hours and I'm leaning towards doing the following:

1. Remove the option to enable Footnotes.
2. Add (at least) a new "flavor", calling it something like "GitHub flavored+" and including Footnotes and maybe a few other commonly-used extensions (I'm open to suggestions.) If we go this route, then I may want to consider automatically updating the "Limit allowed HTML" config based on the flavor selected.

The goal is to both keep the codebase of this module as simple as possible (for easy updates) as well as keeping its implementation as easy as possible for end users.

Thoughts?

-mike

As I mentioned in my previous comment, I didn't want to go down the road of adding additional configuration to the Markdown Easy so I decided to remove the Footnotes extension option and add a new "Markdownpalooza" flavor that includes both the Footnotes extension as well as the Description lists extension. (Full list of all possible extensions provided by the CommonMark library this module is already using.)

I believe that adding this functionality this way keeps my mission of this module intact: to be easy-to-use and easy-to-maintain.

Summarizing the three available flavors:

1. Existing flavor "Standard Markdown" is comprised of only the CommonMarkCoreExtension.
2. Existing flavor "GitHub-flavored Markdown" is comprised of the GithubFlavoredMarkdownExtension which is the same as CommonMarkCoreExtension plus the following extensions: Autolinks, Disallowed Raw HTML, Strikethrough, Tables, and Task Lists.
3. New flavor "Markdownpalooza" is comprised of GithubFlavoredMarkdownExtension plus the Description list extension and the Footnotes extension.

Some additional thoughts:

1. I am not against adding addition extensions to Markdownpalooza, but after going through the list of available extensions, I didn't see much value in any of the other available extensions.
2. I'm not 100% in love with the name Markdownpalooza, and am open to suggestions. My only criteria is that I'd like a name that is a bit fun.

Additional changes in this MR:

1. I added the relevant Footnotes and Description list HTML tags to the default configuration of the Markdown text format that is installed with this module.
2. I added test support for the new Markdownpalooza extensions.
3. I renamed the "Important" information when configuring the Markdown Easy text filter to "Tips" and added some additional information.
4. I added a new hook_markdown_easy_environment_modify that will allow folks to add additional extensions via a small custom module. Once this MR is merged, I'll update https://www.drupal.org/docs/extending-drupal/contributed-modules/contrib... → with an example of how to use it. I'm happy to provide issue credit for anyone who wants to help me with this.
5. As this is a fairly significant addition to the module, once merged, I'll be releasing a 1.1.0 version.
6. Similarly, once 1.1.0 is released, I'll be reviewing and updating all of the documentation (including the README.md). I'm happy to provide issue credit for anyone who wants to help me with this.

Thoughts? Feedback?

thanks,
-mike

Seems good to me.

Possible new flavor names suggested by DrupalEasy alumni:

• Markdown Goulash
• Markie Markdown and the Funky Bunch
• Markdown Loaded
• Markdown Madness

We also discussed the possibility of adding additional extensions like Table of Contents, but the concern is that they're always on, and not enable-able (or disable-able) on a per entity basis.

-mike

Another couple of ideas:

• Markdown Smörgåsbord
• Markdown Extras

After much discussion and difficulty in (my) figuring out the correct spelling, I'm going to go with "Markdown Smörgåsbord".

-mike

I've just updated this fork with the following changes:

1. Changed the name of the new flavor to "Markdown Smörgåsbord"
2. Added a new hook_markdown_easy_environment_modify() that will allow folks to add additional extensions. I also added a supporting test.
3. (scope creep alert!) While working on the new hook, I discovered that the way I implemented hook_markdown_easy_config_modify() was bad. So very bad (because it would force the developer to recreate the entire converter - including the extensions - in the hook's implementation.) So, I changed the hook to fire a bit earlier (and this is the breaking part) I changed its input parameter from the $converter to just the $config array. This is a breaking change (so sorry!) for anyone who has already implemented this hook. But, the code is much better for the long haul now. Once this is merged, I will update the documentation (I have already updated the README in this issue fork.)

I'm going to let this sit for a week or so for feedback before merging. Will dance for feedback.

-mike

I think the change looks good.

I did a manual test and verified the Markdown Smörgåsbord flavor was available. When selected, I was able to use the footnote markdown. Switching back the GitLab flavor caused the footnotes to not render, as expected.

[ Note, it would appear that GitHub does now support footnotes. However, I think you stick to the GitHub markdown provided by Common Markdown. It might be worth opening an issue for that library, though. ]

I like the implementation of the new hook.

As the hook change is a breaking change, should you consider a major version change (to 2.0.0) when releasing this?

Moving to RTBC.

I just found and fixed some issues in the markdown_easy.api.php file as well as some code comments related to the markdown_easy hooks.

-mike

I want to consider another change to this MR before merging...

Currently, the Markdown Easy module strongly suggests (requires, mostly) that the “Convert line breaks into HTML” filter be used in conjunction with the Markdown Easy text filter.

There are many potential use cases where it would be advantageous for the Markdown parser to completely own the HTML, and not rely on the “Convert line breaks into HTML” filter at all. So, I would like to look into changing the default config for the two new flavors introduced in this issue so that they do not require the “Convert line breaks into HTML” filter. This way, if someone is copy/pasting Markdown from another source, the output is consistent.

If we make this change, the module documentation will need to be updated as well (I just updated it with the hook changes introduced in this issue.)

Finally, once merged, as @lostcarpark suggested, I'll tag and release a 2.0.0 version of the module.

Thoughts?
-mike

I started reviewing the module and testing this branch. I came across a few small things that could potentially be included in this branch. If you prefer, I can create a separate merge request instead. For now, I’ll go ahead and upload a patch (old school).

@dries - thanks. "small things" added.

Also - after chatting with @lostcarpark about comment 18, he convinced me that we should move it into a new issue. So, once this issue is RTBCd, I'll create a 2.0.x branch, merge this issue into that, and then we can work on the future issue from comment 18.

-mike

Thank you, that sounds like a plan.

And apologies for breaking your tests. I'm not used to seeing ->render() chained to a t() function. I figured it was safe to remove, but apparently not. I just grep'ed Core, and it never occurs there, except in a couple of specific locale translation tests. It still feels a bit off to me, so maybe it's worth investigating at some point.

Instead of using t()->render(), try t()->toString() or simply (string) t()? Calling render() triggers the rendering system, which seems unnecessary in this context. Casting to a string avoids invoking the render pipeline. I have not tested this myself yet. Sorry for going slightly off-topic. I didn't mean to sidetrack the main issue here.

After a lot of frustration and tinkering the solution turned out to be annoyingly straightforward - just strip the HTML out of the assert statements!

I have removed the ->render() from the error messages, and stripped out HTML from the expected results. Tests are now all passing.

Thank you, @lostcarpark! That looks much better, and it also partially resolves 🐛 Filter enforcement and raw HTML output Active .

Yes, the raw HTML output part should be resolved now.

I goofed on the workflow a little bit, but I have merged this into a new 2.0.x branch (my goof was not creating a pushing the 2.0.x branch before merging locally.)

Anyway, I'm marking this as Fixed and hopefully giving everybody the proper credit.

Thanks, everyone!

-mike

I checked out the dev-2.0.x branch to test the GitHub-flavored Markdown support, but I couldn't get it to work at first. I eventually figured out the problem.

Problem 1: Required HTML tags must be manually configured

When enabling GitHub-flavored Markdown (or Smörgåsbord), features like tables and strikethrough do not work out of the box. This is because Drupal's "Limit allowed HTML tags" filter strips out essential tags such as

In GitHub-flavored Markdown, you can align text in columns using colons in the header row

| Left-aligned | Center-aligned | Right-aligned |
| :---         |     :---:      |          ---: |
| git status   | git status     | git status    |
| git diff     | git diff       | git diff      |

However, Drupal strips out the alignment styling because the "Limit allowed HTML tags" filter removes the necessary style / CSS attributes. As a result, the rendered table loses its intended alignment.

I had another thought on this topic. Option 4 would be to run the “Limit allowed HTML tags” filter before the Markdown filter. This way, any unsafe or disallowed HTML is stripped out first. This approach could simplify configuration while preserving security, depending on how much you trust the Markdown library itself. I haven't tested this yet, but I believe it could address both issue #29 and #30.

I looked into this more and the league/commonmark library uses the align attribute for table cell alignment, not style or class attributes. See https://commonmark.thephpleague.com/2.7/extensions/tables/.

So I added<table> <thead> <tbody> <tr> <th align> <td align> to my "Allowed HTML tags" configuration, and that made it work.

Unfortunately, the align attribute is deprecated, and many browsers (especially with default stylesheets) no longer respect it. It doesn't work in Chrome, for example.

It looks like we can configure league/commonmark to use the style attribute instead:

 $config = [
    'table' => [
        'alignment_attributes' => [
            'left' => ['style' => 'text-align:left'],
            'center' => ['style' => 'text-align:center'],
            'right' => ['style' => 'text-align:right'],
        ],
    ],
];

In that case, a user would need to allow <table> <thead> <tbody> <tr> <th stlye> <td style> in the "Allowed HTML tags" configuration.

On a related note, GitHub-flavored Markdown also supports Task lists, so the "Allow HTML tags" would need to allow something like <input type checked disabled>.

As a next step, it might make sense to expand the test coverage. We could add tests for:

• GitHub-style tables with alignment (e.g. left, center, right)
• Task lists with checkboxes (- [ ] and - [x])

Some very good points here.

Changing flavor

I like the idea of adding allowed tags when you change the Markdown "flavor" (option 2)... but if we change the allowed tags on form submission, we would potentially be changing configuration (of the "Allowed tags" filter) the site admin deliberately selected, causing unexpected results.

So I'm thinking if we go that route, we should add a library and run some JS when the Markdown flavor changes to add appropriate tags.

Should we also take away tags if the user changes to a more restrictive flavor?

I also like the idea of warning if tags for the enabled Markdown flavor are not in the allowed tags list (option 1). I do this for <p> and <br> in 📌 Remove/lesson dependency on "Convert line breaks into HTML" Active .

Option 3 is already supported... sort of. You need to use a hook to allow it, as can be seen in markdown_easy_test_module. I would not be opposed to reducing the current error to a warning to enable this more easily, but I guess Mike has to decide on that.

I kinda like option 4. It would allow us to limit the tags in the source but allow the library to add what it needs. Again, Mike's call on whether he trusts the library.

Oops, I accidentally changed the status... changing back.

Tables

I definitely agree we shouldn't use deprecated attributes!

I don't love inline styles, as they create the potential for people to commit all kinds of crimes against CSS, though I think modern browsers protect against most forms of malicious CSS injection threat.

I presume we could instead use something like:

 $config = [
    'table' => [
        'alignment_attributes' => [
            'left' => ['class' => 'markdown-align-left'],
            'center' => ['class' => 'markdown-align-center'],
            'right' => ['class' => 'markdown-align-right'],
        ],
    ],
];

And add a library with CSS to be included with the filter.

We'd then be adding the allowed tags: <table> <thead> <tbody> <tr> <th class> <td class>.

The downside is, of course, that we'd be adding the library, and including CSS whether the Markdown contained tables or not.

Test Coverage

I agree this needs improving, though it's funny you mention strikethrough, as that is covered by the current tests (and seems to be supported in the CommonMark "standard Markdown" flavor, even though it's a non-standard feature).

I would like to see test cases for specific Markdown covered by different flavors, to verify it gets processed only by the respective flavor(s).

I already added tests for line breaks in 📌 Remove/lesson dependency on "Convert line breaks into HTML" Active , and I would like to add additional tests for Markdown syntax.

Moving forward

None of the problems mentioned above directly relate to the original request in this issue, which was to add support for footnotes. We've already had a fair bit of scope creep on this issue!

Also, the change in this issue has already been merged, and rebased with 📌 Remove/lesson dependency on "Convert line breaks into HTML" Active . From a selfish point of view, I worry adding more to this issue will result in a very complex rebase.

I propose moving this issue back to Fixed, and creating follow on issues for the problems above. It seems to me that the 3 problems (changing flavor, tables, and improving test coverage) should each have their own issue (the first two should, of course, have their own test cover, and the third should deal with improving coverage of already supported Markdown).

Sounds good to me. I agree those are best handled in separate issues.

I opened 📌 Support Markdown table alignment Active for the table cell alignment stuff.

-mike

Automatically closed - issue fixed for 2 weeks with no activity.