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
Disable visitorId segment when feature disabled #17508
Conversation
@flamisz could you maybe test below diff that might be an easier fix for it (dimensions usually create segments automatically) diff --git a/plugins/CoreHome/Columns/VisitorId.php b/plugins/CoreHome/Columns/VisitorId.php
index 7c8fdec80e..fb4456c61e 100644
--- a/plugins/CoreHome/Columns/VisitorId.php
+++ b/plugins/CoreHome/Columns/VisitorId.php
@@ -9,11 +9,14 @@
namespace Piwik\Plugins\CoreHome\Columns;
use Piwik\Columns\DimensionMetricFactory;
+use Piwik\Columns\DimensionSegmentFactory;
use Piwik\Columns\MetricsList;
use Piwik\Metrics\Formatter;
use Piwik\Piwik;
use Piwik\Plugin\ArchivedMetric;
use Piwik\Plugin\Dimension\VisitDimension;
+use Piwik\Plugins\Live\SystemSettings;
+use Piwik\Segment\SegmentsList;
/**
* Dimension for the log_visit.idvisitor column. This column is added in the CREATE TABLE
@@ -31,6 +34,17 @@ class VisitorId extends VisitDimension
protected $sqlFilterValue = array('Piwik\Common', 'convertVisitorIdToBin');
protected $type = self::TYPE_BINARY;
+ public function configureSegments(SegmentsList $segmentsList, DimensionSegmentFactory $dimensionSegmentFactory)
+ {
+ $systemSettings = new SystemSettings();
+ $visitorProfileEnabled = $systemSettings->disableVisitorProfile->getValue() === false
+ && $systemSettings->disableVisitorLog->getValue() === false;
+ if ($visitorProfileEnabled) {
+ parent::configureSegments($segmentsList, $dimensionSegmentFactory);
+ }
+
+ }
+
public function configureMetrics(MetricsList $metricsList, DimensionMetricFactory $dimensionMetricFactory)
{
$metric = $dimensionMetricFactory->createMetric(ArchivedMetric::AGGREGATION_UNIQUE); |
Hi @tsteur this solves a part of the issue, because this way I refactor the code using this part as base, thanks for pointing this out. |
@tsteur I have a question: |
@flamisz not sure with what you mean hide where it includes it?
That should be fine AFAIK as it's rarely used I would say and the LIVE setting is also new and rarely used yet so this combination should happen very rarely (or not at all) and we might want to rather go for the 80/20 solution. The alternative be to remove any such segments on update or when enabling this feature but I think it might not be needed. |
In the saved segment list dropdown. Hide all segments that has any visitorId dimension, because if we don't hide those, choosing them will throw an error. The change you showed only hide the visitorid dimension from the action list when we create a new segment or editing one. But we can keep them there and throw the error, as you said it's very rarely used settings, and we can expand the description for that setting to warn the admin. So basically this issue about not using visitorid segment when this setting is turned on. What I did is:
Only the first point is enough? So after these modifications we will throw a Is this ok? Do we want to hide those saved segments? They can't use it anyway, we just throw the error then? |
Just one more thing. If someone has this turned on already and has a cron job to run archiving after an update the job will fail if has any segment with visitorId. That's why I thought maybe we should handle that. But as you said, maybe there is no such a combination at all. I go with the bare minimum, update the pr, we can add more if we want. |
Fully aware of this. Same problem is happening when you use segments defined by plugins and then disable the plugin. It's a problem for another day I would say :) |
Description:
fixes: #17488
Review