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

[MatthiasKuehneEllerhold]

As requested in #104 (comment) I provide a negative unit test to prove that error message from zend-validator will get doubly translated.

Some notes:

• This tests uses some actual objects instead of mocks.
  • Especially Zend\Form\Form, Zend\Form\Element\Color, Zend\Validator\Regex, etc...
  • They should be replaced, but I wanted to show the error in a real-world example and not in a fully mocked-out test case that only shows what I tell them to.
• Zend\Form\Form uses duck-typing to fulfill Zend\InputFilter\InputFilterAwareInterface but not actually implementing it. Why is there no "implements InputFilterAwareInterface"?
• Zend\Validator\Translator\TranslatorInterface and Zend\I18n\Translator\TranslatorInterface are 2 different interfaces and only Zend\Mvc\I18n\Translator implements both. But thats not a dependency of this module.
  • Thats why I needed two different translator-mocks.

[weierophinney]

Why is there no "implements InputFilterAwareInterface"?

That code likely pre-dates the interface. That said, we're removing "Aware" interfaces everywhere, so I'm not sure it makes sense to add that check at this time, either.

[weierophinney]

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).

[MatthiasKuehneEllerhold]

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.

[froschdesign]

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

[weierophinney]

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.

[MatthiasKuehneEllerhold]

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?

[weierophinney]

In looking through this, I think there's really only one way we can achieve both the goals of #104 and resolve the issues you brought up in here: have two separate helpers, and a way to tell FormRow which one to use. This way the default behavior can revert to what it was pre-#104, and users can use the translatable variant on-demand as a helper, or tell formRow() which one to use prior to invoking it. My thought is something like this:

echo $this->formElementErrors($element); // no translation
echo $this->translatedFormElementErrors($element); // translated version

echo $this->formRow($element); // no translation
echo $this->formRow()->setErrorsHelper($this->translatedFormElementErrors()); // w/translation
$this->formRow()->setErrorsHelper($this->formElementErrors()); // reset to original variant

Thoughts?

[MatthiasKuehneEllerhold]

Wouldn't it be enough to make this behavior togglable in the current helper?

$this->formElementErrors()->setTranslateMessages(true);
echo $this->formRow($element); // helper will translate messages

$this->formElementErrors()->setTranslateMessages(false);
echo $this->formRow($element); // helper will not translate messages

I'd argue that the default value should be false (= no translation) because this was the behavior of most of the libraries lifetime and it only recently changed.
On the other hand, setting it to false would be another BC break and should be avoided for all users that already have arrenged themselves with the new behavior.

[froschdesign]

Wouldn't it be enough to make this behavior togglable in the current helper?

I also prefer this solution. No need for:

• new view helpers
• extends of the formRow helper
• or other special handling

echo $this->formElementErrors($element); // no translation
echo $this->formElementErrors($element)->setTranslateMessages(true); // translated version

And the view helper formElementErrors already has some methods for messages:

• setMessageCloseString()
• getMessageCloseString()
• setMessageOpenFormat()
• getMessageOpenFormat()
• setMessageSeparatorString()
• getMessageSeparatorString()

[weierophinney]

@MatthiasKuehneEllerhold — I have pushed changes to your branch:

• I rebased
• I fixed a testing issue introduced by your new test
• I added the new method, setTranslateMessages()

As noted, I've kept the default to true, as that is the behavior currently. However, usage will be exactly like @froschdesign has noted above (you can set the flag and render in one step, or set it once, use it everywhere).

If both of you could review, I'd appreciate it!

[weierophinney]

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

[weierophinney]

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

[weierophinney]

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

[weierophinney]

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

[weierophinney]

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

[weierophinney]

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.

[MatthiasKuehneEllerhold]

Your edits look fine to me! Thank you very much for listening and keeping this issue on your radar.

I have just a small correction for the example calls from @froschdesign:

echo $this->formElementErrors($element); // no translation
echo $this->formElementErrors()->setTranslateMessages(true)->render($element); // translated version

Otherwise: if you call the helper with an $element it'll return a string and not the Helper itself!
Seems like you fixed it in the release notes yourself.

[froschdesign]

@MatthiasKuehneEllerhold

I have just a small correction for the example calls…

That's right, only the navigation view helpers have a __toString() method.

That's why we need the documentation. (#203)