ZonedDateTime.format() should fallback to it's own zone when not specified
gwillz opened this issue · 8 comments
Calling zoned_date_object.format(fmt_string) without the second argument for zone_id will use the system's timezone - which really defeats the purpose of a ZonedDateTime.
In it's current form, to correctly format the zoned date time the ZoneId must also be passed as a string into the template - zoned_date_object.format(fmt_string, zoned_id_string).
If the user wanted to format the date object in the system timezone, they should use LocalDateTime.
I could probably submit a PR, but I first wanted to see if this line of thinking was valid.
@gwillz Hmm. I'm not sure if I fully understand the problem here. You are arguing to remove the support for ${myzoneddatetime.format('yyyy-MM-dd Z')} ?
Haha sorry, no that's not quite it. It's a bit confusing, I didn't fully grasp it until I was running the code on a server in UTC time.
So if I were to write ${zoneddatetime.format('HH:mm Z')} and the zoned date time was "10pm EST", I would expect the result to be 22:00 EST.
Instead, I get 02:00 UTC - the zone automatically being converted to whatever the system's timezone is, in this case UTC.
If I want to format it correctly, I must write ${zoneddatetime.format('hh:mm a Z', zoneidstring)}. The zoneidstring being zoneddatetime.getZone().toString().
But i want to use the first form, without manually specifying the timezone. Because being a ZonedDateTime I would expect it to format with it's own timezone by default, not the system's.
The culprit is here. The zonedIdLookup() method will fallback to the system timezone if one is not specified in the argument list.
I fully understand what you're saying now :) And I agree that this most likely would have been the most logic behavior from the start. However, for our usecase we always wanted the dates to be converted to the systems timezone. Changing this now would probably break logic in our (Amedia) systems.
My suggestion is to introduce a flag (or configuration object) in the Java8ObjectWrapper constructor, allowing you to configure the default behavior for this.
FreemarkerJava8Config myConfig = new FreemarkerJava8Config();
// Either
myConfig.setDefaultTimeZoneStrategy(TimeZoneStrategy.KEEP_ORIGINAL):
// Or
myConfig.setDefaultTimeZoneStrategy(TimeZoneStrategy.SYSTEM):
this.configuration.setObjectWrapper(new Java8ObjectWrapper(Configuration.VERSION_2_3_23, myConfig));@gwillz @michaelkrog What do you think ?
Default logic will be as today.
@lazee IMO the correct path would be to respect the Zone by default, but I fully understand that it breaks compatibility.
A configuration object is probably a good idea. In case it grows with more properties, perhaps it could be expanded to a builder pattern later on.
FreemarkerJava8Config myConfig = FreemarkerJava8ConfigBuilder
.defaultTimeZoneStrategy(TimeZoneStrategy.KEEP_ORIGINAL)
.anotherCoolProperty(Some.VALUE)
.build();
this.configuration.setObjectWrapper(new Java8ObjectWrapper(Configuration.VERSION_2_3_23, myConfig));
We can do builder pattern in the first version I think.
I'm not sure when I will have the time to look at this, so if any of you would like to come up with a PR.... :)
Love it!
Regarding configuration; it's worth considering that freemarker likes to use a string syntax, which constructs a configuration object.
Example:
com.example.MyObjectWrapper(1, 2, exposeFields=true, cacheSize=5000)
is nearly equivalent with this Java code:obj = new com.example.MyObjectWrapper(1, 2); obj.setExposeFields(true); obj.setCacheSize(5000);
The string syntax is described here. It's uses the java bean pattern, but then also prefers the builder pattern if it can find one.