Make error message in controller translatable. #522

roelvanduijnhoven · 2014-09-04T13:09:20Z

At the moment the error message that is placed in the controller seems not to be translated.

The message is copied from the flash messenger into to the form messages. However the FormError renderer that is used by default does not translate messages. Hence no translation.

This PR fixes this by adding the Translator as an optional dependency of the UserController. If present it is used to translate.

ojhaujjwal · 2014-09-04T13:34:27Z

src/ZfcUser/Controller/UserController.php

     */
-    protected $failedLoginMessage = 'Authentication failed. Please try again.';


Why is this removed? If this property is kept, we can easily override this message which is the beauty of this module. :D

@roelvanduijnhoven Suppose, the site I am developing is not multi-language based. In that case, if this property is kept, we can easily override the message by extending the controller(ofcourse). I don't need to worry about translation in that case.

In that case you could now overwrite the getFailedLoginMessage method.

But as I argued I think overwritting the controller is not the correct way to go. Whatsmore ZfcUser comes packed with translation out-of-the-box. So I really think this property has no added value.

But to be fare. Removing the property could be seen as a BC break. I'll reintroduce it.

Also i think that message should be configuable by the moduleOptions bu( that is siomething for an other PR

This also introduces unwanted BC break.

@stijnhau We cannot make everything in the world configurable using module options. ZfcUser is intended to be simple and extendible

What unwanted BC are you talking about? I just reintroduced the property.

Now it's fine

ojhaujjwal · 2014-09-04T13:35:26Z

src/ZfcUser/Controller/UserController.php

+     */
+    protected function getFailedLoginMessage()
+    {
+        $message =' Authentication failed. Please try again.';


Instead of declaring message here, please keep it in a property

roelvanduijnhoven · 2014-09-04T13:42:16Z

The PR is updated.

Regarding the property: why would you want to override it or make it more configurable? If you want to change the text there is now a mechanism to do so: translation.

Furthermore making the text a property does not enhance it's configurability I would say. It is a protected property without any getter or setter. Thus the only way of overriding would be to extend the class. The later being a strange choice for a controller.

roelvanduijnhoven · 2014-09-04T13:42:24Z

Thanks for your input!

roelvanduijnhoven · 2014-09-04T13:43:49Z

Sorry I rebased the commit so some commits are not associated with their lines anymore.

Regarding the return value. What do you mean precisly?

stijnhau · 2014-09-04T13:48:32Z

Is that return value in a setter realy needed?
A setter that have a return value seems kinda strange to me.

roelvanduijnhoven · 2014-09-04T13:51:35Z

@stijnhau I just mimicked the existing code. But yeah, it is pretty common, and called fluent interface.

stijnhau · 2014-09-04T13:55:14Z

in this case is isn't needed so.
BNut i usnderstand why u added it because so that they all are the same.

ojhaujjwal · 2014-09-04T15:05:29Z

src/ZfcUser/Controller/UserController.php

+    public function getTranslator()
+    {
+        if (!$this->translator && $this->getServiceLocator()) {
+            $this->setTranslator($this->getServiceLocator()->get('translator'));


translator should be MvcTranslator. See https://github.com/zendframework/zf2/blob/master/library/Zend/Mvc/Service/ServiceListenerFactory.php#L62. The alias translator is defined in ZendSkeleton

fabwu · 2014-09-04T16:54:40Z

Why we don't use the flashmessanger view helper in the login view and add the translator there?

roelvanduijnhoven · 2014-09-05T10:30:07Z

@ojhaujjwal PR updated. Thanks for feedback.

ojhaujjwal · 2014-09-05T10:31:13Z

you are welcome

roelvanduijnhoven · 2014-09-15T14:19:13Z

The latest version unintentionally changed behaviour (this error message is no longer translated), thus I would say this is a serious bug.

What is the status of this one, that is keeping this from being merged?

Also #520 unintentionally changed behaviour. So I would recon a 1.2.2. release after these two problems have been fixed would be the correct way forward.

roelvanduijnhoven · 2014-12-09T11:38:16Z

It seems the bug introduced in the last release will not be fixed. Or even spoken about..

I guess any effort put into ZfcUser is better spent migrating this module away out of our code base..

Danielss89 · 2014-12-09T12:01:28Z

This commit is made to the master branch. @roelvanduijnhoven please don't use master as it's a work in progress. Instead use the tags. And if you want to do PR agains stable ZfcUser, please do the PR agains the 1.x branch.

Thank you.

roelvanduijnhoven · 2014-12-09T12:25:34Z

Allright, thanks.

What is the status of 2.0 and what is the estimated release date?

ojhaujjwal · 2014-12-09T12:31:10Z

@roelvanduijnhoven Good question!

roelvanduijnhoven · 2014-12-09T12:32:02Z

In the meantime I solved this by using reflection to set the property correctly.

Register the following class as a delegator for the ZfcUser controller:

class ZfcUserControllerDecorator implements DelegatorFactoryInterface
{
    public function createDelegatorWithName(ServiceLocatorInterface $serviceLocator, $name, $requestedName, $callback)
    {
        /** @var UserController $controller */
        $controller = $callback();

        $property = new \ReflectionProperty($controller, 'failedLoginMessage');
        $property->setAccessible(true);
        $property->setValue($controller, 'E-mailadres of wachtwoord onjuist');

        return $controller;
    }
}

ojhaujjwal · 2014-12-09T12:38:16Z

I think the idea by @wuethrich44 is much better!

ojhaujjwal · 2014-12-09T12:41:23Z

Why dont use just use translator in the view templates?

roelvanduijnhoven · 2014-12-09T12:55:16Z

These ideas revolve around touching source code in this package. This hack does not.

ojhaujjwal · 2014-12-09T12:58:46Z

How is using translator in the view a hack? IMO translator lies in View of MVC!

Danielss89 · 2014-12-12T11:52:50Z

Agree with @ojhaujjwal. Translation should be handled in the view.
POedit won't catch this though, so maybe we should create a Translate file, which just list all of the translation that can't be catched?

roelvanduijnhoven force-pushed the feature/translate-errors branch from 09aa317 to ef6c7e0 Compare September 4, 2014 13:12

Make error message in controller translatable.

205ab55

roelvanduijnhoven force-pushed the feature/translate-errors branch from ef6c7e0 to 205ab55 Compare September 4, 2014 13:12

ojhaujjwal reviewed Sep 4, 2014
View reviewed changes

Roel van Duijnhoven added 2 commits September 4, 2014 15:35

Typo.

56e2540

Change DocType.

b8ca3fb

ojhaujjwal reviewed Sep 4, 2014
View reviewed changes

stijnhau mentioned this pull request Sep 4, 2014

not translated error message #517

Closed

Reintroduce property to preserve BC.

f92bc10

ojhaujjwal reviewed Sep 4, 2014
View reviewed changes

Use correct translator service.

e4940dd

Danielss89 closed this Dec 12, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make error message in controller translatable. #522

Make error message in controller translatable. #522

roelvanduijnhoven commented Sep 4, 2014

ojhaujjwal Sep 4, 2014

ojhaujjwal Sep 4, 2014

roelvanduijnhoven Sep 4, 2014

roelvanduijnhoven Sep 4, 2014

stijnhau Sep 4, 2014

ojhaujjwal Sep 4, 2014

ojhaujjwal Sep 4, 2014

roelvanduijnhoven Sep 4, 2014

ojhaujjwal Sep 4, 2014

ojhaujjwal Sep 4, 2014

roelvanduijnhoven commented Sep 4, 2014

roelvanduijnhoven commented Sep 4, 2014

roelvanduijnhoven commented Sep 4, 2014

stijnhau commented Sep 4, 2014

roelvanduijnhoven commented Sep 4, 2014

stijnhau commented Sep 4, 2014

ojhaujjwal Sep 4, 2014

fabwu commented Sep 4, 2014

roelvanduijnhoven commented Sep 5, 2014

ojhaujjwal commented Sep 5, 2014

roelvanduijnhoven commented Sep 15, 2014

roelvanduijnhoven commented Dec 9, 2014

Danielss89 commented Dec 9, 2014

roelvanduijnhoven commented Dec 9, 2014

ojhaujjwal commented Dec 9, 2014

roelvanduijnhoven commented Dec 9, 2014

ojhaujjwal commented Dec 9, 2014

ojhaujjwal commented Dec 9, 2014

roelvanduijnhoven commented Dec 9, 2014

ojhaujjwal commented Dec 9, 2014

Danielss89 commented Dec 12, 2014

		*/
		protected $failedLoginMessage = 'Authentication failed. Please try again.';

Make error message in controller translatable. #522

Make error message in controller translatable. #522

Conversation

roelvanduijnhoven commented Sep 4, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roelvanduijnhoven commented Sep 4, 2014

roelvanduijnhoven commented Sep 4, 2014

roelvanduijnhoven commented Sep 4, 2014

stijnhau commented Sep 4, 2014

roelvanduijnhoven commented Sep 4, 2014

stijnhau commented Sep 4, 2014

Choose a reason for hiding this comment

fabwu commented Sep 4, 2014

roelvanduijnhoven commented Sep 5, 2014

ojhaujjwal commented Sep 5, 2014

roelvanduijnhoven commented Sep 15, 2014

roelvanduijnhoven commented Dec 9, 2014

Danielss89 commented Dec 9, 2014

roelvanduijnhoven commented Dec 9, 2014

ojhaujjwal commented Dec 9, 2014

roelvanduijnhoven commented Dec 9, 2014

ojhaujjwal commented Dec 9, 2014

ojhaujjwal commented Dec 9, 2014

roelvanduijnhoven commented Dec 9, 2014

ojhaujjwal commented Dec 9, 2014

Danielss89 commented Dec 12, 2014