Add support for Commonmark v2

Perhaps that way forward would be to roll a stable 3.0 and then tackle Commonmark v2 in a new branch?

There's a dependency on Commonmark 2 with the Migrate Markdown to HTML module (https://www.drupal.org/project/migrate_process_markdown_to_html), so I'd say this feature is quite important

Hi,

I have a very minimal patch, to prevent error on converting when we want to use commonmark 2.4.
This patch can apply on 3.0.0-rc2 version of markdown module

Hi @tikaszvince, thanks for the patch! I maybe testing this out in an upcoming project sprint.

Have you encountered any issues? I'm curious how you are using this module with Commonmark v2.

A quick glance Commonmark 1.6 to 2.0 upgrade docs suggests other changes are required.

Perhaps these are already in place when looking at https://git.drupalcode.org/project/markdown/-/blob/3.0.x/src/Plugin/Mark... from changes in #3142418: Support multiple libraries per plugin.

I had=ve a problem with the approach checking which class exists. The Commonmark Environment should be constructed in different way with version 2.0 and there were some changes since 2.0 to 2.4. So i had to check for the version string.

The more i think about this the more I'm convinced about the proper approach should be a different @MarkdownParser for newer Commonmark versions.

My patch also uses the \ReflectionObject to merge settings which is against the intention of the Commonmark, which triggers a deprecation warning too when you call merge().

I'm using the markdown module and Commonmark for convert texts from an API and store them in a formatted text field. In my case these were the required changes to make my integration code work after upgrades.

I tried to get Commonmark 2.4 to run with this #5 patch and got still WSOD. Because upgrading Commonmark is currently not so important on the related project I downgraded again and just wanted to report here.

Thanks tikaszvince for the notes and C-Logemann for reporting. I'm looking into these changes this week.

@tikaszvince, I agree about a new plugin for Commonmark v2, but I do wonder about sharing the commonmark markdown extensions across both; will there also need to be v2 only extensions.

The project requirement I'm hoping to satisfy utilizes Commonmark v2 ability to configure disallowed raw HTML tags, which will require an update to the commonmark-disallowed-raw-html markdown extension; a change not compatible with Commonmark v1.

The v2's Commonmark::convertToHtml() will have to change per these update docs.

From

  protected function convertToHtml($markdown, LanguageInterface $language = NULL) {
    return $this->converter()->convertToHtml($markdown);
  }

to

  protected function convertToHtml($markdown, LanguageInterface $language = NULL) {
    return $this->converter()->convertToHtml($markdown)->getContent();
  }

Hi,

I've attached two files as work in progress: 1) commonmark-v1-v2.diff to show changes between the two versions and 2) a new patch to add CommonMarkV2.php MarkdownParser plugin. The second patch differs from the first patch by making an entirely new Commonmark v2 plugin.

I have to switch to some other work briefly, but will return to this later today.

Here's a list of next steps

• Solve for max_nesting_level value type casting as string instead of integer; this results in an error: League\Config\Exception\ValidationException: The item 'max_nesting_level' expects to be int, '10' given. in League\Config\Configuration->build() (line 195 of /var/www/vendor/league/config/src/Configuration.php)
• Solve for Error: RuntimeException: Unable to find corresponding renderer for node type League\CommonMark\Node\Block\Document in League\CommonMark\Renderer\HtmlRenderer->renderNode() (line 88 of /var/www/vendor/league/commonmark/src/Renderer/HtmlRenderer.php)
• Check the Configuration Option Changes, @see: https://commonmark.thephpleague.com/2.0/upgrading/developers/#configurat...
• Refactor max_nesting_level default value management instead of using PHP_INT_MAX
• General code cleanup and refactoring
• Updating CommonMark MarkdownParser to not include v2
• Creating required CommonMark v2 Extensions; updating existing existing extensions to not include v2

While I work on this, I'll assign this issue to myself. I'll share some progress in the next few days.

Attached is a new patch that solves the following error by creating a CommonMarkCoreExtension MarkdownExtension plugin. This probably isn't the best way to solve the error; I imagine CommonMarkCoreExtension should be added to the environment by default without enabling it as a MarkdownExtension plugin.

Solve for Error: RuntimeException: Unable to find corresponding renderer for node type League\CommonMark\Node\Block\Document

The patch also changes the existing Commonmark MarkdownExtension plugins InstallableRequirement constraints to prevent them from being used with the new CommonMarkV2 parser.

