New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Date factory test when date object is passed #15998
Conversation
@@ -60,7 +60,7 @@ public function trackVisits() | |||
$this->trackEventWithoutUrl($vis); | |||
$this->trackMovieWatchingIncludingInterval($vis); | |||
|
|||
$this->dateTime = Date::factory($this->dateTime)->addHour(0.5); | |||
$this->dateTime = Date::factory($this->dateTime)->addHour(0.5)->getDatetime(); | |||
$vis2 = self::getTracker($this->idSite, $this->dateTime, $useDefault = true, $uselocal); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we used to pass a Date
object here meaning the tracker would do $date->toString()
which then would only result in YYYY-MM-DD
and throw away the entire time. So this was before actually not working which is why some tests are failing.
@@ -47,7 +47,7 @@ public function trackVisits() | |||
|
|||
$this->trackContentImpressionsAndInteractions($vis); | |||
|
|||
$this->dateTime = Date::factory($this->dateTime)->addHour(0.5); | |||
$this->dateTime = Date::factory($this->dateTime)->addHour(0.5)->getDatetime(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we used to pass a Date
object here meaning the tracker would do $date->toString()
which then would only result in YYYY-MM-DD
and throw away the entire time. So this was before actually not working which is why some tests are failing.
Any thoughts @sgiehl @diosmosis ? |
In general I would vote for fixing the tests and merging this change. That is actually the behavior I imho would have expected before 🙈 |
👍 fixed the tests @sgiehl |
As suggested in chat made this change to see if anything breaks.
There are some failing tests in CustomEvents and Content tracking but that's because of how the date was falsely used in the test fixture. Happy to update the tests and add a changelog entry if we are keen to merge this. Not sure what else it would maybe break that maybe isn't covered by tests.