Do not set the "hl" cookie if the response is an error, as the locale ma... #55

Merged
merged 1 commit into from Nov 16, 2012

Conversation

Projects
None yet
5 participants
Collaborator

acasademont commented Jul 17, 2012

...y not be properly set, thus rendering the cookie useless-

I check only if the response is a 200 status code (isOk method). Is there any other case where a cookie should be set?

As said in #45

Contributor

frosas commented Jul 17, 2012

Probably checking if isSuccessful() would be equally safe.

Collaborator

acasademont commented Jul 17, 2012

Maybe even better than isOk()

Owner

schmittjoh commented Jul 20, 2012

Yeah, let's do isSuccessful.

Collaborator

acasademont commented Jul 20, 2012

fixed!

Owner

schmittjoh commented Jul 20, 2012

Tests are failing for me. Did you run them?

Owner

schmittjoh commented Jul 20, 2012

While you are at the tests, could you please add a small functional test (there are already a couple), so we avoid regressions here.

That would be much appreciated.

Collaborator

acasademont commented Jul 20, 2012

Hope to find some time this weekend to do them. Also the docs should be updated, is there any way to edit them so you don't have to do the work?

Collaborator

acasademont commented Jul 25, 2012

Ok i see the point here. When a route is requested with /?hl=(LANG) the user is redirected to the correct /LANG/ prefix and i wasn't aware of that option. The problem is that now we are not setting the cookie only in isSuccessful() responses, and that method only returns true for codes 200-299, whereas a redirect is a 301 or 301.

So i guess the best we can do is to check if it is successful or a redirection

Collaborator

acasademont commented Jul 25, 2012

The old test and the new test should be working now :)

Owner

schmittjoh commented Aug 14, 2012

I just tried, I get the following error:

There was 1 failure:

1) JMS\I18nRoutingBundle\Tests\Functional\PrefixStrategyTest::testLanguageCookieIsSet
Failed asserting that 1 is identical to 2.

/home/johannes/workspace/jms/vendor/bundles/JMS/I18nRoutingBundle/Tests/Functional/PrefixStrategyTest.php:57
Collaborator

acasademont commented Oct 10, 2012

Sorry for the delays! I'll resume this PR as soon as I can :)

Do not set the "hl" cookie if the response is an error, as the locale…
… may not be properly set, thus rendering the cookie useless
Collaborator

acasademont commented Oct 15, 2012

Fixed and squashed everything :)

+1 to this pull request

Apart from that, I think that hl cookie should be set to whatever you've defined in

jms_i18n_routing:
default_locale: %locale%

And not to English, which I think is the default fallback if everything fails

What are your thought on that?

webspin commented Oct 30, 2012

+1 too!

If I don't add 'en' as available languages, my routes stop working after an error is encoutered...

Collaborator

acasademont commented Nov 16, 2012

@schmittjoh whate are your thoughts on this one? We really need it.

@ricardclau as you can see, the locale comes from the Request object, and as the very last option, the Request class has the 'en' language set by default. Nothing we can do about it!

schmittjoh added a commit that referenced this pull request Nov 16, 2012

Merge pull request #55 from acasademont/no_cookie_on_error
Do not set the "hl" cookie if the response is an error, as the locale ma...

@schmittjoh schmittjoh merged commit 64070a8 into schmittjoh:master Nov 16, 2012

1 check passed

default Review: No Comments — Travis: Passed
Details
Owner

schmittjoh commented Nov 16, 2012

I've merged it, thanks.

Collaborator

acasademont commented Nov 16, 2012

great! thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment