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

Do not force string return type from ITranslator interface #231

Closed
smuuf opened this issue Aug 10, 2020 · 8 comments
Closed

Do not force string return type from ITranslator interface #231

smuuf opened this issue Aug 10, 2020 · 8 comments

Comments

@smuuf
Copy link

smuuf commented Aug 10, 2020

Is your feature request related to a problem? Please describe.

Due to the change in \Nette\Localization\ITranslator, where string return type was added in version 3, we're no longer able to have easily translatable pieces of HTML in our Latte templates/Nette forms.

Explain your intentions.

Previously, in Nette 2.4, we were able to return instances of \Nette\Utils\Html (class which implements the \Nette\Utils\IHtmlString interface) from our own implementation of \Nette\Localization\ITranslator. This is no longer possible as of version 3, due to the added string return type, which disallows the previously working logic.

This, unfortunately, makes the transition to Nette 3 very painful.

A "translatable piece of HTML" can, for example, be some piece of Markdown rendered into HTML. Markdown-markuped text is identified by our translator, processed, and then returned as a \Nette\Utils\Html object (instead of just string) - which is then not escaped when placed inside a Latte template.

Minimal example from our current 2.4 translator:

class OurTranslator implements \Nette\Localization\ITranslator {

	private $parsedown;

	// ...

	public function translate($message, $count = null) {
		$translated = gettext($message);
		return $this->processMarkdown(sprintf($translated, ...func_get_args()));
	}

	/**
	 * If the string begins with a magic "md:" prefix, process that string as
	 * if it uses markdown markup. This is opt-in to avoid any potential
	 * vulnerabilities from strings provided by untrusted third-parties.
	 *
	 * @return string|\Nette\Utils\Html
	 */
	protected function processMarkdown(string $message) {

		if (strpos($message, "md:") === 0) {
			$html = new \Nette\Utils\Html;
			return $html->setHtml($this->parsedown->line(substr($message, 3)));
		}

		return $message;

	}

}

The translator can simply return a Html object and then we don't have to deal with manually escaping/unescaping everything in Latte templates. This is because the \Latte\Runtime\Filters\escapeHtmlText itself supports this:

https://github.com/nette/latte/blob/bda925b140fe1fbc42b14c3f4f6343b808caf795/src/Latte/Runtime/Filters.php#L45-L50

	public static function escapeHtmlText($s): string
	{
		return $s instanceof HtmlStringable || $s instanceof \Nette\Utils\IHtmlString
			? $s->__toString(true)
			: htmlspecialchars((string) $s, ENT_NOQUOTES | ENT_SUBSTITUTE, 'UTF-8');
	}

Now, because of the change of the ITranslator interface forcing the string return type, this mechanism can no longer be used.

Yes, in PHP 8 with union types it might be ok to use union string|IHtmlString|HtmlStringable return type for the translate method, but since that's not what can be done now, I'm proposing for the removal of the currently needlessly limiting string return type.

@her-ur
Copy link

her-ur commented Nov 16, 2021

We are in the same situation as @smuuf

@dg
Copy link
Member

dg commented Nov 16, 2021

How about this?

interface Translator
{
	/** @param  mixed  ...$parameters */
	function translate(string|Stringable $message, ...$parameters): string|Stringable;
}

@her-ur
Copy link

her-ur commented Nov 16, 2021

Looks good 👍

@dg
Copy link
Member

dg commented Nov 16, 2021

I think it's not even a BC break.

@martenb
Copy link

martenb commented Mar 18, 2024

What is correct solution of this phpstan error? It looks like BC for me...

$presenter->flashMessage($presenter->translator->translate('Test'));
Parameter #1 $message of method Nette\Application\UI\Control::flashMessage() expects 
Nette\HtmlStringable|stdClass|string, string|Stringable given.

@petrparolek
Copy link

petrparolek commented Mar 18, 2024

I rewrite code to $presenter->flashMessage((string) $presenter->translator->translate('Test')); everywhere

@petrparolek
Copy link

@ondrejmirtes Could you resolve error in phpstan/phpstan-nette?

@ondrejmirtes
Copy link
Contributor

@petrparolek I'm not going to read all this. Please open an issue in phpstan-nette with a full description of what you are trying to fix.

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

No branches or pull requests

6 participants