bkiers / Liqp

An ANTLR based 'Liquid Template' parser and rendering engine.
MIT License
165 stars 94 forks source link

Date Filter Does Not Work With 'T' Formatted Dates #171

Closed micah-williamson closed 3 years ago

micah-williamson commented 4 years ago

According to this blog post on shopify-

https://www.shopify.com/partners/blog/liquid-date-format

A datetime should be valid if the date-time separator is either a space or 'T'. However, this doesn't appear to work with Liqp.

Map<String, Object> values = new HashMap<>();
values.put("date_with_t", "2020-01-01T12:30:00");
values.put("date_with_space", "2020-01-01 12:30:00");

Template t = Template.parse("Space: {{ date_with_space | date: '%Y' }} |  T: {{ date_with_t | date: '%Y' }}");

String result = t.render(values);

This results in-

Space: 2020 | T: 2020-01-01T12:30:00

But it should result in-

Space: 2020 | T: 2020

PChou commented 4 years ago

this PR https://github.com/bkiers/Liqp/pull/173 should solve that

msangel commented 4 years ago

@PChou This looks nice but what looks danger to me is new joda-time dependencies. Will it be hard do not include that as dependency? Because if we will include that - this is not compatible change.

msangel commented 3 years ago

Currently working on this. Thanks for the test cases, I will use them. And just for the history: there exists a way to put 'T' letter into pattern https://stackoverflow.com/questions/2416809/using-alphabetic-characters-in-simpledateformat-pattern-string

PChou commented 3 years ago

@msangel what about shade joda-time if you are afraid that? i think following standard jruby implementation should avoid bugs that not known yet WDYT?

msangel commented 3 years ago

@PChou , there no need in that. Pre-java8 jdk date API is broken, so the library will move to the jdk8 and will use normal datetime api as a base(old types still will be supported via internal plugin system, btw that can be tuned for joda-time aswell). I made a big effort to make this working with pre-java8 datetypes, but once I realise how it is broken(I had one test that works fine in jdk7 but became broken in jdk8), I made a big decision to throw that away, and use normal one. Having tradeoff between jdk7 compatible and normal datetime handling with support of new types the second was chosen. The feature is almost done, there are few more tasks and I believe in nearest future we will have a release with that.

msangel commented 3 years ago

You can track the progress there: https://github.com/bkiers/Liqp/pull/207/commits

msangel commented 3 years ago

@PChou Now it is in the master branch.

Is that fix works?