Make redirect url fallback configurable #190

AsfalothDE · 2018-06-07T08:08:00Z

Because one of my projects needs to run in a subfolder, I needed to make the hardcoded "/" redirect fallback configurable. I hope I did it correctly. :)

oleg-andreyev · 2018-06-07T13:25:27Z

Can you describe your case? How you're running your project?

oleg-andreyev · 2018-06-07T13:26:22Z

Also you need to adjust unit tests, they should not fail.

AsfalothDE · 2018-06-07T13:30:51Z

My current symfony installation is not running in root (e.g. domain.com/homepage) but in a subfolder (domain.com/subfolder/homepage). The redirect fallback after theme switching leads me to "/" which basically means "domain.com/homepage". But I need the redirect to "domain.com/subfolder/homepage". Therefore I altered the redirect fallback code to use a configurable route instead of the hard-coded "/".

I will check the unit tests, sorry, forgot about that (first contribution...).

AsfalothDE · 2018-06-13T14:11:40Z

Hi, sorry for the late response. The unit tests should complete without errors now. :) Also I refactored my code slightly to not inject the whole container but only the needed routing component.

lsmith77 · 2018-06-13T14:21:00Z

Controller/ThemeController.php

     */
-    public function __construct(ActiveTheme $activeTheme, array $themes, array $cookieOptions = null)
+    public function __construct(ActiveTheme $activeTheme, array $themes, array $cookieOptions = null, RouterInterface $router, $defaultRoute = '/')


default route probably should be the name of an actual route now, no?

so I think what we should probably do us to allow null for $defaultRoute and then when we do the redirect, check if its null and in that case fallback to the /

Yeah, that's a good idea. I will change that. :)

AsfalothDE · 2018-06-13T14:33:50Z

Done :)

lsmith77

I didn’t check in detail but do we still have a test for the old behvior because ->generate("/“) should have caused test failures

lsmith77 · 2018-06-13T14:30:32Z

Controller/ThemeController.php

@@ -73,7 +88,13 @@ public function switchAction(Request $request)

        $this->activeTheme->setName($theme);

-        $url = $request->headers->get('Referer', '/');
+        if ($defaultRoute) {


it would be good to not duplicate the get() call on the headers property. so i will either remove the 2nd parameter and then check for null in $url and in that case either use / or compute the route. or compute the fallback route before calling the get() method

AsfalothDE · 2018-06-13T14:43:11Z

Hm, not sure if I understood you correctly - but the old behaviour was never ->generate("/") but simply "/". So in my opinion there is no test needed for that, because using no redirect route at all will just fallback to "/" as before. Or do I miss something?

lsmith77 · 2018-06-13T14:46:18Z

yes and no. before you made the change to set defaultRoute to null if no fallabck route was set, I would have exepcted the tests if the case of not setting a route was still properly tested. since they didn’t fail, I assume this case is not tested (anymore)

AsfalothDE · 2018-06-13T14:52:49Z

I'm not sure because I'm new to this bundle but I think the whole redirect functionality was never tested. At least I don't find anything regarding this. I'm a bit at a loss at where to add it in the right way.

lsmith77 · 2018-06-13T15:43:49Z

ah that is indeed possible.

only on my ipad atm .. but it appears that at least parts of the ThemeController are being tested inside https://github.com/liip/LiipThemeBundle/blob/a78e762b68c6a44096f6a4fe4ca6d16a00952ebe/Tests/UseCaseTest.php

AsfalothDE · 2018-06-13T15:52:02Z

Yeah, that's the test I had to modify to fix the failing build. But that tests only cookie and switching functionality, not the redirect functionality. I really think the redirect was never tested before just because it was so basic.

lsmith77 · 2018-06-13T16:02:45Z

yeah but maybe this would serve as inspiration for a new test. anyway I truly to have a closer look and give more concrete hints tonight or tomorrow

AsfalothDE · 2018-06-13T18:44:19Z

Okay, great, thanks so far! :)

oleg-andreyev · 2018-06-07T13:23:57Z

Controller/ThemeController.php

    {
        $this->activeTheme = $activeTheme;
        $this->themes = $themes;
        $this->cookieOptions = $cookieOptions;
+        $this->container = $container;


Please inject router and parameter instead whole container

This change is already done. :) See later commits.

oleg-andreyev · 2018-06-13T18:47:43Z

DependencyInjection/Configuration.php

@@ -41,6 +41,7 @@ public function getConfigTreeBuilder()
                    ->prototype('scalar')->end()
                ->end()
                ->scalarNode('active_theme')->defaultNull()->end()
+                ->scalarNode('redirect_fallback')->defaultValue('homepage')->end()


homepage may not exist for other people who are using this bundle, it's better to put /

Okay, fair point, I will change that to use null as default as prepared in ThemeController.php already.

oleg-andreyev · 2018-06-13T18:51:22Z

Controller/ThemeController.php

-        $url = $request->headers->get('Referer', '/');
+
+        if ($this->defaultRoute) {
+          $redirect = $this->router->generate($this->defaultRoute);


because we cannot use homepage as default route, and will probably use hardcoded / (see comment to Configuration), you no longer need router in this controller

The point of this change is not to use a hardcoded route back to "/", because in some cases Symfony may not run in root domain. In a later commit I added "/" as a fallback in case there is no route set in config, so we should have that covered.

Default configuration for route is now "null". When there is a route given in the configuration file, the controller uses the router to redirect back to that route. If no route is given in configuration, it defaults to "null" and the redirect goes to "/" as before. :)

lsmith77 · 2018-06-14T11:11:04Z

@AsfalothDE Just had a look: I would do a unit test.

ie. in a new test case in UseCaseTest, make an instance of ThemeController passing in the relevant dependencies (I would use a real instance of ActiveTheme and not a mock object but for the RouterInterface I would recommend using a mock) and then call switchAction() on this instance and then introspect the returned Response instance if the referrer is set as you expect it.

If you are motivated it would also be great if you could add a test to check if the cookieOptions are handled properly.

Does this give you enough pointers to get started on this?

AsfalothDE · 2018-06-14T12:06:09Z

@lsmith77, thanks so far, I will look into this and try to add the test! :)

