? Failure
Referenced as Related to: # 4505

User tests: Successful: Unsuccessful:

avatar schnuti
schnuti
10 Oct 2014

Steps to reproduce the issue

For a calendar form field you set the default value to "NOW" and add a new item.
To test you can edit any form with a calendar field.

Expected result

  1. The actual user date and time shown or
  2. The actual server date and time shown depending on the filter used

Actual result

The date and time is wrongly calculated with the actual server date and time as basis.
Must use UTC+0 as basis.

System information (as much as possible)

J 3.3.6

Additional comments

PR on GitHub
calendar.php
// Handle the special case for "now".
if (strtoupper($this->value) == 'NOW')
{
//$this->value = strftime($format);
$this->value = JFactory::getDate()->toSql();
}

avatar schnuti schnuti - open - 10 Oct 2014
avatar jissues-bot jissues-bot - change - 10 Oct 2014
Labels Added: ?
avatar smanzi
smanzi - comment - 11 Oct 2014

@Test
Success.
It seems Travis is failing for its own reasons...

avatar mbabker
mbabker - comment - 11 Oct 2014

Actually, the Travis failures (minus the PostgreSQL related stuff on the PHP 5.3 build) are because of the test assertions. The tests are asserting that a date in the format Y-m-d is being returned, but with your patch the condition is returning a date in the format Y-m-d H:i:s. Whether that means there's an unintended side effect with this patch or there's an issue with the test method needs to be investigated.

avatar smanzi
smanzi - comment - 11 Oct 2014

Oops...I focused on the warning Travis is issuing for composer being older than 30 days and didn't scroll down: shame on me!

What are the 3 other errors issued by Travis for the JDatabaseDriverPostgresqlTest class? It seems to be a case of misalignment... I should learn reading before writing...

avatar schnuti
schnuti - comment - 13 Oct 2014

I've little knowledge about the tests but obviously the test expects yy-mm-dd from a method handling time zones. i.e hh:mm:ss is of course needed. Well at least hh:mm.

Message
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'2014-10-10'
+'2014-10-10 21:50:06'

avatar brianteeman brianteeman - change - 17 Oct 2014
The description was changed
avatar brianteeman brianteeman - change - 17 Oct 2014
Category Libraries
avatar jissues-bot jissues-bot - change - 18 Oct 2014
Title
[#4505] - Wrong datetime NOW for JForm field calendar
Wrong datetime NOW for JForm field calendar
avatar brianteeman brianteeman - change - 18 Oct 2014
Title
[#4505] - Wrong datetime NOW for JForm field calendar
Wrong datetime NOW for JForm field calendar
avatar mbabker mbabker - reference | 353603a - 18 Oct 14
avatar mbabker
mbabker - comment - 18 Oct 2014

So looking at this some and trying to make sure I understand what's expected, I started looking at the line you changed and what the test expectations are, and I see the issue. In the case of 'NOW', the time string was being formatted with the custom format as set into the field class instance. With your patch, the string is always returned in a specific format (that specified by the database driver). The custom format in that line can only properly work if you don't set one of the filters supported in the class later on as they reformat the string to Y-m-d H:i:s without regard to the custom format, but as it is something supported by the current API, removing the ability to custom format the string would be a B/C break.

But not to fear, that doesn't make this patch a show stopper. I changed the toSql call in that case to call format instead and specified the custom format in that line. See mbabker@353603a for the diff (includes fixes to the format strings in the unit test case which were causing other failures). With it, the unit tests do pass (see https://travis-ci.org/mbabker/joomla-cms/builds/38375426). From what I can tell, this works correctly in any of the edit views too.

avatar smanzi
smanzi - comment - 18 Oct 2014

Michael, I'm not sure at all that the custom format should be applied at that point: isn't it handled by the last "return JHtml::_('calendar', $this->value, $this->name, $this->id, $format, $attributes);" ?

Have you tested if the correct "now time" is returned, also in different timezones?

avatar mbabker mbabker - reference | 23ec2d7 - 18 Oct 14
avatar mbabker
mbabker - comment - 18 Oct 2014

Didn't go through all test cases, no. But, JDate::toSql() is in essence a proxy to JDate::format() and injects a format based on the database driver, so thorough testing at that level shouldn't be necessary; it's the same call you changed to just a different date format.

Which leads to an interesting issue in that the unit tests were indeed rather inconsistent with the expected method calls, even after hardcoding the format as is done later in the class. So after fixing that up, I've gotten to mbabker@23ec2d7 which should take care of things. In terms of production code, the only difference between my patch and your's is that mine uses a consistently hardcoded date format while your's could change based on the database driver (none of the ones technically supported by the CMS use a different format, so not a real difference, but for the sake of sanity let's just be consistent in the method one way or the other).

avatar smanzi
smanzi - comment - 18 Oct 2014

Got it! Thanks!

avatar schnuti
schnuti - comment - 19 Oct 2014

@Test
Tested mbabker@23ec2d7 with success.
Do I have to confirm somewhere else?

avatar schnuti
schnuti - comment - 19 Oct 2014

A general comment on the calendar field type:

I think there are some problems in mixing time zone handling with free date formatting. If hh:mm is missing you can get a date-2 older than date-1. At least you have to use some kind of filter to avoid the default user-utc.
It's hard to test as you need UTC time in one day and user/server time in previous/next day. So I'll keep this as a theory.

avatar Hackwar
Hackwar - comment - 3 Feb 2015

Please make sure that the unit test errors are solved. Maybe the tests are wrong, but please first expect your code to break backwards compatibility and fix that before changing the tests.

avatar roland-d
roland-d - comment - 20 Aug 2015

Hello @schnuti

Thank you for your contribution.

The last comment here was on 3rd February. Can you please update the PR as requested by Hackwar?

If no reply is received within 4 weeks we will close this issue.

Thanks for understanding!


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4508.

avatar roland-d roland-d - change - 20 Aug 2015
Status Pending Information Required
avatar schnuti
schnuti - comment - 23 Aug 2015

See mbabkers proposed changes above including fixes to the unit tests.
The bug should be fixed with mbabkers proposal. Sorry, I can't see any BW compatibility problems with it.

avatar schnuti schnuti - change - 23 Aug 2015
Status Information Required Closed
Closed_Date 0000-00-00 00:00:00 2015-08-23 08:21:26
Closed_By schnuti
avatar schnuti schnuti - change - 23 Aug 2015
Title
Wrong datetime NOW for JForm field calendar
[#4505] Wrong datetime NOW for JForm field calendar
avatar schnuti schnuti - close - 23 Aug 2015
avatar schnuti schnuti - close - 23 Aug 2015
avatar joomla-cms-bot joomla-cms-bot - change - 23 Aug 2015
Title
Wrong datetime NOW for JForm field calendar
[#4505] Wrong datetime NOW for JForm field calendar
avatar zero-24
zero-24 - comment - 23 Aug 2015

Here is the fix by @mbabker #7753

Add a Comment

Login with GitHub to post a comment