And lastly, the patch includes a new configurable Disallowed Raw HTML MarkdownExtension plugin with CommonMarkV2 parser Installable Requirement constraint. This uses the v2 supported config options and satisfied my project requirement to remove <img> tags.

While I can render content using the CommonMarkV2 parser...HOWEVER, its not rendering using the Enable Emphasis, Enable Strong, Use Asterisk, Use Underscore or Unordered List Markers options.

I need to check-in with my project team about the remaining effort and timing. I'm starting to think using a custom, or contributed, Drupal @Filter plugin that integrates directly with CommonMark V2 may be more reliable and maintainable path going forward. As much as I'd like to see this Markdown module work with CommonMark v2 for more broader usage, there's a lot of abstraction found within this Markdown module to account for.

Well, we've pivoted away from using this module. Instead, we've implemented a custom Drupal @Filter plugin (see d.o. forum for related discussion on filter plugins) to use CommonMark V2 directly.

Here's an example of that implementation. In ours, I created custom extension and image renderer to replace commonmarkcore below and its processors and renderers to not render image markdown.

namespace Drupal\my_module\Plugin\Filter;

use Drupal\filter\FilterProcessResult;
use Drupal\filter\Plugin\FilterBase;
use Drupal\Core\Logger\LoggerChannelTrait;
use League\CommonMark\Environment\Environment;
use League\CommonMark\Extension\CommonMark\
use League\CommonMark\Exception\CommonMarkException\CommonMarkCoreExtension;
use League\CommonMark\Extension\Strikethrough\StrikethroughExtension;
use League\CommonMark\MarkdownConverter;

/**
 * CommonMark V2 filter plugin for Markdown.
 *
 * @Filter(
 *   id = "commonmark_v2",
 *   title = @Translation("CommonMark V2"),
 *   description = @Translation("Utilizes CommonMark V2 to parse markdown and convert into HTML"),
 *   type = Drupal\filter\Plugin\FilterInterface::TYPE_MARKUP_LANGUAGE,
 * )
 */
class CommonMarkV2 extends FilterBase {

  use LoggerChannelTrait;

  /**
   * The configuration array.
   *
   * @see https://commonmark.thephpleague.com/configuration/.
   */
  private array $config = [
    'renderer' => [
      'block_separator' => "\n",
      'inner_separator' => "\n",
      'soft_break'      => "\n",
    ],
    'html_input' => 'strip',
    'allow_unsafe_links' => FALSE,
    'max_nesting_level' => 10,
  ];

  /**
   * {@inheritdoc}
   */
  public function process($text, $langcode): FilterProcessResult {
    try {
      $environment = $this->createEnvironment();
      $converter = new MarkdownConverter($environment);
      $converted_text = $converter->convert($text);
      return new FilterProcessResult($converted_text);
    }
    catch (CommonMarkException $e) {
      $this->getLogger('my_module')->critical('Unable to convert markdown into HTML.');

      return new FilterProcessResult($text);
    }
  }

  /**
   * Generate an environment with extensions.
   */
  private function createEnvironment(): Environment {
    $environment = new Environment($this->config);

    // Add CommonMarkCoreExtension for commonly used parsers and
    // renders.
    $environment->addExtension(new CommonMarkCoreExtension());

    // Add additional extensions.
    // @see https://commonmark.thephpleague.com/2.4/basic-usage/#using-extensions
    $environment->addExtension(new StrikethroughExtension());

    return $environment;
  }

}

I've just attached an updated version of my patch from #5 to handle error on Drupal10.1 + PHP8.1 caused by INF passed as max_nesting_level.

---

Hey @markdorison,

Is there any news around how we can upgrade to Commonmark v2.4+?

There is are new comments in the issue Adopt CommonMark spec for Markdown files since the last comment here.

The description on the Markdown Module states "There is current an issue open to make the CommonMark Spec the "official" Drupal Coding Standard." which may also need an update.

Tested the patch and it's working well.

I applied patch #14 on 3.0.1 and updated league/commonmark to 2.5.3, but unfortunately I immediately get a fatal error on any site page: Fatal error: Could not check compatibility between League\CommonMark\CommonMarkConverter::getEnvironment(): League\CommonMark\Environment\Environment and League\CommonMark\MarkdownConverter::getEnvironment(): League\CommonMark\Environment\EnvironmentInterface, because class League\CommonMark\Environment\Environment is not available in /app/vendor/league/commonmark/src/CommonMarkConverter.php on line 40

Have created MR 39 based on patch #14

@jonathan_hunt I'm unable to reproduce the fatal error you mention, although in our installation we have patches applied for the following issues:

