michelf / php-markdown

Parser for Markdown and Markdown Extra derived from the original Markdown.pl by John Gruber.
http://michelf.ca/projects/php-markdown/
Other
3.42k stars 530 forks source link

Update Markdown.php #392

Open edgering opened 1 year ago

michelf commented 1 year ago

I don't see how that change makes sense. No one is calling these callback functions with a second parameter.

edgering commented 1 year ago

Sure. It's just matched as an undefined variable in validators.

wilson1000-MoJ commented 1 year ago

The change makes sense; $text has no value before this PR. I suggest merging to prevent validators from screaming out.

michelf commented 1 year ago

What makes no sense here is to add a function parameter. This function is never going to receive a $text parameter, and has no reason to receive one either, so it's misleading. A local variable would make more sense.

wilson1000-MoJ commented 1 year ago

Put like that @michelf, I'm behind you. Something simple like this might be better. Although, it is important to note that encodeURLAttribute() is setting $text by reference - to evade any confusion.

/**
 * Parse URL callback
 * @var string|null $text set by reference in encodeURLAttribute() 
 * @param array $matches
 * @return string
 */
protected function _doAutoLinks_url_callback($matches) {
        $text = null;
    $url = $this->encodeURLAttribute($matches[1], $text);
    $link = "<a href=\"$url\">$text</a>";
    return $this->hashPart($link);
}

@edgering does this make sense to you?

edgering commented 1 year ago

Absolutely. Thank you.