Prevent templating being treated as attributes

[jasonvarga]

This PR prevents mustache-style template language strings being interpreted as attributes.

This was introduced in 2.5.0 by #986. It caused template strings to be wiped out.

$converter->convert('# Hello {{ foo }}');

-<h1>Hello {{ foo }}</h1>
+<h1>Hello {}</h1>

This PR adds negative lookaheads to the regex to ignore the double-braces, allowing the template strings to be maintained:

-<h1>Hello {}</h1>
+<h1>Hello {{ foo }}</h1>

I'm not sure if the changes to AttributesHelperTest are needed. They passed before making the regex change too.

[colinodell]

Thanks for reporting this issue and suggesting a fix!

I'd generally recommend that templating engines run first to avoid these types of syntax conflicts. However, in this case, I think it's a good idea to tighten our attribute syntax, which happens to have the nice benefit of fixing your templating issue 😉

Would you mind revising the regex and tweaking the test per my comments? Once done I'll cut a 2.5.1 release with the fix.

[jasonvarga]

Both done!

[colinodell]

Thank you! I'll cut a new release with this fix shortly :)

[colinodell]

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

[jasonvarga]

Much appreciated!

[xavierlacot]

Hi @colinodell,

Thanks a lot for this fix an more generaly for your impressive work on this library.

As for this fix, I believe that it is maybe not enough in some cases where curly braces are employed within the text. For example,

some {text in curly} brackets

some `{text in curly}` brackets

will render as:

<p text in curly>some brackets</p>
<p>some <code>{text in curly}</code> brackets</p>

I am not sure of the right solution here:

1. either accept that activating the attributes extension requires the author to write mardown content in a different way, by escaping occurrences of curly brackets in certain contexts;
2. deprecate the support for empty-value attributes (or find an alternative syntax that would be light)

Case 1- enabling the Attributes extension requires the Markdown author to change the way he writes content:

Before	After
A [Simple{Test}](https://example.com).	A [Simple\{Test}](https://example.com). escaping required
Another [Simple{{Test}}](https://example.com).	Another [Simple{{Test}}](https://example.com).
some {text in} brackets	some \{text in} brackets escaping required
some `{text in}` brackets	some `{text in}` brackets

Here is a summary of the required changes in the way of writing markdown content:

- A [Simple{Test}](https://example.com).
+ A [Simple\{Test}](https://example.com).

Another [Simple{{Test}}](https://example.com).

- some {text in} brackets
+ some \{text in} brackets

some `{text in}` brackets

Case 2- Deprecate the support for empty-value attributes

Maybe could we choose to deprecate the "empty value" attribute syntax and rather use an alternate syntax, for example:

{hello=}
Some text

I think this alternative syntax would be a good compromise:

• avoiding the need for Markdown authors to adapt the syntax used according to the writing context (it is no longer necessary to escape curly braces)
• allowing attributes without values (with {attribute-name=} rendered as attribute-name)
• allowing attributes with an empty value (with {attribute-name=""} which is rendered as attribute-name="")

However, the syntax proposed above ({attribute-name=}) does not work well when combined with other attributes (eg. {attribute-name= .some-class} would get interpreted as attribute-name="some-class"). Therefore, I think we should rather use a special attribute prefix character (like the special cases . and #) to distinguish this specific case.

In conclusion, I would either propose to prefix empty-value attributes with ~ , or to prevent empty-values altogether, as they are equivalent toi the long-form (a value that is an ASCII case-insensitive match for the attribute's canonical name, with no leading or trailing whitespace)

some text{~hello .test}

// translates to:
// <p hello class="test">some text</p>

or:

some text{hello=hello .test}

// translates to:
// <p hello="hello" class="test">some text</p>

If you are interested, I can provide you with a pull request to enable this behavior :-)
Edit: the PR is available in #1040