Move more conversion/actions related logic from Visit::handle() to plugin specific RequestProcessors in manipulateVisitProperties method. Also temporarily made GoalManager a singleton that is set as a static var in GoalsRequestProcessor. Should be in DI, but until all of Visit::handle() is dealt w/, this can't be done.

6 files changed, 61 insertions, 32 deletions

diff --git a/core/Tracker/RequestProcessor.php b/core/Tracker/RequestProcessor.php
index 253c8f69f4..9b3be1bb3d 100644
--- a/core/Tracker/RequestProcessor.php
+++ b/core/Tracker/RequestProcessor.php
@@ -12,6 +12,8 @@ use Piwik\Tracker\Visit\VisitProperties;
 
 /**
  * TODO
+ *
+ * TODO: maybe we should rename manipulateVisitProperties to afterRequestProcessed and rename processRequest to handleRequest or recordLogs
  */
 abstract class RequestProcessor
 {
@@ -26,7 +28,7 @@ abstract class RequestProcessor
     /**
      * TODO
      */
-    public function manipulateVisitProperties(VisitProperties $visitProperties)
+    public function manipulateVisitProperties(VisitProperties $visitProperties, Request $request)
     {
         return false;
     }
diff --git a/core/Tracker/Visit.php b/core/Tracker/Visit.php
index 123d3f931b..74e490726d 100644
--- a/core/Tracker/Visit.php
+++ b/core/Tracker/Visit.php
@@ -18,6 +18,7 @@ use Piwik\Exception\UnexpectedWebsiteFoundException;
 use Piwik\Network\IPUtils;
 use Piwik\Piwik;
 use Piwik\Plugin\Dimension\VisitDimension;
+use Piwik\Plugins\Goals\Tracker\GoalsRequestProcessor;
 use Piwik\SettingsPiwik;
 use Piwik\Tracker;
 use Piwik\Tracker\Visit\VisitProperties;
@@ -111,29 +112,6 @@ class Visit implements VisitInterface
             }
         }
 
-        /**
-         * Goals & Ecommerce conversions
-         */
-        /** @var Action $action */
-        $action = $this->visitProperties->getRequestMetadata('Actions', 'action');
-
-        $goalManager = new GoalManager($this->request);
-        $isManualGoalConversion = $goalManager->isManualGoalConversion();
-        $requestIsEcommerce = $goalManager->requestIsEcommerce;
-
-        if (!empty($action)) {
-
-            if (!$requestIsEcommerce && !$isManualGoalConversion) {
-
-                $someGoalsConverted = $goalManager->detectGoalsMatchingUrl($this->request->getIdSite(), $action);
-                $this->visitProperties->setRequestMetadata('Goals', 'someGoalsConverted', $someGoalsConverted);
-                $this->visitProperties->setRequestMetadata('Goals', 'visitIsConverted', $someGoalsConverted);
-
-                $action->loadIdsFromLogActionTable();
-            }
-
-        }
-
         /***
          * Visitor recognition
          */
@@ -143,8 +121,15 @@ class Visit implements VisitInterface
 
         $this->visitProperties->visitorInfo = $visitor->getVisitorInfo();
 
+        /** @var Action $action */
+        $action = $this->visitProperties->getRequestMetadata('Actions', 'action');
+
         $isNewVisit = $this->isVisitNew($visitor, $action);
 
+        $goalManager = GoalsRequestProcessor::$goalManager;
+        $isManualGoalConversion = $goalManager->isManualGoalConversion();
+        $requestIsEcommerce = $goalManager->requestIsEcommerce;
+
         // TODO: re-add optimization where if custom variables exist in request, don't bother selecting them in Visitor
         $this->visitorCustomVariables = $this->request->getCustomVariables($scope = 'visit');
         if (!empty($this->visitorCustomVariables)) {
@@ -153,7 +138,7 @@ class Visit implements VisitInterface
         }
 
         foreach ($this->requestProcessors as $processor) {
-            $abort = $processor->manipulateVisitProperties($this->visitProperties);
+            $abort = $processor->manipulateVisitProperties($this->visitProperties, $this->request);
             if ($abort) {
                 return;
             }
diff --git a/plugins/Actions/Tracker/ActionsRequestProcessor.php b/plugins/Actions/Tracker/ActionsRequestProcessor.php
index 5b19b0ac9f..aa7090b224 100644
--- a/plugins/Actions/Tracker/ActionsRequestProcessor.php
+++ b/plugins/Actions/Tracker/ActionsRequestProcessor.php
@@ -28,4 +28,14 @@ class ActionsRequestProcessor extends RequestProcessor
 
         $visitProperties->setRequestMetadata('Actions', 'action', $action);
     }
