addRenderer and addExtension inconsistency

[Arcesilas]

Version(s) affected

2.4.2

Description

When adding an additional custom renderer, it is not executed if no priority is set when added via an extension.

Renderer added with: $environment->addRenderer($class, $renderer) will be actually run on the appropriate node class, whereas renderer added via the register method of an extension will not be run at all.

Dump of $environment->getRenderersForClass($class) shows that the order of the renderers for the class if not the same when added via addRenderer() or via an extension.

When renderer is added via an extension, if it is added with a priority > 0, it will take precedence on the default renderer and be executed. It's ok, but adding a renderer via both methods should produce the same result, especially as renderer is added with the same priority in both cases by default.

If this behavior is not bogus, then maybe it should be explained in the documentation: I've spent about 2 hours trying to figure out why my extension was not working while in my previous tests with addRenderer the renderer was working fine...

How to reproduce

Create a custom renderer. For example:

class YamlCodeRenderer implements NodeRendererInterface
{
    public function render(Node $node, ChildNodeRendererInterface $childRenderer)
    {
        if (str_starts_with($node->getInfo(), 'yaml')) {
            return "We should do something with YAML code block";
        }
    }
}

Register the renderer:

$cmark = new \League\CommonMark\CommonMarkConverter();
$cmark->getEnvironment()->addRenderer(FencedCode::class, new YamlCodeRenderer());
echo $cmark->convert("```yaml\ntest\n```");

Output:

We should do something with YAML code block

Now, register the renderer via an extension:

class MyExtension implements ExtensionInterface
{
    public function register(EnvironmentBuilderInterface $environment): void
    {
        $environment->addRenderer(
            FencedCode::class,
            new YamlCodeRenderer()
        );
    }
}

and:

$cmark = new \League\CommonMark\CommonMarkConverter();
$cmark->getEnvironment()->addExtension(new MyExtension());
echo $cmark->convert("```yaml\ntest\n```");

Output:

test

Possible solution

No response

Additional context

Dump of $environment->getRenderersForClass() when added via $environment->addRenderer():

class League\CommonMark\Util\PrioritizedList#40 (2) {
  private array $list =>
  array(1) {
    [0] =>
    array(2) {
      [0] =>
      class YamlCodeRenderer#39 (0) {
      }
      [1] =>
      class League\CommonMark\Extension\CommonMark\Renderer\Block\FencedCodeRenderer#70 (0) {
      }
    }
  }
  private ?Traversable $optimized =>
  class ArrayIterator#116 (1) {
    private $storage =>
    array(2) {
      [0] =>
      class YamlCodeRenderer#39 (0) {
      }
      [1] =>
      class League\CommonMark\Extension\CommonMark\Renderer\Block\FencedCodeRenderer#70 (0) {
      }
    }
  }
}

Dump of $environment->getRenderersForClass() when added via extension:

class League\CommonMark\Util\PrioritizedList#70 (2) {
  private array $list =>
  array(1) {
    [0] =>
    array(2) {
      [0] =>
      class League\CommonMark\Extension\CommonMark\Renderer\Block\FencedCodeRenderer#69 (0) {
      }
      [1] =>
      class YamlCodeRenderer#102 (0) {
      }
    }
  }
  private ?Traversable $optimized =>
  class ArrayIterator#117 (1) {
    private $storage =>
    array(2) {
      [0] =>
      class League\CommonMark\Extension\CommonMark\Renderer\Block\FencedCodeRenderer#69 (0) {
      }
      [1] =>
      class YamlCodeRenderer#102 (0) {
      }
    }
  }
}

Did this project help you today? Did it make you happy in any way?

Thanks a lot for this great package. It's incredibly powerful and customizable: it helps make everything become possible.

[colinodell]

Thank you for the detailed report!

The only way to guarantee the execution order is to set explicitly set the priority to be higher or lower than others. Adding multiple renderers with the same priority is considered to have undefined behavior. Because the default renderer has a priority of 0:

https://github.com/thephpleague/commonmark/blob/9a2195da2600970e343d3d5f397cf0f5ecdc06eb/src/Extension/CommonMark/CommonMarkCoreExtension.php#L65

You'll want to add your custom renderer with a priority of 1 or higher

If this behavior is not bogus, then maybe it should be explained in the documentation

Yeah, the documentation doesn't do a good job of explaining of why or when you'd want to set a custom priority, or what happens if two renderers have the same priority 😞

https://github.com/thephpleague/commonmark/blob/9a2195da2600970e343d3d5f397cf0f5ecdc06eb/docs/2.4/customization/environment.md?plain=1#L84-L90

Anyone who would like to revise the documentation is welcome to submit a PR! (Note that renderers aren't the only things that have a priority - all references should be updated.)

When renderer is added via an extension, if it is added with a priority > 0, it will take precedence on the default renderer and be executed. It's ok, but adding a renderer via both methods should produce the same result, especially as renderer is added with the same priority in both cases by default.

This behavior you're seeing occurs because the extension's register() method isn't called immediately when the extension is added. I can't remember why we do that - it's been 9 years since I made that design choice and apparently I forgot to document why 🙃 This is something we should probably revisit soon:

• If there's still a valid reason for this behavior, we should document it.
• If there's no valid reason for this, we should consider changing it in a way that minimizes BC breaks.

I'll keep this open to remind me to revisit that again when I have the bandwidth.

Thanks a lot for this great package. It's incredibly powerful and customizable: it helps make everything become possible.

Thank you so much, I really appreciate that! 😊

[Arcesilas]

Thanks for your quick reply!

I struggle with parser and extensions... It's not what I'm best at 😅 Anyway, I may provide a PR to highlight this point in the documentation.

Thanks again for the great job!