Skip to content
This repository was archived by the owner on Jan 30, 2020. It is now read-only.

Provide a negative unit test for the double translation of PR-104 #188

Merged
merged 3 commits into from
May 16, 2018
Merged

Provide a negative unit test for the double translation of PR-104 #188

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
25f5b9b
Provide a negative unit test for the double translation of PR-104
MatthiasKuehneEllerhold Dec 8, 2017
f49af08
Reset default validator translator between tests
weierophinney May 14, 2018
455ab01
Implements `setTranslateMessages()` for en/dis-abling translations
weierophinney May 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 71 additions & 17 deletions src/View/Helper/FormElementErrors.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ class FormElementErrors extends AbstractHelper
*/
protected $attributes = [];

/**
* @var bool Whether or not to translate error messages during render.
*/
protected $translateErrorMessages = true;

/**
* Invoke helper as functor
*
Expand All @@ -49,6 +54,10 @@ public function __invoke(ElementInterface $element = null, array $attributes = [
/**
* Render validation errors for the provided $element
*
* If {@link $translateErrorMessages} is true, and a translator is
* composed, messages retrieved from the element will be translated; if
* either is not the case, they will not.
*
* @param ElementInterface $element
* @param array $attributes
* @throws Exception\DomainException
Expand All @@ -60,39 +69,32 @@ public function render(ElementInterface $element, array $attributes = [])
if (empty($messages)) {
return '';
}
if (! is_array($messages) && ! $messages instanceof Traversable) {

$messages = $messages instanceof Traversable ? iterator_to_array($messages) : $messages;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this, as array_walk_recursive() ONLY allows arrays, and allowing a Traversable was going to lead to problems.

if (! is_array($messages)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing so allowed simplifying this conditional to only look for array values.

throw new Exception\DomainException(sprintf(
'%s expects that $element->getMessages() will return an array or Traversable; received "%s"',
__METHOD__,
(is_object($messages) ? get_class($messages) : gettype($messages))
));
}

// Flatten message array
$messages = $this->flattenMessages($messages);
if (empty($messages)) {
return '';
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flattening messages first allows us to return early, before doing any attribute aggregation.


// Prepare attributes for opening tag
$attributes = array_merge($this->attributes, $attributes);
$attributes = $this->createAttributesString($attributes);
if (! empty($attributes)) {
$attributes = ' ' . $attributes;
}

// Flatten message array
$escapeHtml = $this->getEscapeHtmlHelper();
$messagesToPrint = [];
$translator = $this->getTranslator();
$textDomain = $this->getTranslatorTextDomain();
$messageCallback = function ($item) use (&$messagesToPrint, $escapeHtml, $translator, $textDomain) {
$item = $translator ? $translator->translate($item, $textDomain) : $item;
$messagesToPrint[] = $escapeHtml($item);
};
array_walk_recursive($messages, $messageCallback);

if (empty($messagesToPrint)) {
return '';
}

// Generate markup
$markup = sprintf($this->getMessageOpenFormat(), $attributes);
$markup .= implode($this->getMessageSeparatorString(), $messagesToPrint);
$markup .= implode($this->getMessageSeparatorString(), $messages);
$markup .= $this->getMessageCloseString();

return $markup;
Expand Down Expand Up @@ -185,4 +187,56 @@ public function getMessageSeparatorString()
{
return $this->messageSeparatorString;
}

/**
* Set the flag detailing whether or not to translate error messages.
*
* @param bool $flag
* @return self
*/
public function setTranslateMessages($flag)
{
$this->translateErrorMessages = (bool) $flag;
return $this;
}

/**
* @param array $messages
* @return array
*/
private function flattenMessages(array $messages)
{
return $this->translateErrorMessages && $this->getTranslator()
? $this->flattenMessagesWithTranslator($messages)
: $this->flattenMessagesWithoutTranslator($messages);
}