lsmith77 · 2018-06-14T20:19:49Z

ok great .. thank you for investing the time into finalizing this PR

AsfalothDE · 2018-06-15T09:40:49Z

@lsmith77 I added a test in UseCaseTest (not committed for now), but I ran into a (hopefully minor) problem. The request mock currently sets a referer URL ("/") in the headers. But when there is a referer URL set, my whole fallback route logic is never used (and therefore never tested), because it's a fallback in case there is NO referer at all. I'm not sure if I should change the request mock to remove the referer and HOW to remove it. Do you have an idea? Also I'm not sure if that would affect the other tests.

lsmith77 · 2018-06-15T09:47:14Z

with Request mock, you mean the Request instance? I don’t think you need to mock the Request object since its essentially just a value object at any rate if it defaults to / then you could try to setting it to null by overriding the headers attribute. but it would to me also mean that most likely the code doesnt really do what you desire.

AsfalothDE · 2018-06-15T09:51:07Z

I mean the method in line 71 (https://github.com/liip/LiipThemeBundle/blob/master/Tests/UseCaseTest.php#L71). The redirect code is working, because it redirects to the referer ("/") as expected, so that is fine, but in this case I want to test the behaviour without the referer (my fallback code), and that is not possible because of the referer set in the request mock. I tried setting it to null and removing it completely, but I think I've not totally understood how this mock works and that's why I'm struggling. I don't want to break anything.

lsmith77 · 2018-06-15T09:59:18Z

ok. just commit what you have. this will make it easier for me to give you pointers.

AsfalothDE · 2018-06-15T12:10:23Z

I committed my changes to UseCastTest.php. I commented out the actual test I added (line 172), because that would break the build at the moment because of the still existing referer. Also I added a method to mock the router and make it return my "test_route" route and I added the assertion for the route.

lsmith77 · 2018-06-16T07:32:53Z

@AsfalothDE I send a PR to your PR branch :)

