Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Markdown.php #392

Open
wants to merge 1 commit into
base: lib
Choose a base branch
from
Open

Update Markdown.php #392

wants to merge 1 commit into from

Conversation

edgering
Copy link

No description provided.

@michelf
Copy link
Owner

michelf commented Apr 21, 2023

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

@edgering
Copy link
Author

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

@wilson1000-MoJ
Copy link

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

@michelf
Copy link
Owner

michelf commented Jul 6, 2023

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
Copy link

wilson1000-MoJ commented Jul 6, 2023

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
Copy link
Author

Absolutely. Thank you.

@edgering edgering closed this Jul 10, 2023
@edgering edgering reopened this Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants