Skip to content

Conversation

@MilanPala
Copy link
Contributor

  • bug fix: yes
  • BC break? no

@dg
Copy link
Member

dg commented Jul 7, 2019

Which bug does it solve?

@mabar
Copy link
Contributor

mabar commented Jul 7, 2019

Translator which supports only strings, I guess.

Easier would be solve it in translator, this is probably not the only place where is non-string value translated.

if (!is_string($message)) {
  return (string) $message;
}

@dg
Copy link
Member

dg commented Jul 7, 2019

It creates inconsistency with others usage of translate().

@MilanPala
Copy link
Contributor Author

With behavior in Nette 3 i can't use Nette\Utils\Html as label (for example with links to terms and conditions). In Nette 2 I create label with link and push it into Nette\Forms\Controls\Checkbox. It works in Nette 3 too: https://github.com/nette/forms/blob/master/src/Forms/Controls/Checkbox.php#L26 But translator converts \Nette\Utils\Html\ instance into string and escape html tags into entities: https://github.com/nette/forms/blob/master/src/Forms/Controls/BaseControl.php#L279 and https://github.com/nette/utils/blob/master/src/Utils/ITranslator.php#L22

In Nette 2 I don't change Nette\Utils\Html in my translator and it works so far:

	if ($message instanceof Nette\Utils\Html) {
			return $message;
	}

In this case would be better use Nette\Forms\Controls\BaseControl::translate instead of $this->getForm()->getTranslator()->translate() in https://github.com/nette/forms/blob/master/src/Forms/Controls/BaseControl.php#L278-L279, because this method has condition for this use case. What do you think?

@mabar
Copy link
Contributor

mabar commented Jul 8, 2019

I see. Translator must return string, but string is escaped by template engine.

@dg dg force-pushed the master branch 2 times, most recently from 608d595 to b3356d0 Compare July 8, 2019 09:56
@dg dg merged commit 4876961 into nette:master Jul 8, 2019
@dg
Copy link
Member

dg commented Jul 8, 2019

Thanks

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