thephpleague / commonmark

Highly-extensible PHP Markdown parser which fully supports the CommonMark and GFM specs.
https://commonmark.thephpleague.com
BSD 3-Clause "New" or "Revised" License
2.74k stars 194 forks source link

Allow setting attributes without values in Attributes extension #985

Open svenluijten opened 1 year ago

svenluijten commented 1 year ago

Description

I'm writing my own extension that makes use of the capabilities of the baked-in attributes extension. However, upon testing, I found that it is currently not possible to add an attribute without a value on an element.

I would like to be able to add an attribute without any value to prevent me (and the eventual users of my own extension) from having to add an arbitrary value every time like so:

![image](/assets/image.jpg){my-attribute=true}

Example

Consider parsing the following Markdown with CommonMark, with the Attributes extension loaded:

![image](/assets/image.jpg){my-attribute}

I would expect the following HTML to be rendered:

<p><img src="/assets/image.jpg" alt="image" my-attribute /></p>

However, I'm seeing the following:

<p><img src="/assets/image.jpg" alt="image" />{my-attribute}</p>

I believe this has to do with the League\CommonMark\Util\RegexHelper::PARTIAL_ATTRIBUTEVALUESPEC constant enforcing the presence of an equals character (=) and a subsequent value.

If this were to be changed, the way we extract the attribute name and value would also have to be changed in League\CommonMark\Extension\Attributes\Util\AttributesHelper.

I'll try to whip up a quick PR to get this going, but would love some input if this is even something you want to consider for this project before I dive in fully 🙂

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

I'm exceptionally happy with the way this project is set up. It was super easy to dig and start working on my own extension because of the outstanding documentation and the easy-to-follow code when I needed to dig deeper and do some source-diving. So thank you :heart:

colinodell commented 1 year ago

I really like this proposal! Let's do it :)

I believe this has to do with the League\CommonMark\Util\RegexHelper::PARTIAL_ATTRIBUTEVALUESPEC constant enforcing the presence of an equals character (=) and a subsequent value.

If possible, we should try to avoid changing those PARTIAL_ regular expressions in RegexHelper. Those are based directly on the commonmark.js reference parser and are used elsewhere in the library; any changes could have unintended side effects on other features like parsing HTML content. Instead, we should be able to modify how they are used in AttributesHelper::SINGLE_ATTRIBUTE.

That regular expression basically says to match:

Making the "attribute value spec" part optional (by adding a ? immediately afterward) should be enough to get the regex to match. Inside the parseAttributes() method, we can check if \explode('=', $attribute, 2); only resulted in 1 item, meaning the value part was omitted and should be set to true behind-the-scenes.

Would you be open to revising your PR to try this approach? I haven't actually tested it, but I think it'll put you on the right path :)

I'm exceptionally happy with the way this project is set up. It was super easy to dig and start working on my own extension because of the outstanding documentation and the easy-to-follow code when I needed to dig deeper and do some source-diving. So thank you ❤️

Thank you so much for that kind feedback! 😊