-}
\ No newline at end of file
+
+    public function manipulateVisitProperties(VisitProperties $visitProperties, Request $request)
+    {
+        /** @var Action $action */
+        $action = $visitProperties->getRequestMetadata('Actions', 'action');
+
+        if (!empty($action)) { // other plugins can unset the action if they want
+            $action->loadIdsFromLogActionTable();
+        }
+    }
+}
diff --git a/plugins/CoreHome/Tracker/VisitRequestProcessor.php b/plugins/CoreHome/Tracker/VisitRequestProcessor.php
index 513b78b54a..596d0b926f 100644
--- a/plugins/CoreHome/Tracker/VisitRequestProcessor.php
+++ b/plugins/CoreHome/Tracker/VisitRequestProcessor.php
@@ -43,7 +43,7 @@ class VisitRequestProcessor extends RequestProcessor
         return false;
     }
 
-    public function manipulateVisitProperties(VisitProperties $visitProperties)
+    public function manipulateVisitProperties(VisitProperties $visitProperties, Request $request)
     {
         /**
          * Triggered after visits are tested for exclusion so plugins can modify the IP address
diff --git a/plugins/Goals/Tracker/GoalsRequestProcessor.php b/plugins/Goals/Tracker/GoalsRequestProcessor.php
index 0cadff1a21..22315f7511 100644
--- a/plugins/Goals/Tracker/GoalsRequestProcessor.php
+++ b/plugins/Goals/Tracker/GoalsRequestProcessor.php
@@ -9,6 +9,7 @@
 namespace Piwik\Plugins\Goals\Tracker;
 
 use Piwik\Common;
+use Piwik\Tracker\Action;
 use Piwik\Tracker\GoalManager;
 use Piwik\Tracker\Request;
 use Piwik\Tracker\RequestProcessor;
@@ -21,13 +22,20 @@ use Piwik\Tracker\Visit\VisitProperties;
  */
 class GoalsRequestProcessor extends RequestProcessor
 {
+    /**
+     * TODO: GoalManager should be stateless and stored in DI.
+     *
+     * @var GoalManager
+     */
+    public static $goalManager = null;
+
     public function processRequestParams(VisitProperties $visitProperties, Request $request)
     {
-        $goalManager = new GoalManager($request); // TODO: GoalManager should be stateless and stored in DI.
+        self::$goalManager = new GoalManager($request);
 
-        if ($goalManager->isManualGoalConversion()) {
+        if (self::$goalManager->isManualGoalConversion()) {
             // this request is from the JS call to piwikTracker.trackGoal()
-            $someGoalsConverted = $goalManager->detectGoalId($request->getIdSite());
+            $someGoalsConverted = self::$goalManager->detectGoalId($request->getIdSite());
 
             $visitProperties->setRequestMetadata('Goals', 'someGoalsConverted', $someGoalsConverted);
             $visitProperties->setRequestMetadata('Goals', 'visitIsConverted', $someGoalsConverted);
@@ -36,11 +44,30 @@ class GoalsRequestProcessor extends RequestProcessor
 
             // if we find a idgoal in the URL, but then the goal is not valid, this is most likely a fake request
             if (!$someGoalsConverted) {
-                Common::printDebug('Invalid goal tracking request for goal id = ' . $goalManager->idGoal);
+                Common::printDebug('Invalid goal tracking request for goal id = ' . self::$goalManager->idGoal);
                 return true;
             }
         }
 
         return false;
     }
-}
\ No newline at end of file
+
+    public function manipulateVisitProperties(VisitProperties $visitProperties, Request $request)
+    {
+        $visitsConverted = $visitProperties->getRequestMetadata('Goals', 'visitIsConverted');
+
+        /** @var Action $action */
+        $action = $visitProperties->getRequestMetadata('Actions', 'action');
+
+        // if the visit hasn't already been converted another way (ie, manual goal conversion or ecommerce conversion,
+        // try to convert based on the action)
+        if (!$visitsConverted
+            && $action
+        ) {
+            $someGoalsConverted = self::$goalManager->detectGoalsMatchingUrl($request->getIdSite(), $action);
+
+            $visitProperties->setRequestMetadata('Goals', 'someGoalsConverted', $someGoalsConverted);
+            $visitProperties->setRequestMetadata('Goals', 'visitIsConverted', $someGoalsConverted);
+        }
+    }
+}
diff --git a/plugins/Heartbeat/Tracker/PingRequestProcessor.php b/plugins/Heartbeat/Tracker/PingRequestProcessor.php
index 44224ce0ab..d513e3f223 100644
--- a/plugins/Heartbeat/Tracker/PingRequestProcessor.php
+++ b/plugins/Heartbeat/Tracker/PingRequestProcessor.php
@@ -24,7 +24,12 @@ class PingRequestProcessor extends RequestProcessor
             Common::printDebug("-> ping=1 request: we do not track a new action nor a new visit nor any goal.");
 
             $visitProperties->setRequestMetadata('Actions', 'action', null);
+        }
+    }
 
+    public function manipulateVisitProperties(VisitProperties $visitProperties, Request $request)
+    {
+        if ($this->isPingRequest($request)) {
             $visitProperties->setRequestMetadata('Goals', 'someGoalsConverted', false);
             $visitProperties->setRequestMetadata('Goals', 'visitIsConverted', false);
         }