/**
* @param array $messages
* @return array
*/
private function flattenMessagesWithoutTranslator(array $messages)
{
$messagesToPrint = [];
array_walk_recursive($messages, function ($item) use (&$messagesToPrint) {
$messagesToPrint[] = $item;
});
return $messagesToPrint;
}

/**
* @param array $messages
* @return array
*/
private function flattenMessagesWithTranslator(array $messages)
{
$translator = $this->getTranslator();
$textDomain = $this->getTranslatorTextDomain();
$messagesToPrint = [];
$messageCallback = function ($item) use (&$messagesToPrint, $translator, $textDomain) {
$messagesToPrint[] = $translator->translate($item, $textDomain);
};
array_walk_recursive($messages, $messageCallback);
return $messagesToPrint;
}
}
60 changes: 60 additions & 0 deletions test/View/Helper/FormElementErrorsTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,29 @@
namespace ZendTest\Form\View\Helper;

use Zend\Form\Element;
use Zend\Form\Form;
use Zend\Form\View\Helper\FormElementErrors as FormElementErrorsHelper;
use Zend\Validator\AbstractValidator;

class FormElementErrorsTest extends CommonTestCase
{
/**
* @var null|\Zend\Validator\Translator\TranslatorInterface
*/
protected $defaultTranslator;

public function setUp()
{
$this->defaultTranslator = AbstractValidator::getDefaultTranslator();
$this->helper = new FormElementErrorsHelper();
parent::setUp();
}

public function tearDown()
{
AbstractValidator::setDefaultTranslator($this->defaultTranslator);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to add this, as otherwise having a default translator caused other tests to fail.

}

public function getMessageList()
{
return [
Expand Down Expand Up @@ -76,6 +89,53 @@ public function testRendersErrorMessagesUsingUnorderedListTranslated()
// @codingStandardsIgnoreEnd
}

public function testRendersErrorMessagesWithoutDoubleTranslation()
{
$form = new Form('test_form');
$form->add([
'name' => 'test_element',
'type' => Element\Color::class,
]);
$form->setData(['test_element' => 'This is invalid!']);

$mockValidatorTranslator = $this->createMock('Zend\Validator\Translator\TranslatorInterface');
$mockValidatorTranslator
->expects(self::once())
->method('translate')
->willReturnCallback(
function ($message) {
self::assertEquals(
'The input does not match against pattern \'%pattern%\'',
$message,
'Unexpected translation key.'
);

return 'TRANSLATED: The input does not match against pattern \'%pattern%\'';
}
);

AbstractValidator::setDefaultTranslator($mockValidatorTranslator, 'default');

self::assertFalse($form->isValid());

$mockFormTranslator = $this->createMock('Zend\I18n\Translator\Translator');
$mockFormTranslator
->expects(self::never())
->method('translate');

$this->helper->setTranslator($mockFormTranslator);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This setup doesn't make sense.

If the helper has a translator - and it does - then it WILL be called. Saying you don't expect it to be called indicates that you think the solution is not to call it, which may not be correct.

The difference, though, is what messages it attempts to translate. render() loops through the result of $element->getMessages(). This value is set by extracting messages from a zend-inputfilter Input instance, and those values are extracted from the validator chain attached to the input filter.

Now, with validators, translation happens at message creation, which happens as soon as $validator->error() is called internally with a message key. At that point, the translator is provided the message matching that message key and the configured text domain, and it then returns the translated message.

What this means is that by the point we get to the FormElementErrors view helper, if the validator translator has translated the message, then the translator associated with the view helper will be asked to attempt to translate the translated message.

In reading back through #104, you got this far.

What I'm asserting is: this test does not prove double translation. It proves that when a translator is attached to the view helper, it will get called. What you should be demonstrating is that it gets called with the translated message from the previous validator.

Now, the next question is: how should we approach a fix, and/or should we?

As you note in #104, the effects can be none, minor, or major. I would argue that the number of scenarios where it would have any effect are incredibly few, and easily avoidable. Check your translations to see if any match the translation keys; if you have any, make changes.

That said, if you are affected, and there's good reason not to fix the translations themselves, there are already solutions.

  • Do not pass a translator to the helper before invoking it, or pass null to setTranslator() immediately prior to invoking it. Unlike zend-validator, we do not have a concept of a default translator for translatable view helpers. You have to pass it to each helper. Using one of these approaches allows you to say, "do not perform translation when invoked" to this helper.
  • Choose to either translate error messages at the validator or at render. In other words, you can choose not to set a default translator and/or text domain for validators, and only translate at the view layer, or vice versa. If you will be translating other items at the view layer (e.g., labels, descriptions, etc.), set a null default translator for validators.

Otherwise, I do not think there are any solutions we can attempt that allow us to keep the feature from #104 and prevent the double translation scenario. And, frankly, I'd argue translation should only ever happen in the view layer, not the validators (the proposed zend-datavalidator component, for example, does not provide ANY translation capabilities; all translation is accomplished either via decorators or in the view).

Copy link
Contributor Author

@MatthiasKuehneEllerhold MatthiasKuehneEllerhold May 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Saying you don't expect it to be called indicates that you think the solution is not to call it, which may not be correct.

You're correct. But there is no easy way to fix the unit test without deciding which behavior is correct (translation at validator vs translation at render time).
Thats why I opted to use the pre-#104 behavior.

That said, if you are affected, and there's good reason not to fix the translations themselves, there are already solutions.

The effect for our scenario is that we're getting a lot of 'missing translation for key "already translated"' log entries.

Do not pass a translator to the helper before invoking it, or pass null to setTranslator() immediately prior to invoking it.

This is done automatically in Zend\View\HelperPluginManager->injectTranslator(). I'd have to do some magic to null it after the initializer is done. Sadly delegators wont work because they are invoked before the initializers...

Choose to either translate error messages at the validator or at render.

Pre-#104 error messages were translated at the validator.
Post-#104 error messages are translated at both points in time.

If I translatev the error messages at render time I lose the placeholder capabilities. Thats why the validator has to translate the error messages and then replace the placeholders with their actual values.

The FormElementErrors helper always assumed that all messages were already translated. There was no ambiguity there and everybody could manage. If I needed a custom message (that was not originating from a validator) I'd set a translated message into the element.

#104 changed this. Now the FormElementErrors helper always assumes that all message are NOT already translated and tries to translate them again. If we'd follow this train of thought we'd have to remove the translations capabilities from the validators... and lose the capability to use placeholders.

Thats why I think #104 was wrong from a basic level. I doesnt enable anything new and potentionally breaks existing setups.

I've yet to take a look at the new component you've mentioned.

Copy link
Member

@froschdesign froschdesign May 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MatthiasKuehneEllerhold
Link (RFC) to the new validation component: https://discourse.zendframework.com/t/rfc-new-validation-component/208

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is done automatically in Zend\View\HelperPluginManager->injectTranslator(). I'd have to do some magic to null it after the initializer is done. Sadly delegators wont work because they are invoked before the initializers...

I'd forgotten about that part, and that does change my analysis.

I'm re-opening; I have no idea how we'll approach a solution, nor how soon, but that aspect definitely means we need to address this somehow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've looked at the RFC and a little bit at the new component. Sounds promising! Is there a timeline when it'll be ready and will the old zend-validator component be deprecated then?

$this->assertTrue($this->helper->hasTranslator());

$this->helper->setTranslatorTextDomain('default');

// Disable translation...
$this->helper->setTranslateMessages(false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this line to your test, as it's now testing what happens when we disable translations.


$markup = $this->helper->render($form->get('test_element'));

$this->assertRegexp('#^<ul>\s*<li>TRANSLATED#s', $markup);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I modified this regex, as the main thing is we're looking for the string "TRANSLATED"; the previous regex never matched, and it was more difficult to shoe-horn it to work than to do the above.

}

public function testCanSpecifyAttributesForOpeningTag()
{
$messages = $this->getMessageList();
Expand Down