Instantion error using Dependency Injection

[antonioribeiro]

If you use something like Laravel, which recursively instantiates all the things, you get an error, because Environment is only correctly instantiable if we use the static method createCommonMarkEnvironment to boot it up.

[GrahamCampbell]

https://github.com/GrahamCampbell/Laravel-Markdown

[antonioribeiro]

There's no need to use a Laravel package to solve this problem, once you understand what's happening it's a pretty easy fix:

Injecting and erroring:

public function __construct(CommonMarkConverter $converter)
{
    $this->converter = $converter;
}

Fixed

public function __construct()
{
    $this->converter = new CommonMarkConverter();
}

[antonioribeiro]

But... was it really necessary to make the library so dependable of a static method? Seems super odd these days, and not only to me:

https://twitter.com/afilina/status/688437808252465152

And in the library it's an easy fix too, I did it, but I've not created a PR because it broke some tests enforcing Environment being created without extensions:

public function testAddGetExtensions()
{
    $environment = new Environment();
    $this->assertCount(0, $environment->getExtensions());

    ...
}

[colinodell]

Environment is only correctly instantiable if we use the static method createCommonMarkEnvironment to boot it up.

The Environment constructor is intentionally designed to not include any extensions by default. This allows users to customize the environment to their needs.

For example, some users may only want to use a subset of CommonMark, or they may want to include their own custom extension before the CommonMarkCoreExtension is added (because the order extensions are loaded impacts the precedence of parsers and renderers). This is why we provide a separate static factory method for obtaining a pre-configured environment with the default values for convenience.

According to @GrahamCampbell's Laravel package, you should be able to easily instantiate the Environment as a service:

$app->singleton('markdown.environment', function ($app) {
    return $environment; Environment::createCommonMarkEnvironment();
});
$app->alias('markdown.environment', Environment::class);

Could you try that and see if it works for you? If not, could you please provide a code example of what you're trying so I can better understand the situation?

[fesor]

@antonioribeiro your fix breaks the whole idea of dependency injection.

[antonioribeiro]

@fesor, that fix was implementend on my own Markdown converter, wich uses commonmark, I don't really need it to be injected in this case, that's why it was so easy to fix, sorry if I confused you by not pasting the whole code.

@colinodell, as I just said, I don't really need dependency injection to work, my use case is too simple and I was only using it because Laravel makes it easy, but, yes, I'm pretty I'm sure binding it to a closure, to correctly instantiate it, is something that will work.

The whole problem started because Commonmark broke on me after a composer update, because I was not using its "new way" of instantianting, this is the error I got:

ErrorException in Cursor.php line 312:
preg_match(): Compilation failed: missing terminating ] for character class at offset 5

Of course my first thought was "gosh, what I did wrong here?", and started to hunt down a bug I was not supposed to see, because, in my opinion, if you are making a change to allow **some** users to customize the environment to their needs, you should not affect those that are already using your library, should you?

So I went to try to fix instantiation, rewriting some stuff, and got some tests on red because some people really need Environment to be empty, sometimes...

[colinodell]

From what I can tell, the Environment::createCommonMarkEnvironment() method has always existed on that class (since version 0.5, released 13 months ago), even before it had a __construct method. I'd have to see exactly how you were instantiating everything to know why an upgrade may have broken things.

We do follow Semantic Versioning, so as long as you're using the proper version constraints in composer.json then nothing should break when running composer update. The README section on versioning does a great job explaining our versioning policy.

The error you encountered is a legitimate issue which seems to be caused when parsing text without having any inline parsers declared. I'll work on a solution for this issue now.

[GrahamCampbell]

I disagree that this is a bug. Nothing needs changing IMO.

[colinodell]

I agree that the instantiation question is probably not a bug, but the Environment generating an invalid regex is. The issue can be replicated with this code:

$environment = new Environment();
$environment->addBlockRenderer(Document::class, new DocumentRenderer());
$environment->addBlockRenderer(Paragraph::class, new ParagraphRenderer());
$environment->addInlineRenderer(Text::class, new TextRenderer());

$converter = new CommonMarkConverter([], $environment);
echo $converter->convertToHtml('Hello World!');

This will echo out <p>Hello World!</p> as expected. However, it'll also raise several E_WARNING errors because Environment::getInlineParserCharacterRegex() returns an invalid regex. I've corrected this issue in #226.

@antonioribeiro if you can provide some sample code of instantiating failing I'd be happy to take a closer look. Otherwise I'm going to close this report since the underlying issue should be resolved.