refs #4610 some code tweaks and optimizations

matomo-org · Feb 11, 2014 · 531b357 · 531b357
1 parent fc57db8
commit 531b357
Show file tree

Hide file tree

Showing 6 changed files with 80 additions and 36 deletions.
diff --git a/core/CliMulti.php b/core/CliMulti.php
@@ -54,7 +54,7 @@ public function request(array $piwikUrls)
 
         do {
             usleep(100000); // 100 * 1000 = 100ms
-        } while (!$this->isFinished());
+        } while (!$this->hasFinished());
 
         $results = $this->getResponse($piwikUrls);
         $this->cleanup();
@@ -90,33 +90,12 @@ private function start($piwikUrls)
         }
     }
 
-    private function getQueryFromUrl($aUrl, array $additionalParams)
-    {
-        $url   = @parse_url($aUrl);
-        $query = '';
-
-        if (!empty($url['query'])) {
-            $query .= $url['query'];
-        }
-
-        if (!empty($additionalParams)) {
-            if (!empty($query)) {
-                $query .= '&';
-            }
-
-            $query .= http_build_query($additionalParams);
-        }
-
-        return $query;
-    }
-
     private function buildCommand($query, $outputFile)
     {
-        $appendix = $this->supportsAsync ? ' > ' . $outputFile . ' 2>&1 &' : '';
-        $bin      = $this->getPhpBinary();
+        $bin = $this->findPhpBinary();
 
-        return sprintf('%s %s/console climulti:request %s %s',
-                       $bin, PIWIK_INCLUDE_PATH, escapeshellarg($query), $appendix);
+        return sprintf('%s %s/console climulti:request %s > %s 2>&1 &',
+                       $bin, PIWIK_INCLUDE_PATH, escapeshellarg($query), $outputFile);
     }
 
     private function getResponse()
@@ -130,7 +109,7 @@ private function getResponse()
         return $response;
     }
 
-    private function isFinished()
+    private function hasFinished()
     {
         foreach ($this->processes as $index => $process) {
             $hasStarted = $process->hasStarted();
@@ -160,7 +139,7 @@ private function isFinished()
 
     private function generateCommandId($command)
     {
-        return md5($command . microtime(true) . rand(0, 99999));
+        return substr(Common::hash($command . microtime(true) . rand(0, 99999)), 0, 100);
     }
 
     /**
@@ -169,7 +148,7 @@ private function generateCommandId($command)
      */
     private function supportsAsync()
     {
-        return !SettingsServer::isWindows() && Process::isSupported() && $this->getPhpBinary();
+        return !SettingsServer::isWindows() && Process::isSupported() && $this->findPhpBinary();
     }
 
     private function cleanup()
@@ -203,7 +182,7 @@ public static function cleanupNotRemovedFiles()
         }
     }
 
