User tests: Successful: Unsuccessful:
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.
The date and time is wrongly calculated with the actual server date and time as basis.
Must use UTC+0 as basis.
J 3.3.6
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();
}
Labels |
Added:
?
|
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.
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...
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'
Category | ⇒ | Libraries |
Title |
|
Title |
|
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.
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?
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).
Got it! Thanks!
@Test
Tested mbabker@23ec2d7 with success.
Do I have to confirm somewhere else?
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.
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.
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!
Status | Pending | ⇒ | Information Required |
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.
Status | Information Required | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2015-08-23 08:21:26 |
Closed_By | ⇒ | schnuti |
Title |
|
Title |
|
@Test
Success.
It seems Travis is failing for its own reasons...