CRM-21148 - Refactor two near-identical functions into one #10941
Conversation
CRM/Utils/Date.php
Outdated
| @@ -800,17 +800,20 @@ public static function getRange($startDate, $endDate) { | |||
| * @return array | |||
| * start date, end date | |||
| */ | |||
| public static function getFromTo($relative, $from, $to) { | |||
| public static function getFromTo($relative, $from, $to, $fromTime = NULL, $toTime = NULL) { | |||
| if (empty($toTime)) { | |||
There was a problem hiding this comment.
Would this be cleaner in the function signature - ie
public static function getFromTo($relative, $from, $to, $fromTime = 0, $toTime = '235959') {
| $dateRange = self::relativeToAbsolute($term, $unit); | ||
| $from = $dateRange['from']; | ||
| $from = substr($dateRange['from'], 0, 8); |
There was a problem hiding this comment.
I wonder if there could be any existing cases where it is working 'because' the string is passed through uncut here - looks like it would work :-(
There was a problem hiding this comment.
I did some checks & it seems existing usages of this function are mostly in the advanced search & they don't appear to offer time granularity so it's probably fine / safe
There was a problem hiding this comment.
I did the same check - I actually considered removing the $fromTime and $toTime from the function signature since this function never returns time-level granularity. However, since my goal is to insert a hook here, I kept them in, in case someone wants to write an extension that provides it.
There was a problem hiding this comment.
OK - well I guess we both checked & didn't find something it's probably ok
There was a problem hiding this comment.
I like the fact this leaves a path to adding 'last one hour' as a filter too
|
@MegaphoneJon can you squash (rebase -i) & check the fails |
MegaphoneJon
force-pushed
the
relativedates
branch
from
9fba69d to
154a7cd
Compare
|
Well, I failed my own test in the same PR; I don't know whether to be proud or ashamed. Anyway, @eileenmcnaughton - turns out I couldn't drop this: if (empty($toTime)) {
$toTime = '235959';
}There's the edge case of someone passing |
|
Hmm - I personally disagree with handling invalid developer-written bugs. People should actually have to write valid code for it to work. Could you make passing in NULL more obviously invalid by going with ($relative, $from, $to, $fromTime = NULL, (int) $toTime = 23599) |
More unit tests properly handle times CRM-21148 - incorporate feedback style fix style fix Handle passed in as NULL CRM-21148 - Refactor two functions into one
MegaphoneJon
force-pushed
the
relativedates
branch
from
154a7cd to
7aab2a8
Compare
|
jenkins is not your friend |
|
@eileenmcnaughton I know it's been a minute since you looked at this, but since this my changes are in response to your review, could you please take a look? |
| @@ -1996,14 +1996,14 @@ public function whereClause(&$field, $op, $value, $min, $max) { | |||
| * @param string $from | |||
| * @param string $to | |||
| * @param string $type | |||
| * @param string $fromTime | |||
| * @param string $toTime | |||
| * @param int $fromTime | |||
There was a problem hiding this comment.
I'm concerned of the impact of changing fromTime/toTime to an int. What if the desired fromTime is 000100 (12:01 am) or any other value in the first hour of the day? I suspect that value will be interpreted as 100 and impact the datetime construction downstream.
|
I suspect this is fine: http://php.net/manual/en/datetime.formats.compound.php#119223 However, it's worth checking. I'm adding it to my to-do list. That said, this is a pet project. @lcdservices - if you have a paid client interested in this, you have incentive to do it first :) |
|
@MegaphoneJon I've opened #11887 as a cut down version of this - can you check it with a view to merging? |
|
Last week was tough but I should be able to do this later this week. |
|
@MegaphoneJon any chance to look at the proposed replacement? |
|
Closing this since we remerged the other - you can decide if there is more in here you want to open a new PR for |
Overview
At some point, a method was copy-pasted, then modified. This reconciles the differences and reduces code duplication.
Before
CRM_Utils_Date::getFromTo()andCRM_Report_Form::getFromTo()contain almost identical code.After
CRM_Report_Form::getFromTo()wrapsCRM_Utils_Date::getFromTo().Comments
My unit tests bypass the normal
setUp()andtearDown()methods, which cuts the time to run down considerably. These are unit tests and not functional tests; there's no need to load the database. Is this a kosher technique? My thinking is that if someone adds functional tests later, it'll be obvious that this needs changing - but that a Utils subclass probably never needs functional tests.