[make:reset-password] Translate exception reasons provided by ResetPasswordBundle #1059

bocharsky-bw · 2022-01-31T19:07:28Z

If there's Symfony\Contracts\Translation\TranslatorInterface service available in the project - Maker will generate a slightly different controller's code to inject translator service and translate exception reasons out of the box.

Available after: SymfonyCasts/reset-password-bundle#206

codedmonkey

Looks good :)

src/Maker/MakeResetPassword.php

bocharsky-bw · 2022-02-11T14:58:03Z

Thank you for the review, @codedmonkey !

bocharsky-bw · 2022-02-11T15:08:05Z

src/Maker/MakeResetPassword.php

+
+        $optionalTranslatorParams = [];
+        if ($isTranslatorAvailable = interface_exists(TranslatorInterface::class)) {
+            $optionalTranslatorParams['translator_class_details'] = $generator->createClassNameDetails(TranslatorInterface::class, '\\');


Fairly speaking, I'm not sure we need to create class name details for translator as well. I just did the same way password_hasher_class_details is implemented. Should we just hardcode translator class instead?

Good catch. I would accept it like this but that would be cleaner, the translator is rarely variable.

Thank you for sharing some thoughts about it! I made this change, the logic would be simpler this way unless we really need to use some benefits of that createClassNameDetails()

jrushlow · 2022-02-14T16:20:22Z

https://github.com/symfonycasts/reset-password-bundle/releases/tag/v1.12.0 implements translation support for translations

jrushlow

Awesome thanks @bocharsky-bw - this will pair nicely with the work you did over in ResetPasswordBundle

jrushlow

@bocharsky-bw I think the <?php PHP_EOL ?> is causing issues on windows (appveyor tests).. try removing those EOL tags and we should be good to go

src/Maker/MakeResetPassword.php

weaverryan · 2022-02-15T19:22:10Z

src/Maker/MakeResetPassword.php

@@ -111,6 +112,7 @@ public function configureDependencies(DependencyBuilder $dependencies): void
        $dependencies->addClassDependency(Validation::class, 'symfony/validator');
        $dependencies->addClassDependency(SecurityBundle::class, 'security-bundle');
        $dependencies->addClassDependency(AppVariable::class, 'twig');
+        $dependencies->addClassDependency(TranslatorInterface::class, 'symfony/translation', false);


Do we want to add this here? I believe the end result is if you are missing any required dependencies, then the composer require will now include symfony/translation in the error message. But, the user doesn't NEED to install this package, and I think the intention is just to detect IF they have it, so that we can know how to generate the controller.

@weaverryan The intention is meant to be exactly like you described! If users have a translator installed - they will have translated exceptions. But without this line tests were failing somehow... and I passed false as the 3rd argument there that already says this dependency is "optional" and not "required". Is it enough? Or do I understand that 3rd option wrong and it's not about specifying required/optional dependency?

Ah, if it was to get the tests working, then try adding ->addExtraDependencies() in the test case - e.g.

maker-bundle/tests/Maker/MakeEntityTest.php

Line 81 in 112ed50

->addExtraDependencies('api')

Normally, when you run a test, it will find all of the dependencies configured in this method and will install those. So probably this solution worked because it caused the symfony/translator to be installed. BUT, we don't really want to recommend it as a package, so probably the better thing to do is to not include it here, but then hint to the test system that it SHOULD install it.

However, if you didn't include it here AND you didn't include it in setExtraDependencies(), in theory, the tests should NOT have failed. In that situation, the test would have been testing the "what happens of symfony/translator is not installed situation". We may need to have one test that does NOT include symfony/translator and another that DOES include it, and we make sure both work.

OK, failed tests were valid due to empty line, fixed in a00d568

And thanks for the tip about addExtraDependencies()! I added a new test to cover the case installed Translator in 6e64745

src/Maker/MakeResetPassword.php

bocharsky-bw · 2022-02-15T20:03:42Z

I think the is causing issues on windows (appveyor tests).. try removing those EOL tags and we should be good to go

@jrushlow Thanks for the tip! Hm, I tried to replace PHP_EOL with "\n" to see if tests will pass. But without that new line char at the end the line pops up to the previous in the rendered HTML, i.e. something like:

        if ($form->isSubmitted() && $form->isValid()) {
            return $this->processSendingPasswordResetEmail(
                $form->get('email')->getData(),
                $mailer,                $translator            );
        }

Let's see if "\n" helps

bocharsky-bw · 2022-02-15T21:10:44Z

Tests are green! Though this question is still open probably: #1059 (comment)

bocharsky-bw · 2022-02-16T10:50:07Z

tests/Maker/MakeResetPasswordTest.php

+
+                $this->assertSame('App\Repository\ResetPasswordRequestRepository', $resetPasswordConfig['symfonycasts_reset_password']['request_password_repository']);
+
+                $this->runResetPasswordTest($runner, 'it_generates_with_normal_setup.php');


I hope it's OK to reuse this it_generates_with_normal_setup.php here because I literally just copy/pasted the it_generates_with_normal_setup test completely where just added addExtraDependencies('symfony/translation')

I think we could remove all the assertions inside of run() except for $this->assertStringContainsString('Success', $output); and $this->runResetPasswordTest($runner, 'it_generates_with_normal_setup.php');. So, just make it a simpler "smoke test" since the earlier test already covers all the details

Makes sense 👍

…sswordBundle

weaverryan · 2022-02-16T15:29:09Z

Thank you Victor!

This PR was merged into the 1.0-dev branch. Discussion ---------- [release] prep v1.37.0 # RELEASE Hi Makers! This release handles several bug fixes within MakerBundle and adds support for translating ResetPasswordBundle Exception messages, using constants instead of strings with `make:voter`, improved types for `Collection` getter methods in Entities, and support for attributes in `make:registration`! Diff: v1.36.4...v1.37.0 Happy making! --- # CHANGELOG ## [v1.37.0](https://github.com/symfony/maker-bundle/releases/tag/v1.37.0) *February 15th, 2022* ### Feature - [#1062](#1062) - [MakeRegistration] add support for verify email attributes - *`@jrushlow`* - [#1059](#1059) - [make:reset-password] Translate exception reasons provided by ResetPasswordBundle - *`@bocharsky`-bw* - [#1057](#1057) - [Voter] Refactor attributes - *`@mdoutreluingne`* - [#1040](#1040) - [make:entity] Chaing getter PHPDoc return type on Collection - *`@mehdibo`* ### Bug Fix - [#1060](#1060) - Add missing Passport use statement - *`@bocharsky`-bw* - [#1032](#1032) - [reset-password] Coding standards - Twig - *`@seb`-jean* - [#1031](#1031) - [verify-email] Coding standards - Twig - *`@seb`-jean* - [#1027](#1027) - Fixing wrong messaging in make:auth about checking password in final steps - *`@weaverryan`* - [#985](#985) - [make:auth] fix security controller attributes - *`@jrushlow`* Commits ------- 52063a9 [release] prep v1.37.0

bocharsky-bw mentioned this pull request Jan 31, 2022

RFC: Add translations for exception reasons SymfonyCasts/reset-password-bundle#206

Merged

bocharsky-bw mentioned this pull request Feb 10, 2022

How translate error messages? SymfonyCasts/reset-password-bundle#125

Closed

codedmonkey reviewed Feb 11, 2022

View reviewed changes

src/Maker/MakeResetPassword.php Outdated Show resolved Hide resolved

src/Maker/MakeResetPassword.php Outdated Show resolved Hide resolved

bocharsky-bw commented Feb 11, 2022

View reviewed changes

codedmonkey approved these changes Feb 12, 2022

View reviewed changes

jrushlow approved these changes Feb 15, 2022

View reviewed changes

jrushlow requested changes Feb 15, 2022

View reviewed changes

weaverryan reviewed Feb 15, 2022

View reviewed changes

jrushlow changed the title ~~Translate exception reasons in symfonycasts/reset-password-bundle~~ [make:reset-password] Translate exception reasons provided by ResetPasswordBundle Feb 15, 2022

jrushlow mentioned this pull request Feb 15, 2022

[release] prep v1.37.0 #1070

Merged

bocharsky-bw commented Feb 16, 2022

View reviewed changes

[make:reset-password] Translate exception reasons provided by ResetPa…

b771178

…sswordBundle

weaverryan force-pushed the reset-password-translate-reason branch from 8ad30eb to b771178 Compare February 16, 2022 15:28

weaverryan merged commit 64b8399 into symfony:main Feb 16, 2022

bocharsky-bw deleted the reset-password-translate-reason branch February 16, 2022 15:32

bocharsky-bw mentioned this pull request Feb 22, 2022

[make:registration-form] Translate reasons for VerifyEmailBundle if translator available #1076

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[make:reset-password] Translate exception reasons provided by ResetPasswordBundle #1059

[make:reset-password] Translate exception reasons provided by ResetPasswordBundle #1059

bocharsky-bw commented Jan 31, 2022 •

edited

Loading

codedmonkey left a comment

bocharsky-bw commented Feb 11, 2022

bocharsky-bw Feb 11, 2022

codedmonkey Feb 12, 2022

bocharsky-bw Feb 14, 2022

jrushlow commented Feb 14, 2022

jrushlow left a comment

jrushlow left a comment

weaverryan Feb 15, 2022

bocharsky-bw Feb 15, 2022

weaverryan Feb 16, 2022

bocharsky-bw Feb 16, 2022

bocharsky-bw commented Feb 15, 2022

bocharsky-bw commented Feb 15, 2022

bocharsky-bw Feb 16, 2022

weaverryan Feb 16, 2022

bocharsky-bw Feb 16, 2022

weaverryan commented Feb 16, 2022


		$this->assertSame('App\Repository\ResetPasswordRequestRepository', $resetPasswordConfig['symfonycasts_reset_password']['request_password_repository']);

		$this->runResetPasswordTest($runner, 'it_generates_with_normal_setup.php');

[make:reset-password] Translate exception reasons provided by ResetPasswordBundle #1059

[make:reset-password] Translate exception reasons provided by ResetPasswordBundle #1059

Conversation

bocharsky-bw commented Jan 31, 2022 • edited Loading

codedmonkey left a comment

Choose a reason for hiding this comment

bocharsky-bw commented Feb 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrushlow commented Feb 14, 2022

jrushlow left a comment

Choose a reason for hiding this comment

jrushlow left a 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

bocharsky-bw commented Feb 15, 2022

bocharsky-bw commented Feb 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan commented Feb 16, 2022

bocharsky-bw commented Jan 31, 2022 •

edited

Loading