zendframework / zend-form

Form component from Zend Framework
BSD 3-Clause "New" or "Revised" License
69 stars 87 forks source link

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

Closed MatthiasKuehneEllerhold closed 6 years ago

MatthiasKuehneEllerhold commented 6 years ago

As requested in https://github.com/zendframework/zend-form/pull/104#issuecomment-350272000 I provide a negative unit test to prove that error message from zend-validator will get doubly translated.

Some notes:

weierophinney commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

I also prefer this solution. No need for:

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:

weierophinney commented 6 years ago

@MatthiasKuehneEllerhold — I have pushed changes to your branch:

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!

MatthiasKuehneEllerhold commented 6 years ago

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 commented 6 years ago

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