Locale Handler

[ghost]

Updated the LocaleHandler to use a cookie which can be
defined in the config file

[svenkubiak]

Cool. Thanks for the enhancement. I got a couple of (small) things:

• Could you add a unit test for this
• Could you add a default value set in the Default enum for the cookie name, if the user doesnt set one
• Could you update the documentation, specially the table with the default values

Thanks.

[svenkubiak]

Why did you re-order the Default enums? Makes it hard so seet, what was changed.

[ghost]

I can undo it, but there seemed to be no particularly logical order. If you want I can put them back

[svenkubiak]

Would be nice to only see, what you have changed.

[ghost]

I have already done the Default value (
LOCALE_COOKIE_NAME("lang")) but the reordering didn't make it too clear in the diff.

Later I'll move the setting key to be under Application and update the docs (and add a unit test).

[svenkubiak]

I have already done the Default value

Sorry, missed that.

[MarkVink]

Maybe it would also be nice to add a different create() method on the CookieBuilder class in such way the developer doesn't have to bother getting the right configured cookie name;

public static CookieBuilder createLocale() {
    String name = Application.getInstance(Config.class).getLocaleCookieName();
    return new CookieBuilder(name);
}

In controller;

Cookie cookie = CookieBuilder.createLocale().value('NL').build();
return Response.withOk().andCookie(cookie).andEmptyBody();

[ghost]

That was the next thing I was going to work on, along with automatically
setting the domain and cookie security by default. I completely agree.

[svenkubiak]

@MarkVink +1

[ghost]

I wanted to create an integration test the LocaleHandler, but that's
an awful lot of data to build up first. In reality I think if there is little
need anyway.

[ghost]

Also; I need to get better at checking before I commit

[MarkVink]

I'm not sure if I agree on the changes on CookieBuilder.java;

• Do we want to set the domain by default? I'm not running a Mangoo-application in production yet, but in my expectation I will run it on localhost behind a (ssl offloading) reverse proxy (Nginx). In that case the application host (localhost) will not match the real domain.
• In my example above I did make an additional create method which will instantiate the builder. In your implementation it is used as a setter on the builder. I don't think a 'language cookie' is a property on Cookie we set.

[ghost]

Do we want to set the domain by default? I'm not running a Mangoo-application in production yet, but in my expectation I will run it on localhost behind a (ssl offloading) reverse proxy (Nginx). In that case the application host (localhost) will not match the real domain.

In that case would you not set your host as the domain? I don't know much about using reverse-proxies. Either way it's a default, and in your situation would you not be manually overriding it yourself anyway?

In my example above I did make an additional create method which will instantiate the builder. In your implementation it is used as a setter on the builder. I don't think a 'language cookie' is a property on Cookie we set.

Yeah I think your way was better, I'll do that.

[MarkVink]

Oh I didn't notice your first implementation didn't follow the builder pattern by not returning this. Now it does but you need an instance of the builder first before calling it, using the following code; CookieBuilder.create().createLocale().value('NL').build();

I would prefer one of these;

public static CookieBuilder createLocale() {
    String name = Application.getInstance(Config.class).getLocaleCookieName();
    return new CookieBuilder(name);
}
//CookieBuilder.createLocale().value('NL').build();

public static CookieBuilder createLocale(Locale locale) {
    String name = Application.getInstance(Config.class).getLocaleCookieName();
    return new CookieBuilder(name).value(locale.getISO3Language);
}
//CookieBuilder.createLocale(Locale.CHINA).build();

[MarkVink]

We do not need this import since Locale isn't used in CookieHandler (anymore).

[svenkubiak]

I wanted to create an integration test the LocaleHandler, but that's
an awful lot of data to build up first. In reality I think if there is little
need anyway.

By using the test utilities you could easily create a cookie and add it to the request and checking if the response has the correct language. I have added a method for adding a custom cookie in the lastest master push.

[MarkVink]

@svenkubiak What are your thoughts on setting the cookie domain by default with the application host value?

[svenkubiak]

@MarkVink, @WilliamDunne
IMHO I wouldn't set a default value. Mainly because there is already a default machanism for that

If a cookie's domain and path are not specified by the server, they default to the domain and path of      the resource that was requested

See: https://en.wikipedia.org/wiki/HTTP_cookie#Domain_and_Path

Besides that, running a mangoo I/O application in the recommended way (behind a reverse-proxy) will result in errors if domains are not set correctly in prod/test and dev mode.

[svenkubiak]

I have moved this to 2.4.0 for now as I want to upgrade to undertow 1.3.15.Final (https://issues.jboss.org/projects/UNDERTOW/versions/12329419) as soon as it is released as it fixes some majob bugs. If we got this PR working in time, I will add it to 2.3.0 of course.

[ghost]

Working? The only thing wrong is the default (didn't realize about the cookie domain, but it make sense).

[svenkubiak]

"Working" like "ready to merge". Build passing etc ;-)

[ghost]

@MarkVink still used for creating locale cookie. I'll look at the build.

[MarkVink]

@WilliamDunne I mean, its not necessary to instantiate it as a field in the CookieBuilder, since its only used one time, and only in the case the developer wants to set a language cookie. Move it to the createLocale method.

[ghost]

Pull-able?

[svenkubiak]

would be application.i18n.cookie.name, right?

[ghost]

Done

[svenkubiak]

Pull-able?

I made some comments on the files. Also we still need that unit test for the locale cookie. If you need any help on that, just let me know.

[ghost]

I could use some help with the unit test, not something I've done a large amount of

[svenkubiak]

I could use some help with the unit test, not something I've done a large amount of

No Problem. I look into it asap.

[svenkubiak]

I'll merge this and make some minor changes and add the unit test in the master.

[svenkubiak]

I have made some minor refactorings and added a unit test. Nothing fancy. However, I did remove the getLocaleCookie method from the CookieBuilder. I don't think it is a good idea, to have a builder within a builder just to have a short way of

Cookie cookie = CookieBuilder.create().name(Default.COOKIE_I18N_NAME.toString())

We could introduce a CookieUtils class, but IMHO this fits most needs for now.