Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

JDate breaks valid DateTime input #705

Closed
wants to merge 2 commits into from

4 participants

@bembelimen

JDate assumes that every number is an unix timestamp, but according to the PHP documentation (see http://www.php.net/manual/en/datetime.formats.date.php) it is possible to use an 8 digit number as valid parameter which is not a timestamp (see ISO8601 Notations). This number consists the year, the month and the day without any separators (e.g. 20120101 for 2012/01/01). It's also valid to use only the year (four digits) as parameter.

So the first step is, that Joomla! should only handle numbers with more than 8 digits as unix timestamp.
The second step should be the removing of the whole test, because DateTime can handle unix timestamps by itself, if the programmer prepend an "@" char to the number (see http://www.php.net/manual/en/datetime.formats.compound.php "Localized Notations"). But this needs further testing, if there are any conflicts in the Joomla! environment. So this requests fixs only the "8 digit problem" and mark the test itself as deprecated.

bembelimen added some commits
@bembelimen bembelimen Joomla! assumes that every number is an unix timestamp, but according…
… to the PHP documentation (see http://www.php.net/manual/en/datetime.formats.date.php) it is possible to use an 8 digit number as valid parameter which is not a timestamp (see ISO8601 Notations). This number consists the year, the month and the day without any separators (e.g. 20120101 for 2012/01/01). It's also valid to use only the year (four digits) as parameter.

So the first step is, that Joomla! should only handle numbers with more than 8 digits as unix timestamp.
The second step should be the removing of the whole test, because DateTime can handle unix timestamps by itself, if the programmer prepend an "@" char to the number (see http://www.php.net/manual/en/datetime.formats.compound.php "Localized Notations"). But this needs further testing, if there are any conflicts in the Joomla! environment. So this requests fixs only the "8 digit problem" and mark the test itself as deprecated.
16d71c2
@bembelimen bembelimen Fixed style error 58dace4
@joomla-jenkins
Collaborator

Unit testing complete. There were 0 failures and 0 errors from 1971 tests and 11146 assertions.
Checkstyle analysis reported 199 warnings and 0 errors.

@pasamio

Wouldn't this break backwards compatibility for anyone with a date with 8 or less digits in it?

@bembelimen

Yep, some small numbers will be broken. So do you think, it would be better to mark the check as "deprecated" and don't change the function at all. And then remove the check complete in the following Joomla! version?

@LouisLandry

This has been sitting for over 2 months without update and is not currently mergeable. I'm going to close it for now. If you want to continue the discussion please rebase the branch against current master and re-open the pull request. Cheers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 4, 2012
  1. @bembelimen

    Joomla! assumes that every number is an unix timestamp, but according…

    bembelimen authored
    … to the PHP documentation (see http://www.php.net/manual/en/datetime.formats.date.php) it is possible to use an 8 digit number as valid parameter which is not a timestamp (see ISO8601 Notations). This number consists the year, the month and the day without any separators (e.g. 20120101 for 2012/01/01). It's also valid to use only the year (four digits) as parameter.
    
    So the first step is, that Joomla! should only handle numbers with more than 8 digits as unix timestamp.
    The second step should be the removing of the whole test, because DateTime can handle unix timestamps by itself, if the programmer prepend an "@" char to the number (see http://www.php.net/manual/en/datetime.formats.compound.php "Localized Notations"). But this needs further testing, if there are any conflicts in the Joomla! environment. So this requests fixs only the "8 digit problem" and mark the test itself as deprecated.
  2. @bembelimen

    Fixed style error

    bembelimen authored
This page is out of date. Refresh to see the latest.
Showing with 9 additions and 2 deletions.
  1. +9 −2 libraries/joomla/utilities/date.php
View
11 libraries/joomla/utilities/date.php
@@ -113,9 +113,16 @@ public function __construct($date = 'now', $tz = null)
}
}
- // If the date is numeric assume a unix timestamp and convert it.
+ /**
+ * @deprecated
+ * This test is obsolete, because DateTime supports timestamps
+ * if there is a prepended "@" char, so everyone should handle
+ * this issue while calling JDate and not JDate itself
+ */
+ // If the date is numeric and have more than 8 digits
+ // assume a unix timestamp and prepend a '@' char.
date_default_timezone_set('UTC');
- $date = is_numeric($date) ? date('c', $date) : $date;
+ $date = is_numeric($date) && strlen($date) > 8 ? '@' . $date : $date;
// Call the DateTime constructor.
parent::__construct($date, $tz);
Something went wrong with that request. Please try again.