10000 LoginLink create get Request Locale by roromix · Pull Request #40641 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

LoginLink create get Request Locale #40641

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Apr 2, 2021
Merged

LoginLink create get Request Locale #40641

merged 1 commit into from
Apr 2, 2021

Conversation

roromix
Copy link
Contributor
@roromix roromix commented Mar 30, 2021 •
edited
Loading
Q A
Branch? 5.x
Bug fix? yes
New feature? no
Deprecations? no
License MIT
Doc PR symfony/symfony-docs#15163

While writing the documentation for PR#40153 I noticed that the RequestContext :: fromRequest method does not add the '_locale' parameter automaticaly. This new pull request to set _locale parameter to the RequestContext.

@roromix roromix requested review from chalasr and wouterj as code owners March 30, 2021 14:05
@roromix roromix mentioned this pull request Mar 30, 2021
Merged
@roromix roromix changed the title LoginLink create get Request Locale [Security] LoginLink create get Request Locale Mar 30, 2021
@chalasr chalasr removed the Feature label Mar 30, 2021
@carsonbot carsonbot changed the title [Security] LoginLink create get Request Locale LoginLink create get Request Locale Mar 30, 2021
@chalasr chalasr added this to the 5.x milestone Mar 30, 2021
@chalasr
Copy link
Member
chalasr commented Mar 30, 2021

Thanks for the follow-up @roromix.
Could you add a test case (or update an existing one) for this?

@roromix
Copy link
Contributor Author
roromix commented Mar 30, 2021 •
edited
Loading

Thanks for the follow-up @roromix.
Could you add a test case (or update an existing one) for this?

@chalasr : I have updated provider

Nyholm
Nyholm requested changes Mar 31, 2021
View reviewed changes
Copy link
Member
@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you.

You are the best. Writing docs and patching bugs.

I agree with this PR and I do like the changes in LoginLinkHandler. But you need to update the test. I will pass even without any modifications to LoginLinkHandler.

@roromix roromix requested a review from Nyholm April 1, 2021 12:17
@Nyholm
Copy link
Member
Nyholm commented Apr 1, 2021

If I copy the tests you written (without the changes in LoginLinkHandler), they will still run an be green. You need to write a test that will only pass if your patch for LoginLinkHandler is applied.

@roromix
Copy link
Contributor Author
roromix commented Apr 1, 2021

If I copy the tests you written (without the changes in LoginLinkHandler), they will still run an be green. You need to write a test that will only pass if your patch for LoginLinkHandler is applied.

Sorry, i have no idea how to do that. Need help

@chalasr
Copy link
Member
chalasr commented Apr 1, 2021

Here is a patch covering this change:

diff --git a/src/Symfony/Component/Security/Http/Tests/LoginLink/LoginLinkHandlerTest.php b/src/Symfony/Component/Security/Http/Tests/LoginLink/LoginLinkHandlerTest.php
index 03dd0287d7..12b8f7da4a 100644
--- a/src/Symfony/Component/Security/Http/Tests/LoginLink/LoginLinkHandlerTest.php
+++ b/src/Symfony/Component/Security/Http/Tests/LoginLink/LoginLinkHandlerTest.php
@@ -69,7 +69,12 @@ class LoginLinkHandlerTest extends TestCase
         if ($request) {
             $this->router->expects($this->once())
                 ->method('getContext')
-                ->willReturn(new RequestContext());
+                ->willReturn($currentRequestContext = new RequestContext());
+
+            $this->router
+                ->expects($this->exactly(2))
+                ->method('setContext')
+                ->withConsecutive([$this->equalTo((new RequestContext())->fromRequest($request)->setParameter('_locale', $request->getLocale()))], [$currentRequestContext]);
         }
 
         $loginLink = $this->createLinker([], array_keys($extraProperties))->createLoginLink($user, $request);
@@ -78,19 +83,15 @@ class LoginLinkHandlerTest extends TestCase
 
     public function provideCreateLoginLinkData()
     {
-        $request = Request::create('https://example.com');
-        $request->setLocale('en');
-
         yield [
             new TestLoginLinkHandlerUser('weaverryan', 'ryan@symfonycasts.com', 'pwhash'),
             ['emailProperty' => 'ryan@symfonycasts.com', 'passwordProperty' => 'pwhash'],
-            $request,
+            Request::create('https://example.com'),
         ];
 
         yield [
             new TestLoginLinkHandlerUser('weaverryan', 'ryan@symfonycasts.com', 'pwhash'),
             ['emailProperty' => 'ryan@symfonycasts.com', 'passwordProperty' => 'pwhash'],
-            Request::create('https://example.com'),
         ];
 
         yield [

@roromix
Copy link
Contributor Author
roromix commented Apr 1, 2021

I still have a lot to learn about the Tests
Thank you @chalasr

Nyholm
Nyholm approved these changes Apr 1, 2021
View reviewed changes
Copy link
Member
@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you

@fabpot
Copy link
Member
fabpot commented Apr 2, 2021

Thank you @roromix.

@fabpot fabpot merged commit bb1e1e5 into symfony:5.x Apr 2, 2021
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Apr 21, 2021
@javiereguiluz
minor #15163 [Security] Customize generated Login Link (roromix)
5137e22 
This PR was squashed before being merged into the 5.3-dev branch.

Discussion
----------

[Security] Customize generated Login Link

Fix #15147 and add documentation for symfony/symfony#40641

Commits
-------

830b1f3 [Security] Customize generated Login Link
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@Nyholm Nyholm Nyholm approved these changes

@chalasr chalasr Awaiting requested review from chalasr

@wouterj wouterj Awaiting requested review from wouterj

Assignees
No one assigned
Labels
Bug Status: Reviewed
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.
5 participants
@roromix @chalasr @Nyholm @fabpot @carsonbot
0