dev/core#2325 import second-handling fix #23754
Conversation
|
(Standard links)
|
CRM/Utils/Date.php
Outdated
| if (empty($date)) { | ||
| return $formattedDate; | ||
| return FALSE; |
There was a problem hiding this comment.
Policy-wise I don't think you should be able to just change the signature of a longstanding public static function. And in fact null makes more sense to me as a return value here. Why false? Also see line 2144.
There was a problem hiding this comment.
hmm - ok - it is only used by import - but I'm not strongly pro 'false' - it just always seems like php functions generally do that when invalid
There was a problem hiding this comment.
Ok I thought I saw it elsewhere in universe but looks like really just civihr and I'm not sure what its status is. The use in sepa60 wouldn't make a difference. But still line 2144 here then.
There was a problem hiding this comment.
I updated back to NULL anyway
aa65dcd to
d8db2c1
Compare
CRM/Utils/Date.php
Outdated
| @@ -2122,32 +2131,28 @@ public static function formatDate($date, $dateType) { | |||
|
|
|||
| if (CRM_Utils_Date::convertToDefaultDate($dateParams, $dateType, $dateKey)) { | |||
| $dateVal = $dateParams[$dateKey]; | |||
| $ruleName = 'date'; | |||
| if ($dateType == 1) { | |||
| if ($dateType === 1) { | |||
There was a problem hiding this comment.
This breaks it because $dateType is string '1'.
There was a problem hiding this comment.
argh - I got ahead of myself
There was a problem hiding this comment.
ok - now passing an int but not requiring one
d8db2c1 to
a9d6676
Compare
|
This looks good. There's an issue with the error output but it seems to happen without the PR - I don't know if it's related to https://lab.civicrm.org/dev/core/-/issues/2566 which was reported fixed (i.e. something like the reason now shows but now the column is missing). Will make a lab ticket, e.g. if I have a date like |
|
thanks - I'll check gitlab for the new issue |
Overview
dev/core#2325 import second-handling fix
Before
Seconds dropped when importing dates
After
Seconds retained
Technical Details
This patch only works for
if ($dateType === 1)- which has it's own if - I haven't looked at other typeshttps://lab.civicrm.org/dev/core/-/issues/2325
Comments
@demeritcowboy @monishdeb this improves the earlier fix - and I think can close the issue even thought I kept the scope narrow. The regex is still a bit wonky per @demeritcowboy's comments - but I at least code commented to that effect