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
Add more date segments: visitEndServerDate visitEndServerDayOfMonth visitEndServerDayOfWeek visitEndServerDayOfYear, etc. #12521
Conversation
use Piwik\Columns\MetricsList; | ||
use Piwik\Plugin\Dimension\VisitDimension; | ||
use Piwik\Metrics\Formatter; | ||
use function Piwik\Plugins\VisitTime\translateDayOfWeek; |
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.
use function
is available in PHP 5.6+ only, but we still support 5.5.9+.
See https://secure.php.net/manual/en/language.namespaces.importing.php
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.
Thanks, fixed! Didn't notice it. Will have an eye on the tests once they are finished.
The only failing test now should be in CustomDimensions plugin a system test that we could fix when merged or so? |
public function __construct() | ||
{ | ||
$this->suggestedValuesCallback = function ($idSite, $maxValuesToReturn) { | ||
return range(2016, min(2036, $maxValuesToReturn)); |
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.
I'm not sure if that's what it's meant to be. If $maxValuesToReturn
is 30
like in the tests, it returns a range from 2016 down to 30. I guess it would be better to use something like return range(2016, 2016 + $maxValuesToReturn);
?
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.
Absolutely right, not sure what I was thinking... pushing a fix now.
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.
Jus tested some of the new segments. Found one last thing. Besides that it should be good to merge.
protected $columnName = 'visit_last_action_time'; | ||
protected $type = self::TYPE_DATETIME; | ||
protected $segmentName = 'visitEndServerDayOfWeek'; | ||
protected $nameSingular = 'VisitTime_ColumnVisitEndServerWeek'; |
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.
Seems that translation key does not exists. Guess it needs to be VisitTime_ColumnVisitEndServerDayOfWeek
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.
Cheers... great find. Fixed it. Will have an eye on tests.
FYI: Seems like the tests for |
Adds the following new date segments (and dimensions basically):
Tests will need to be fixed afterwards.