#3226069: Allow "incompatible" filters to be enabled (but validate that they appear after Markdown)
#3274316: After each cache clear, visiting a page filtered with markdown causes calls to pecl.php.net; it is not clear why we are doing this
#3283349: Add support for Commonmark v2
#3409277: SubformState incorrect interface error
#3463119: Error when saving text format - configuration property id doesn't exist
#3470570: Unable to save text format without enabling Markdown filter

This appears to be entering the realm of a security issue now:

ddev composer audit
Found 1 security vulnerability advisory affecting 1 package:
+-------------------+----------------------------------------------------------------------------------+
| Package           | league/commonmark                                                                |
| Severity          | high                                                                             |
| CVE               | NO CVE                                                                           |
| Title             | league/commonmark's quadratic complexity bugs may lead to a denial of service    |
| URL               | https://github.com/advisories/GHSA-c2pc-g5qf-rfrf                                |
| Affected versions | <2.6.0                                                                           |
| Reported at       | 2024-12-09T20:42:07+00:00                                                        |
| Advisory ID       | PKSA-fndg-qryc-dyc9                                                              |
+-------------------+----------------------------------------------------------------------------------+

As mentioned in previous comments I can update to version 2 in composer but that results in errors (I have not applied this patch).
Composer didn't offer an upgrade path to a new minor version of 1, so it appears the fix for this issue has not been backported.

There was an issue with the last version of the MR (fixed now). If someone had selected additional extensions, they all got removed due to the way that the markdown object gets created.

Another issue we ran into is that v1 upstream support for inline css classes is different than v2. In our case, we don't have that many nodes and blocks so we're manually fixing, but I could see someone benefiting from an upgrade path.

In v1 it supported:

{.display-4.mt-5.mx-auto}
## Here is some H2 text!

V2 requires spaces between each class name:

{.display-4 .mt-5 .mx-auto}
## Here is some H2 text!

I've opened https://github.com/thephpleague/commonmark/issues/1071 to see if there is an upstream response.

Upstream already fixed the regression. This needs a review now.

https://github.com/thephpleague/commonmark/releases/tag/2.6.2

Using patch #14 with Drupal 10.4, I also get:

Fatal error: Could not check compatibility between League\CommonMark\CommonMarkConverter::getEnvironment(): League\CommonMark\Environment\Environment and League\CommonMark\MarkdownConverter::getEnvironment(): League\CommonMark\Environment\EnvironmentInterface, because class League\CommonMark\Environment\Environment is not available in /var/www/html/vendor/league/commonmark/src/CommonMarkConverter.php on line 40

No amount of cache clearing fixes the error.

As noted on #3364199: Unknown parser settings when saving CommonMark parser, we're seeing warnings about an unknown parser for commonmark 2.7.0, with the patch from #14.

We don't get those warnings with commonmark 2.6.0, but there is a vulnerability reported for < 2.7.0 - see https://github.com/advisories/GHSA-3527-qv2q-pfvx

The fatal error mentioned in #28 is the same as mentioned in #18.

As noted in #21, we haven't been able to reproduce it, but we do have quite a lot of patches applied to markdown, for these issues:

#3226069: Allow "incompatible" filters to be enabled (but validate that they appear after Markdown)
#3274316: After each cache clear, visiting a page filtered with markdown causes calls to pecl.php.net; it is not clear why we are doing this
#3283349: Add support for Commonmark v2
#3409277: SubformState incorrect interface error
#3438472: Automated Drupal 11 compatibility fixes for markdown
#3483437: Fix fatal error with Drush 13 due to MarkdownCommands replacing logger

As mentioned in #29, you must apply so many patches to even use this module right now. Unfortunately, with all the patch conflicts in composer, it is borderline impossible to run v2. Honestly, I'd almost suggest the maintainers consider delisting the releases as supported while they work to get some of these fatal errors resolved.

Regarding the unknown parsers issue, see #3529633: Update Packagist API usage and clarify parser status output

Thanks for picking up this module and bringing it back to life, @joelpittet!

I saw in the D11 issue that you were leaving Commonmark v1 support in-place. Perhaps it makes sense to add v2 support in a 4.0.x branch and drop v1 there? That could help reduce the maintenance burden moving forward, especially since v1 is insecure / EOL?

@kmonty that sounds like a good plan! Want to add some deprecation messages to help the cleanup in 4.x

We have the option for parsing asterisks turned off, yet the module still seems to be telling commonmark to parse asterisks.