AsfalothDE#1

first up sorry .. I was mostly doing reviews of your PR on my iphone or ipad so I didn't have a full overview. so I didn't realize that the current tests were already mocking the Request when I suggested to you to not mock the Request. I realize now that this must have been confusing.

second I would recommend to create a branch on your side next time when you propose changes. if for example for some reason your PR does not get accepted, then your master would contain changes that would not be in the liip master.

anyway, I think with these changes in my PR, we should be all set except for one last detail: documenting your use case and how to configure in that case inside the README.md

fixed tests by removing the Request mock

AsfalothDE · 2018-06-18T10:15:09Z

@lsmith77 Okay! :) Thank you so much for your help and reviews! I will add the documentation ASAP :)

AsfalothDE · 2018-06-18T12:12:13Z

@lsmith77 Documentation is added :)

lsmith77 · 2018-06-18T19:16:31Z

Controller/ThemeController.php

     */
-    public function __construct(ActiveTheme $activeTheme, array $themes, array $cookieOptions = null)
+    public function __construct(ActiveTheme $activeTheme, array $themes, array $cookieOptions = null, RouterInterface $router, $defaultRoute = null)


can you also make the $router parameter nullable to maintain BC?

lsmith77 · 2018-06-18T19:17:07Z

Controller/ThemeController.php

@@ -73,7 +88,15 @@ public function switchAction(Request $request)

        $this->activeTheme->setName($theme);

-        $url = $request->headers->get('Referer', '/');
+
+        if ($this->defaultRoute) {


once the router property is nullable, you need to check if its set here as well

lsmith77 · 2018-06-18T19:18:08Z

ok great .. I think the last detail is handling the BC issues I noted and then we can merge.

Make redirect url fallback configurable

2bfbe41

Fix unit tests

e961202

lsmith77 reviewed Jun 13, 2018

View reviewed changes

Silvia Ulenberg added 3 commits June 13, 2018 16:23

Use / as fallback when there is no route given

92ba4e6

Fix missing $this…

3b49d2c

Make statement CS compliant

ef72908

lsmith77 suggested changes Jun 13, 2018

View reviewed changes

Remove redundant code

741ca10

oleg-andreyev suggested changes Jun 13, 2018

View reviewed changes

Set default value of redirect_fallback to null

ab6eb14

Draft for routing test

be1e60d

fixed tests by removing the Request mock

7537a58

Merge pull request #1 from liip/AsfalothDE-master

08aef0e

fixed tests by removing the Request mock

Add documentation for redirect_fallback

068dbb0

lsmith77 reviewed Jun 18, 2018

View reviewed changes

Make redirect url fallback configurable #190

Are you sure you want to change the base?

Make redirect url fallback configurable #190

Conversation

AsfalothDE commented Jun 7, 2018

oleg-andreyev commented Jun 7, 2018

oleg-andreyev commented Jun 7, 2018

AsfalothDE commented Jun 7, 2018

AsfalothDE commented Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AsfalothDE commented Jun 13, 2018

lsmith77 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AsfalothDE commented Jun 13, 2018

lsmith77 commented Jun 13, 2018

AsfalothDE commented Jun 13, 2018

lsmith77 commented Jun 13, 2018

AsfalothDE commented Jun 13, 2018

lsmith77 commented Jun 13, 2018 via email

AsfalothDE commented Jun 13, 2018

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

lsmith77 commented Jun 14, 2018

AsfalothDE commented Jun 14, 2018

lsmith77 commented Jun 14, 2018

AsfalothDE commented Jun 15, 2018

lsmith77 commented Jun 15, 2018 via email

AsfalothDE commented Jun 15, 2018

lsmith77 commented Jun 15, 2018

AsfalothDE commented Jun 15, 2018

lsmith77 commented Jun 16, 2018

AsfalothDE commented Jun 18, 2018

AsfalothDE commented Jun 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsmith77 commented Jun 18, 2018

AsfalothDE commented Jun 13, 2018 •

edited

Loading