Make redirect url fallback configurable

[AsfalothDE]

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]

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

[oleg-andreyev]

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

[AsfalothDE]

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]

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.

[AsfalothDE]

Done :)

[AsfalothDE]

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]

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]

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]

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]

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]

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]

Okay, great, thanks so far! :)

[lsmith77]

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

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

[lsmith77]

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

[AsfalothDE]

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

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]

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]

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

[AsfalothDE]

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]

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

https://github.com/AsfalothDE/LiipThemeBundle/pull/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

[AsfalothDE]

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

[AsfalothDE]

@lsmith77 Documentation is added :)

[lsmith77]

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