I suspect this might be to do with the upgrade to v2. Looking at the config structure for CommonMark, it shows that use_asterisk among other options, should be in a nested array under the commonmark key. A quick look at the source code for the CommonMark plguin in this module indicates that these options are not being nested under a commonmark key.

I don't know for sure if that was a change between v1 and v2, as the v1 docs also show these options as nested. Maybe at one point in v1 they weren't nested, and now they are but a BC-layer is handling it. Unsure right now.

@aaronmchale I appreciate the feedback. I am hoping to get something out soon. Do you think it's safe enough to release what we have so we can get some D10/11 users, or does #35 need to be resolved before we can proceed? I have committed most of what this issue was doing in other issues, so it's mostly just the test that @megachriz got started to test 1.6

@joelpittet Personally I would see that as a significant regression, if the site has opted to disable the processing of asterisks. In our case we have content that relies on the asterisk being displayed on the page and not being parsed as markdown. So the site intentionally disables that option, then we probably should assume that it would be a breaking change for them if asterisks were to start being processed again.

From my perspective, this was significant enough of a regression that it meant we very quickly swapped out the makrdown module for a custom filter plugin (similar to the example in #13, so I don't personally have an immediate need now for this issue to be resolved.

@aaronmchale Thanks for taking the time to explain this in more detail, and sorry about the regression. I completely agree that this would be a breaking change for sites that intentionally disable asterisk parsing, especially when content relies on literal output.

I really appreciate the extra context around CommonMark v2 and the config nesting. I’ve created a follow-up in #3567398: CommonMark asterisk parsing option ignored after upgrade to CommonMark v2 to investigate this directly and make sure we’re handling the configuration correctly.

Thanks again the response, even after you had to workaround it on your end.

I know many people rely on this module, and also that it is complex (and even that a simplified fork popped up when this one lapsed). My intent it to dust it off and get it back to a usable state.

Thanks @joelpittet and @aaronmchale for your work on this - it's much appreciated - we've been using this module with lots of patches for a while, so it's great news that there's progress.

@suryabhi why #40? You gotta give more than a reroll patch for a comment, it feels like spam otherwise.

Not sure why the Issue Fork was hidden in favor of the no comment patchfile...

Hi!

I want to update the Markdown module from 3.0.1 to 3.1.0.
The patch from the #14 can't be applied anymore.
Would it be safe to create patch from the current issue's MR and use it with 3.1.0 version?
Or there can be errors or bugs?
Asking because of #41 about #40.

Thanks!

Created patch from the MR's diff.

With applied patch from #46 I have error:

The website encountered an unexpected error. Try again later.

League\CommonMark\Renderer\NoMatchingRendererException: Unable to find corresponding renderer for node type League\CommonMark\Node\Block\Document in League\CommonMark\Renderer\HtmlRenderer->renderNode() (line 88 of /var/www/html/vendor/league/commonmark/src/Renderer/HtmlRenderer.php).
League\CommonMark\Renderer\HtmlRenderer->renderDocument() (Line: 61)
League\CommonMark\MarkdownConverter->convert() (Line: 158)
Drupal\markdown\Plugin\Markdown\CommonMark\CommonMark->convertToHtml() (Line: 227)
Drupal\markdown\Plugin\Markdown\BaseParser->parse() (Line: 174)
Drupal\markdown\Plugin\Filter\FilterMarkdown->process() (Line: 123)
Drupal\filter\Element\ProcessedText::preRenderText()
call_user_func_array() (Line: 113)
Drupal\Core\Render\Renderer->doTrustedCallback() (Line: 886)
Drupal\Core\Render\Renderer->doCallback() (Line: 431)
Drupal\Core\Render\Renderer->doRender() (Line: 248)
Drupal\Core\Render\Renderer->render() (Line: 165)
Drupal\Core\Render\Renderer->Drupal\Core\Render\{closure}() (Line: 637)
Drupal\Core\Render\Renderer->executeInRenderContext() (Line: 164)
Drupal\Core\Render\Renderer->renderInIsolation() (Line: 310)
check_markup() (Line: 34)

Update:
Re-installed the module without this patch.
The error has gone. I'm using league/commonmark 2.8.
Didn't notice other errors at this moment. Will report if anything will be noticed.

@khiminrm Lots of what this issue had was committed already, so you shouldn't need this MR. We were experimenting with 1.x tests which I am less and less interested in committing. Please check #3567398: CommonMark asterisk parsing option ignored after upgrade to CommonMark v2 for some config changes.