Automatically generating id="" attributes

[evert]

Hi @michelf !

This patch does the following:

1. Adds a new public property to the markdown parses: $header_id_func.
2. If this is set, it will be called to automatically generate an id="" attribute, but only for headers that have none!
3. It also adds html escaping for both id and class attributes

Usage:

$md = new Markdown();
$md->header_id_func = function($headerName) {
    return rawurlencode(strtolower(
        strtr($headerName, [' ' => '-'])
    ));
};
$html = $md->transform($text);

So.. you asked only for support in MarkdownExtra. Before I knew it I had written a full implementation in Markdown. Since this doesn't break anything in Markdown, I thought I would keep it in for now.

But if you really only want this feature in MarkdownExtra, I am also happy to remove it from Markdown for you.

Let me know!

cc: @simensen

[GrahamCampbell]

wrong indentation

[evert]

thnx =) fixed

[michelf]

Looks nicely done. I don't mind the feature being in Markdown too, but it'd be better if you could avoid repeating the code under the # id attribute generation comment. Could this be put in a separate function where you assign the returned value to $idAtt? It's just two lines, but it's two packed lines repeated four times.

There's one thing I dislike: you are calling the generator function even when the id attribute has been specified in the Markdown source. It doesn't seem easy to fix that however and it is inconsequential in the default case (no generator function), so I'll probably accept the patch regardless.

I'm actually not sure about htmlspecialchars. Markdown typically allows entities to be specified in the source, and the ampersand in those entities must not be escaped. I think we should use $this-> encodeAmpsAndAngles instead. But I'm unsure what to do with the result of the user-defined id generator. Should it be considered HTML-clean? (The output of url_filter_func is considered clean right now.)

[michelf]

Looking at it a few more seconds, I realize that calling htmlspecialchars on things parsed by doExtraAttributes is not necessary given that the regex used to match the special attribute block does is limited to a small set of characters that pose no threath.

[evert]

I will make the change so it becomes a function.

The main problem I have with not using htmlspecialchars, is that it would allow a user to put a " in the title, which could then be used to break out of the attribute.

[evert]

Based on what you said, it makes sense to me to to remove it for the class attribute though.

[michelf]

Ok, granted. The user function is probably not to be entirely trusted for this. I suggest you use $this->encodeAttribute which will not let " through but will still preserve existing character entities.

[evert]

Ok, sounds good!

[evert]

Ready for review again.

[evert]

If you like, I can also commit my copy of readme.php. I slightly altered it to use this functionality, but I understand if you want to keep it as minimal as possible.

[evert]

One more suggestion:

I can alter this slightly more to also turn all headers with id's into links that point to themselves. This is also how for example github behaves, so a user can click a header and get a reference in their addressbar.

If you are interested in that change, I would be happy to add another setting that automatically inserts <a> tags in <h1-6> that link to its own id.

[michelf]

I'm not really sure what to respond to your last comment. I do think this is a valid thing to want, but I'm not too sure I want to add it. I don't want to bake every possible output into PHP Markdown with a configuration variable for each one.

What I'd really like eventually is to move all the HTML output to a separate class, one where you can easily override and transform the output to what's needed. If you want to work on that, feel free. But I'm not interested in adding an option for transforming headers into links.

I'm going to do a final check on this pull request sometime this weekend, and it'll probably get merged. And for the Readme.php, I'm going to keep it relying on the default configuration.

[evert]

Fair enough!

I'm looking forward to the templating class.

After merging, do you also think that a new release is on the horizon? This will allow me to integrate it into sculpin.

[michelf]

Doesn't pass unit tests for the plain Markdown parser. I get a lot of things like this:

<h1>======</h1>

<h2>------</h2>

You can see that for instance by parsing "Headers.text" in the "PHP Markdown" test suite in MDTest. https://github.com/michelf/mdtest/blob/master/PHP%20Markdown.mdtest/Headers.text

Looks good for MarkdownExtra.

[evert]

Oh man... I messed up with a later change. I see exactly what I did.

[evert]

That problem is now fixed.