-    private function getPhpBinary()
+    private function findPhpBinary()
     {
         if (defined('PHP_BINARY')) {
             return PHP_BINARY;
@@ -224,7 +203,7 @@ private function executeAsyncCli($url, Output $output, $cmdId)
     {
         $this->processes[] = new Process($cmdId);
 
-        $query   = $this->getQueryFromUrl($url, array('pid' => $cmdId));
+        $query   = Url::getQueryFromUrl($url, array('pid' => $cmdId));
         $command = $this->buildCommand($query, $output->getPathToFile());
 
         shell_exec($command);

diff --git a/core/CliMulti/Process.php b/core/CliMulti/Process.php
@@ -52,9 +52,11 @@ private function markAsNotStarted()
         $this->writePidFileContent('');
     }
 
-    public function hasStarted()
+    public function hasStarted($content = null)
     {
-        $content = $this->getPidFileContent();
+        if (is_null($content)) {
+            $content = $this->getPidFileContent();
+        }
 
         if (!$this->doesPidFileExist($content)) {
             // process is finished, this means there was a start before
@@ -99,7 +101,7 @@ public function isRunning()
             return true;
         }
 
-        if ($this->hasStarted()) {
+        if ($this->hasStarted($content)) {
             $this->finishProcess();
         }
 
@@ -118,15 +120,14 @@ private function doesPidFileExist($content)
 
     private function isProcessStillRunning($content)
     {
-        $lockedPID = trim($content);
-
         if (!$this->isSupported) {
             return true;
         }
 
+        $lockedPID   = trim($content);
         $runningPIDs = explode("\n", trim( `ps -e | awk '{print $1}'` ));
 
-        return in_array($lockedPID, $runningPIDs);
+        return !empty($lockedPID) && in_array($lockedPID, $runningPIDs);
     }
 
     private function getPidFileContent()

diff --git a/core/Url.php b/core/Url.php
@@ -434,6 +434,35 @@ static public function getQueryStringFromParameters($parameters)
         return $query;
     }
 
+    /**
+     * Returns the query part from any valid url and adds additional parameters to the query part if needed.
+     *
+     * @param string $aUrl    Any url eg `"http://example.com/piwik/?foo=bar"`
+     * @param array $additionalParamsToAdd    If not empty the given parameters will be added to the query.
+     *
+     * @return string eg. `"foo=bar&foo2=bar2"`
+     * @api
+     */
+    static public function getQueryFromUrl($aUrl, array $additionalParamsToAdd)
+    {
+        $url   = @parse_url($aUrl);
+        $query = '';
+
+        if (!empty($url['query'])) {
+            $query .= $url['query'];
+        }
+
+        if (!empty($additionalParamsToAdd)) {
+            if (!empty($query)) {
+                $query .= '&';
+            }
+
+            $query .= self::getQueryStringFromParameters($additionalParamsToAdd);
+        }
+
+        return $query;
+    }
+
     /**
      * Redirects the user to the referrer. If no referrer exists, the user is redirected
      * to the current URL without query string.

diff --git a/tests/PHPUnit/Core/CliMulti/OutputTest.php b/tests/PHPUnit/Core/CliMulti/OutputTest.php
@@ -43,6 +43,14 @@ public function test_exists_ShouldReturnsFalse_IfNothingWrittenYet()
         $this->assertFalse($this->output->exists());
     }
 
+    public function test_getPathToFile_shouldReturnFullPath()
+    {
+        $expectedEnd = '/tmp/climulti/myid.output';
+
+        $this->assertStringEndsWith($expectedEnd, $this->output->getPathToFile());
+        $this->assertGreaterThan(strlen($expectedEnd), strlen($this->output->getPathToFile()));
+    }
+
     public function test_exists_ShouldReturnTrue_IfSomethingIsWritten()
     {
         $this->output->write('test');

diff --git a/tests/PHPUnit/Core/CliMulti/ProcessTest.php b/tests/PHPUnit/Core/CliMulti/ProcessTest.php
@@ -84,6 +84,14 @@ public function test_finishProcess_ShouldNotThrowError_IfNotStartedBefore()
         $this->assertTrue($this->process->hasFinished());
     }
 
+    public function test_hasStarted()
+    {
+        $this->assertTrue($this->process->hasStarted(false));
+        $this->assertTrue($this->process->hasStarted('6341'));
+
+        $this->assertFalse($this->process->hasStarted(''));
+    }
+
     public function test_isSupported()
     {
         $this->assertTrue(Process::isSupported(), 'This test does not work on windows or if the commands ps and awk are not available');

diff --git a/tests/PHPUnit/Core/UrlTest.php b/tests/PHPUnit/Core/UrlTest.php
@@ -280,5 +280,24 @@ public function getQueryParameters()
         );
     }
 
+    public function test_getQueryFromUrl_ShouldReturnEmtpyString_IfNoQuery()
+    {
+        $this->assertEquals('', Url::getQueryFromUrl('', array()));
+        $this->assertEquals('', Url::getQueryFromUrl(null, array()));
+        $this->assertEquals('', Url::getQueryFromUrl('http://localhost/path', array()));
+    }
+
+    public function test_getQueryFromUrl_ShouldReturnOnlyTheQueryPartOfTheUrl_IfNoAdditionalParamsGiven()
+    {
+        $this->assertEquals('foo=bar&foo2=bar2&test[]=1', Url::getQueryFromUrl('http://example.com/?foo=bar&foo2=bar2&test[]=1', array()));
+        $this->assertEquals('foo=bar&foo2=bar2&test[]=1', Url::getQueryFromUrl('/?foo=bar&foo2=bar2&test[]=1', array()));
+    }
+
+    public function test_getQueryFromUrl_ShouldAddAdditionalParams_IfGiven()
+    {
+        $this->assertEquals('foo=bar&foo2=bar2&test[]=1&add=foo', Url::getQueryFromUrl('http://example.com/?foo=bar&foo2=bar2&test[]=1', array('add' => 'foo')));
+        $this->assertEquals('add=foo', Url::getQueryFromUrl('/', array('add' => 'foo')));
+        $this->assertEquals('add[]=foo&add[]=test', Url::getQueryFromUrl('/', array('add' => array('foo', 'test'))));
+    }
 
 }