In some scripts we are doing unnecessary conversion from string to DT and vice-versa, sometimes several times. For instance, see bug 29711. We have epoch => DT => string => sending to template in 'iso' YYYY-MM-DD HH:MM => DT => string. This is because the stringified version of DT (the real ISO 8601 YYYY-MM-DDTHH:MM:SS) is not supported by our dt_from_string method. There are (at least) two options: * Use DateTime::Format::Strptime In dt_from_string: my $formatter = DateTime::Format::Strptime->new(pattern => '%Y-%m-%d %H:%M:%S'); return DateTime->now( time_zone => $tz, formatter => $formatter, ) unless $date_string; Then later $dt_params{formatter} = $formatter; But that's an overhead for all dt_from_string calls. * add "T?" to $time_re, easy and efficient.
Created attachment 128631 [details] [review] Bug 29718: Support ISO8601
Note that this patch fixes bug 29711. We would also remove all the extra output_pref calls from pl scripts.
Martin, Tomas, what do you think? I knew we wanted to be stricter, but here I have the feeling that this patch is correct.
Right.. I agree with your patch.. mostly. In the RFC3339 regex we actually do '([Tt\s])'.. Whilst I think ISO was slightly more strict.. i.e 'T' now and '\s' before (So I agree to allowing for both).. should we also support lower case 't' ?
Also.. being slightly picky.. I'd update the comment lines that explain the regex's.. # iso or sql format are yyyy-dd-mm[ hh:mm:ss]" and # Add the faculative time part [hh:mm[:ss]]
Also, interestingly.. I note that for ISO I don't believe 'am/pm' is actually allowed.. all times should be 24 hour. That's an oversight I hadn't spotted before..
Also.. to properly adhere to ISO we should also optionally support timezone (with it defaulting to "local" when one isn't passed).
Created attachment 128653 [details] [review] Bug 29718: Support ISO8601 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 128654 [details] [review] Bug 29718: (QA follow-up) Make 'T' check case insensative Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 128658 [details] [review] Bug 29718: (QA follow-up) Add optional offset handling ISO8601 optionally supports timezone offsets. If a timezone is ommited, we should assume 'local' time (Which in our case is instance configuration time), if we are passed 'Z' or 'z' we should set timezone to 'UTC', otherwise we should set the timezone to the passed offset. Whilst ISO8601 does NOT support AM/PM, I left that intact as it has historical relevance in Koha and we share the ISO parsing with SQL type parsing amongth other things.
Clearly, we still need to add some additional unit tests to prove the new support. Whilst testing I took the following set of strings to test the regex's against: 2020-12-09T16:09:53+00:00 2020-12-09 16:09:53+00:00 2020-12-09T 16:09:53+00:00 2020-12-09t16:09:53+00:00 2020-12-0916:09:53+00:00 2020-12-09 16:09:53+00:00 2020-12-09 16:09:53Z 2020-12-09 04:09:53 pm 2020-12-09 04:09:53 am 2020-12-09 04:09:53 2020-12-09 04:09:53am+00:00 # Fails as it should 2020-12-09 04:09:53+00:00am # Fails as it should
Created attachment 128840 [details] [review] Bug 29718: Unit tests
Tests are failing for me t/DateUtils.t .. 20/79 # Failed test 'dt_from_string with no offset assumes "local"' # at t/DateUtils.t line 160. # got: '1325462399' # expected: '1325455199' # Failed test 'dt_from_string with UTC prescribed as Z' # at t/DateUtils.t line 164. # got: '1325462399' # expected: '1325455199' # Looks like you failed 2 tests of 6.
Interesting.. on k-t-d I can't replicate your failures..?
humm.. my local branch misses the unit tests.. wonder why.. I can replicate now I've grabbed the patches here.
(In reply to Martin Renvoize from comment #15) > humm.. my local branch misses the unit tests.. wonder why.. > > I can replicate now I've grabbed the patches here. I am still getting the same failure, patches from bug 29403 and bug 29718 applied.
Shouldn't we mock the timezone here?
Created attachment 129372 [details] [review] Bug 29718: Mock the timezone
You are indeed right, we needed to mock the timezone.. what I was still trying to wrap my head around was a) what timezone we should pick and b) if it should affect all the existing tests in any way and what the actual times should end up as for the tests.. Looks like you've given that some thought :).. I'll take a look and confirm now.
Created attachment 129385 [details] [review] Bug 29718: Support ISO8601 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 129386 [details] [review] Bug 29718: (QA follow-up) Make 'T' check case insensative Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 129387 [details] [review] Bug 29718: (QA follow-up) Add optional offset handling ISO8601 optionally supports timezone offsets. If a timezone is ommited, we should assume 'local' time (Which in our case is instance configuration time), if we are passed 'Z' or 'z' we should set timezone to 'UTC', otherwise we should set the timezone to the passed offset. Whilst ISO8601 does NOT support AM/PM, I left that intact as it has historical relevance in Koha and we share the ISO parsing with SQL type parsing amongth other things. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 129388 [details] [review] Bug 29718: Unit tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 129389 [details] [review] Bug 29718: Mock the timezone Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 129390 [details] [review] Bug 29718: (QA follow-up) Remove duplicate test I'd inadvertantly introduced the exact same test twice! This patch removes the duplicate and annotates the test to clarify the timezone handling expectations. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
I've tested Jonathans follow-up and added another of my own to annotate the tests and remove a duplicate. Setting to Signed off to get QA attention as I think we're ready for a last technical eye here now.
QA: Looking here
(In reply to Martin Renvoize from comment #5) > Also.. being slightly picky.. I'd update the comment lines that explain the > regex's.. > > # iso or sql format are yyyy-dd-mm[ hh:mm:ss]" > > and > > # Add the faculative time part [hh:mm[:ss]] Even more picky. What about spelling? Facultative?
(?<utc>[Zz]$)|((?<offset>[\+|\-])(?<hours>[01][0-9]|2[0-3]):(?<minutes>[0-5][0-9])) What slightly confuses is that we use the names hours and minutes here while we also use hour and minute. It is not completely clear that the hours and minutes belong to the tz offset. Better name?
The UTC offset is appended to the time in the same way that 'Z' was above, in the form ±[hh]:[mm], ±[hh][mm], or ±[hh]. So we should be able to specify +02, +0200 as well.
Created attachment 129465 [details] [review] Bug 29718: Support ISO8601 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 129466 [details] [review] Bug 29718: (QA follow-up) Make 'T' check case insensative Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 129467 [details] [review] Bug 29718: (QA follow-up) Add optional offset handling ISO8601 optionally supports timezone offsets. If a timezone is ommited, we should assume 'local' time (Which in our case is instance configuration time), if we are passed 'Z' or 'z' we should set timezone to 'UTC', otherwise we should set the timezone to the passed offset. Whilst ISO8601 does NOT support AM/PM, I left that intact as it has historical relevance in Koha and we share the ISO parsing with SQL type parsing amongth other things. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 129468 [details] [review] Bug 29718: Unit tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 129469 [details] [review] Bug 29718: Mock the timezone Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 129470 [details] [review] Bug 29718: (QA follow-up) Remove duplicate test I'd inadvertantly introduced the exact same test twice! This patch removes the duplicate and annotates the test to clarify the timezone handling expectations. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 129471 [details] [review] Bug 29718: (QA follow-up) ISO 8601 allows +02 and +0200 The UTC offset is appended to the time in the form: ±[hh]:[mm], ±[hh][mm], or ±[hh] Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Nice call regards +02 vs +02:00 and +0200.. I hadn't spotted that.. and.. it turns out I've been spelling facultative wrong my entire life.. lol.
(In reply to Martin Renvoize from comment #38) > it turns out I've been spelling facultative wrong my entire life.. lol. I had the same realization a few months ago haha. I never say it in English (I just say optional) but in French I would say/spell it as faculative too haha. Do people use the word "facultative" much in the UK?
Pushed to master for 22.05, thanks to everybody involved [U+1F984]
This causes a failure on Ubuntu 18.04: bug 30030
To have the CI still be meaningful for Ubuntu 18.04 (until 2023-04-26) for Koha 22.05 and in the future 22.11, should these patch be reverted until then? (assuming it's possible to revert)
(In reply to Victor Grousset/tuxayo from comment #42) > To have the CI still be meaningful for Ubuntu 18.04 (until 2023-04-26) for > Koha 22.05 and in the future 22.11, should these patch be reverted until > then? (assuming it's possible to revert) We need to backport a newer DateTime library for 18.04.
(In reply to Tomás Cohen Arazi from comment #43) > (In reply to Victor Grousset/tuxayo from comment #42) > > To have the CI still be meaningful for Ubuntu 18.04 (until 2023-04-26) for > > Koha 22.05 and in the future 22.11, should these patch be reverted until > > then? (assuming it's possible to revert) > > We need to backport a newer DateTime library for 18.04. i have had some success with updating just the 'libdatetime-timezone-perl' pkg, for u18 my theory is that a newer olsen database is needed for u18 - its been upgraded from 2018d to 2022a (ps: i cant explain why u20 works with only a 2019c database??) http://ftp.iana.org/tz/releases/ i did the update some days ago, which fixed the broken tests for master and stable https://jenkins.koha-community.org/view/master/job/Koha_Master_U18/1510/ https://jenkins.koha-community.org/view/22.05/job/Koha_22.05_U18/44/ i'll aim to update the prod repo soon
Congratulation Mason :D Even if there is still mystery we will be able to close bug 30030 when it hit the prod repo, right?
> my theory is that a newer olsen database is needed for u18 - its been > upgraded from 2018d to 2022a > (ps: i cant explain why u20 works with only a 2019c database??) > aah, the 2019b database has fixes for Brazil http://changelogs.ubuntu.com/changelogs/pool/universe/libd/libdatetime-timezone-perl/libdatetime-timezone-perl_2.38-1+2019c/changelog ------------------------------------------------------ libdatetime-timezone-perl (1:2.36-1+2019b) unstable; urgency=medium * Import upstream version 2.36. This release is based on version 2019b of the Olson database. It includes contemporary changes for Brazil and Palestine. <<< ------------------------------------------------------ updating u18 to libdatetime-timezone-perl_2.23-1+2022a_all.deb fixes the following errors 'Invalid local time for date in time zone: America/Campo_Grande' 'Invalid local time for date in time zone: America/Cuiaba' 'Invalid local time for date in time zone: America/Sao_Paulo'
reopened, until confirmed fixed
(In reply to Victor Grousset/tuxayo from comment #45) > Congratulation Mason :D > > Even if there is still mystery we will be able to close bug 30030 when it > hit the prod repo, right? yes, correct :)