refs #4053 cleanup code of marketplace integration

10 files changed, 144 insertions, 108 deletions

diff --git a/plugins/CorePluginsAdmin/Controller.php b/plugins/CorePluginsAdmin/Controller.php
index 13ebc522c5..706a16c8d8 100644
--- a/plugins/CorePluginsAdmin/Controller.php
+++ b/plugins/CorePluginsAdmin/Controller.php
@@ -37,13 +37,8 @@ class Controller extends \Piwik\Controller\Admin
 
         $view = $this->configureView('@CorePluginsAdmin/' . $template);
 
-        $pluginName = Common::getRequestVar('pluginName', '', 'string');
-        $pluginName = strip_tags($pluginName);
-        $nonce      = Common::getRequestVar('nonce', '', 'string');
-
-        if (empty($pluginName)) {
-            throw new \Exception('Plugin parameter is missing');
-        }
+        $pluginName = Common::getRequestVar('pluginName', null, 'string');
+        $nonce      = Common::getRequestVar('nonce', null, 'string');
 
         $view->plugin = array('name' => $pluginName);
 
@@ -58,7 +53,7 @@ class Controller extends \Piwik\Controller\Admin
             $pluginInstaller = new PluginInstaller($pluginName);
             $pluginInstaller->installOrUpdatePluginFromMarketplace();
 
-        } catch (PluginInstallerException $e) {
+        } catch (\Exception $e) {
             $view->errorMessage = $e->getMessage();
             return $view;
         }
@@ -85,12 +80,7 @@ class Controller extends \Piwik\Controller\Admin
 
     public function pluginDetails()
     {
-        $pluginName = Common::getRequestVar('pluginName', '', 'string');
-        $pluginName = strip_tags($pluginName);
-
-        if (empty($pluginName)) {
-            return;
-        }
+        $pluginName = Common::getRequestVar('pluginName', null, 'string');
 
         $view = $this->configureView('@CorePluginsAdmin/pluginDetails');
 
@@ -107,7 +97,6 @@ class Controller extends \Piwik\Controller\Admin
     private function createBrowsePluginsOrThemesView($template, $themesOnly)
     {
         $query = Common::getRequestVar('query', '', 'string', $_POST);
-        $query = strip_tags($query);
         $sort  = Common::getRequestVar('sort', $this->defaultSortMethod, 'string');
 
         if (!in_array($sort, $this->validSortMethods)) {
@@ -263,13 +252,8 @@ class Controller extends \Piwik\Controller\Admin
     {
         Piwik::checkUserIsSuperUser();
 
-        $pluginName = Common::getRequestVar('pluginName', '', 'string');
-        $pluginName = strip_tags($pluginName);
-        $nonce      = Common::getRequestVar('nonce', '', 'string');
-
-        if (empty($pluginName)) {
-            throw new \Exception('Plugin parameter is missing');
-        }
+        $pluginName = Common::getRequestVar('pluginName', null, 'string');
+        $nonce      = Common::getRequestVar('nonce', null, 'string');
 
         if (!Nonce::verifyNonce('CorePluginsAdmin.activatePlugin', $nonce)) {
             throw new \Exception(Piwik_Translate('General_ExceptionNonceMismatch'));
@@ -283,11 +267,12 @@ class Controller extends \Piwik\Controller\Admin
             $params = array('activated' => 1, 'pluginName' => $pluginName);
             $plugin = PluginsManager::getInstance()->loadPlugin($pluginName);
 
+            $actionToRedirect = 'plugins';
             if ($plugin->isTheme()) {
-                $this->redirectToIndex('CorePluginsAdmin', 'themes', null, null, null, $params);
-            } else {
-                $this->redirectToIndex('CorePluginsAdmin', 'plugins', null, null, null, $params);
+                $actionToRedirect = 'themes';
             }
+
+            $this->redirectToIndex('CorePluginsAdmin', $actionToRedirect, null, null, null, $params);
         }
     }
 
diff --git a/plugins/CorePluginsAdmin/Marketplace.php b/plugins/CorePluginsAdmin/Marketplace.php
index f2b9e18df5..40dfec5479 100644
--- a/plugins/CorePluginsAdmin/Marketplace.php
+++ b/plugins/CorePluginsAdmin/Marketplace.php
@@ -47,10 +47,10 @@ class Marketplace
 
         $dateFormat = Piwik_Translate('CoreHome_ShortDateFormatWithYear');
 
-        foreach ($plugins as $plugin) {
-            $plugin->canBeUpdated = $this->hasPluginUpdate($plugin);
-            $plugin->isInstalled  = PluginsManager::getInstance()->isPluginLoaded($plugin->name);
-            $plugin->lastUpdated  = Date::factory($plugin->lastUpdated)->getLocalized($dateFormat);
+        foreach ($plugins as &$plugin) {
+            $plugin['canBeUpdated'] = $this->hasPluginUpdate($plugin);
+            $plugin['isInstalled']  = PluginsManager::getInstance()->isPluginLoaded($plugin['name']);
+            $plugin['lastUpdated']  = Date::factory($plugin['lastUpdated'])->getLocalized($dateFormat);
         }
 
         return $plugins;
@@ -58,14 +58,14 @@ class Marketplace
 
     private function hasPluginUpdate($plugin)
     {
-        if (empty($plugin->name)) {
+        if (empty($plugin['name'])) {
             return false;
         }
 
-        $pluginsHavingUpdate = $this->getPluginsHavingUpdate($plugin->isTheme);
+        $pluginsHavingUpdate = $this->getPluginsHavingUpdate($plugin['isTheme']);
 
         foreach ($pluginsHavingUpdate as $pluginHavingUpdate) {
-            if ($plugin->name == $pluginHavingUpdate->name) {
+            if ($plugin['name'] == $pluginHavingUpdate['name']) {
                 return true;
             }
         }
@@ -89,12 +89,14 @@ class Marketplace
             $pluginsHavingUpdate = array();
         }
 
-        foreach ($pluginsHavingUpdate as $updatePlugin) {
+        foreach ($pluginsHavingUpdate as &$updatePlugin) {
             foreach ($loadedPlugins as $loadedPlugin) {
 
-                if (!empty($updatePlugin->name) && $loadedPlugin->getPluginName() == $updatePlugin->name) {
-                    $updatePlugin->currentVersion = $loadedPlugin->getVersion();
-                    $updatePlugin->isActivated    = $pluginManager->isPluginActivated($updatePlugin->name);
+                if (!empty($updatePlugin['name'])
+                    && $loadedPlugin->getPluginName() == $updatePlugin['name']) {
+
+                    $updatePlugin['currentVersion'] = $loadedPlugin->getVersion();
+                    $updatePlugin['isActivated']    = $pluginManager->isPluginActivated($updatePlugin['name']);
                     break;
                 }
             }
diff --git a/plugins/CorePluginsAdmin/MarketplaceApiClient.php b/plugins/CorePluginsAdmin/MarketplaceApiClient.php
index 19c8cf636a..dc7b3841a1 100644
--- a/plugins/CorePluginsAdmin/MarketplaceApiClient.php
+++ b/plugins/CorePluginsAdmin/MarketplaceApiClient.php
@@ -11,6 +11,7 @@
 namespace Piwik\Plugins\CorePluginsAdmin;
 use Piwik\CacheFile;
 use Piwik\Http;
+use Piwik\PluginsManager;
 
 /**
  *
@@ -18,6 +19,9 @@ use Piwik\Http;
  */
 class MarketplaceApiClient
 {
+    const CACHE_TIMEOUT_IN_SECONDS = 1200;
+    const HTTP_REQUEST_TIMEOUT = 30;
+
     private $domain = 'http://plugins.piwik.org';
 
     /**
@@ -27,7 +31,7 @@ class MarketplaceApiClient
 
     public function __construct()
     {
-        $this->cache = new CacheFile('marketplace', 1200);
+        $this->cache = new CacheFile('marketplace', self::CACHE_TIMEOUT_IN_SECONDS);
     }
 
     public static function clearAllCacheEntries()
@@ -45,16 +49,13 @@ class MarketplaceApiClient
 
     public function download($pluginOrThemeName, $target)
     {
-        $plugin = $this->getPluginInfo($pluginOrThemeName);
+        $downloadUrl = $this->getDownloadUrl($pluginOrThemeName);
 
-        if (empty($plugin->versions)) {
+        if (empty($downloadUrl)) {
             return false;
         }
 
-        $latestVersion = array_pop($plugin->versions);
-        $downloadUrl   = $latestVersion->download;
-
-        $success = Http::fetchRemoteFile($this->domain . $downloadUrl, $target);
+        $success = Http::fetchRemoteFile($downloadUrl, $target, 0, static::HTTP_REQUEST_TIMEOUT);
 
         return $success;
     }
@@ -68,7 +69,10 @@ class MarketplaceApiClient
         $params = array();
 
         foreach ($plugins as $plugin) {
-            $params[] = array('name' => $plugin->getPluginName(), 'version' => $plugin->getVersion());
+            $pluginName   = $plugin->getPluginName();
+            if (!PluginsManager::getInstance()->isPluginBundledWithCore($pluginName)) {
+                $params[] = array('name' => $plugin->getPluginName(), 'version' => $plugin->getVersion());
+            }
         }
 
         $params = array('plugins' => $params);
@@ -94,9 +98,9 @@ class MarketplaceApiClient
         $pluginDetails = array();
 
         foreach ($hasUpdates as $pluginHavingUpdate) {
-            $plugin = $this->getPluginInfo($pluginHavingUpdate->name);
+            $plugin = $this->getPluginInfo($pluginHavingUpdate['name']);
 
-            if (!empty($plugin->isTheme) == $themesOnly) {
+            if (!empty($plugin['isTheme']) == $themesOnly) {
                 $pluginDetails[] = $plugin;
             }
         }
@@ -108,8 +112,8 @@ class MarketplaceApiClient
     {
         $response = $this->fetch('plugins', array('keywords' => $keywords, 'query' => $query, 'sort' => $sort));
 
-        if (!empty($response->plugins)) {
-            return $response->plugins;
+        if (!empty($response['plugins'])) {
+            return $response['plugins'];
         }
 
         return array();
@@ -119,8 +123,8 @@ class MarketplaceApiClient
     {
         $response = $this->fetch('themes', array('keywords' => $keywords, 'query' => $query, 'sort' => $sort));
 
-        if (!empty($response->plugins)) {
-            return $response->plugins;
+        if (!empty($response['plugins'])) {
+            return $response['plugins'];
         }
 
         return array();
@@ -128,24 +132,27 @@ class MarketplaceApiClient
 
     private function fetch($action, $params)
     {
+        ksort($params);
         $query  = http_build_query($params);
         $result = $this->getCachedResult($action, $query);
 
         if (false === $result) {
             $endpoint = $this->domain . '/api/1.0/';
             $url      = sprintf('%s%s?%s', $endpoint, $action, $query);
-            $result   = Http::sendHttpRequest($url, 5);
-            $this->cacheResult($action, $query, $result);
-        }
+            $response = Http::sendHttpRequest($url, static::HTTP_REQUEST_TIMEOUT);
+            $result   = json_decode($response, true);
 
-        $result = json_decode($result);
+            if (is_null($result)) {
+                $message = sprintf('There was an error reading the response from the Marketplace: %s. Please try again later.',
+                                   substr($response, 0, 50));
+                throw new MarketplaceApiException($message);
+            }
 
-        if (is_null($result)) {
-            throw new MarketplaceApiException('Failure during communication with marketplace, unable to read response');
-        }
+            if (!empty($result['error'])) {
+                throw new MarketplaceApiException($result['error']);
+            }
 
-        if (!empty($result->error)) {
-            throw new MarketplaceApiException($result->error);
+            $this->cacheResult($action, $query, $result);
         }
 
         return $result;
@@ -170,4 +177,23 @@ class MarketplaceApiClient
         return sprintf('api.1.0.%s.%s', str_replace('/', '.', $action), md5($query));
     }
 
+    /**
+     * @param  $pluginOrThemeName
+     * @throws MarketplaceApiException
+     * @return string
+     */
+    public function getDownloadUrl($pluginOrThemeName)
+    {
+        $plugin = $this->getPluginInfo($pluginOrThemeName);
+
+        if (empty($plugin['versions'])) {
+            throw new MarketplaceApiException('Plugin has no versions.');
+        }
+
+        $latestVersion = array_pop($plugin['versions']);
+        $downloadUrl   = $latestVersion['download'];
+
+        return $this->domain . $downloadUrl;
+    }
+
 }
diff --git a/plugins/CorePluginsAdmin/PluginInstaller.php b/plugins/CorePluginsAdmin/PluginInstaller.php
index e77b79757d..cd664b3aef 100644
--- a/plugins/CorePluginsAdmin/PluginInstaller.php
+++ b/plugins/CorePluginsAdmin/PluginInstaller.php
@@ -35,6 +35,7 @@ class PluginInstaller
         $tmpPluginFolder = PIWIK_USER_PATH . self::PATH_TO_DOWNLOAD . $this->pluginName;
 
         $this->makeSureFoldersAreWritable();
+        $this->makeSurePluginNameIsValid();
         $this->downloadPluginFromMarketplace($tmpPluginZip);
         $this->extractPluginFiles($tmpPluginZip, $tmpPluginFolder);
         $this->makeSurePluginJsonExists($tmpPluginFolder);
@@ -53,21 +54,21 @@ class PluginInstaller
     {
         $this->removeFileIfExists($pluginZipTargetFile);
 
-        try {
-            $marketplace   = new MarketplaceApiClient();
-            $pluginDetails = $marketplace->getPluginInfo($this->pluginName);
-        } catch (\Exception $e) {
-            throw new PluginInstallerException($e->getMessage());
-        }
-
-        if (empty($pluginDetails)) {
-            throw new PluginInstallerException('A plugin with this name does not exist');
-        }
+        $marketplace = new MarketplaceApiClient();
 
         try {
             $marketplace->download($this->pluginName, $pluginZipTargetFile);
         } catch (\Exception $e) {
-            throw new PluginInstallerException('Failed to download plugin: ' . $e->getMessage());
+
+            try {
+                $downloadUrl  = $marketplace->getDownloadUrl($this->pluginName);
+                $errorMessage = sprintf('Failed to download plugin from %s: %s', $downloadUrl, $e->getMessage());
+
+            } catch (\Exception $ex) {
+                $errorMessage = sprintf('Failed to download plugin: %s', $e->getMessage());
+            }
+
+            throw new PluginInstallerException($errorMessage);
         }
     }
 
@@ -94,7 +95,7 @@ class PluginInstaller
     private function makeSurePluginJsonExists($tmpPluginFolder)
     {
         if (!file_exists($tmpPluginFolder . DIRECTORY_SEPARATOR . $this->pluginName . DIRECTORY_SEPARATOR . 'plugin.json')) {
-            throw new PluginInstallerException('Plugin is not valid, missing plugin.json');
+            throw new PluginInstallerException('Plugin is not valid, it is missing the plugin.json file.');
         }
     }
 
@@ -112,9 +113,7 @@ class PluginInstaller
      */
     private function removeFolderIfExists($pathExtracted)
     {
-        if (file_exists($pathExtracted)) {
-            Filesystem::unlinkRecursive($pathExtracted, true);
-        }
+        Filesystem::unlinkRecursive($pathExtracted, true);
     }
 
     /**
@@ -127,4 +126,21 @@ class PluginInstaller
         }
     }
 
+    /**
+     * @throws PluginInstallerException
+     */
+    private function makeSurePluginNameIsValid()
+    {
+        try {
+            $marketplace   = new MarketplaceApiClient();
+            $pluginDetails = $marketplace->getPluginInfo($this->pluginName);
+        } catch (\Exception $e) {
+            throw new PluginInstallerException($e->getMessage());
+        }
+
+        if (empty($pluginDetails)) {
+            throw new PluginInstallerException('This plugin was not found in the Marketplace.');
+        }
+    }
+
 }
diff --git a/plugins/CorePluginsAdmin/templates/installPlugin.twig b/plugins/CorePluginsAdmin/templates/installPlugin.twig
index 41757a56d6..9ef95329dd 100644
--- a/plugins/CorePluginsAdmin/templates/installPlugin.twig
+++ b/plugins/CorePluginsAdmin/templates/installPlugin.twig
@@ -18,30 +18,29 @@
 
                     <p>Unzipping theme</p>
 
-                    <p>Installing theme</p>
-
                     <p>You have successfully installed the theme {{ plugin.name }} {{ plugin.latestVersion }}.</p>
 
                     <p><strong><a href="{{ linkTo({'action': 'activate', 'pluginName': plugin.name, 'nonce': nonce}) }}">Activate Theme</a></strong>
 
+                        |
+                        <a href="{{ linkTo({'action': 'extend'}) }}">Back to Extend Piwik</a></p>
+
                 {% else %}
 
                     <p>Downloading plugin from Marketplace</p>
 
                     <p>Unzipping plugin</p>
 
-                    <p>Installing plugin</p>
-
                     <p>You have successfully installed the Plugin {{ plugin.name }} {{ plugin.latestVersion }}.</p>
 
                     <p><strong><a href="{{ linkTo({'action': 'activate', 'pluginName': plugin.name, 'nonce': nonce}) }}">Activate Plugin</a></strong>
 
+                        |
+                        <a href="{{ linkTo({'action': 'extend'}) }}">Back to Extend Piwik</a></p>
+
                 {% endif %}
             </div>
 
-            |
-            <a href="{{ linkTo({'action': 'extend'}) }}">Back to Extend Piwik</a></p>
-
         {% endif %}
     </div>
 
diff --git a/plugins/CorePluginsAdmin/templates/pluginMetadata.twig b/plugins/CorePluginsAdmin/templates/pluginMetadata.twig
index f6752d0e64..831dc3b220 100644
--- a/plugins/CorePluginsAdmin/templates/pluginMetadata.twig
+++ b/plugins/CorePluginsAdmin/templates/pluginMetadata.twig
@@ -1,4 +1,4 @@
-<hr class="metadataSeparator">
+<hr class="metadataSeparator"/>
 <ul class="metadata">
     <li>{{ 'CorePluginsAdmin_Version'|translate }}: <strong>{{ plugin.latestVersion }}</strong></li>
     <li class="even">Updated: <strong>{{ plugin.lastUpdated }}</strong></li>
diff --git a/plugins/CorePluginsAdmin/templates/pluginOverview.twig b/plugins/CorePluginsAdmin/templates/pluginOverview.twig
index d5da8de487..39a39b5052 100644
--- a/plugins/CorePluginsAdmin/templates/pluginOverview.twig
+++ b/plugins/CorePluginsAdmin/templates/pluginOverview.twig
@@ -1,21 +1,22 @@
-{% if not isSuperUser %}
-{% elseif plugin.canBeUpdated %}
-    <a class="update"
-       href="{{ linkTo({'action':'updatePlugin', 'pluginName': plugin.name, 'nonce': updateNonce}) }}"
-       >Update</a>
-{% elseif plugin.isInstalled %}
-    <span class="install">Installed</span>
-{% else %}
-    <a href="{{ linkTo({'action': 'installPlugin', 'pluginName': plugin.name, 'nonce': installNonce}) }}"
-       class="install">Install</a>
+{% if isSuperUser %}
+    {% if plugin.canBeUpdated %}
+        <a class="update"
+           href="{{ linkTo({'action':'updatePlugin', 'pluginName': plugin.name, 'nonce': updateNonce}) }}"
+           >Update</a>
+    {% elseif plugin.isInstalled %}
+        <span class="install">Installed</span>
+    {% else %}
+        <a href="{{ linkTo({'action': 'installPlugin', 'pluginName': plugin.name, 'nonce': installNonce}) }}"
+           class="install">Install</a>
+    {% endif %}
 {% endif %}
 
-<h3 class="header">
+<h3 class="header" title="Click for more details">
     <a href="javascript:return;" data-pluginName="{{ plugin.name }}" class="more">{{ plugin.name }}</a>
 </h3>
 <p class="description">{{ plugin.description }}
     <br />
-    <a href="javascript:return;" data-pluginName="{{ plugin.name }}" class="more">&gt;&gt; more</a>
+    <a href="javascript:return;" title="Click for more details" data-pluginName="{{ plugin.name }}" class="more">&gt;&gt; more</a>
 </p>
 {% if plugin.canBeUpdated %}
     <p class="updateAvailableNotice">You can update this plugin to version {{ plugin.latestVersion }}</p>
diff --git a/plugins/CorePluginsAdmin/templates/plugins.twig b/plugins/CorePluginsAdmin/templates/plugins.twig
index 7b6887e915..7fa22d01cb 100644
--- a/plugins/CorePluginsAdmin/templates/plugins.twig
+++ b/plugins/CorePluginsAdmin/templates/plugins.twig
@@ -6,12 +6,14 @@
 <div style="max-width:980px;">
 
     {% if activatedPluginName %}
-        <div id="feedback-success">You have successfully activated plugin {{ activatedPluginName }}</div>
+        <div id="feedback-success"><strong>Well done!</strong> You have successfully activated plugin {{ activatedPluginName }}</div>
     {% endif %}
 
     {% if pluginsHavingUpdate|length %}
         <h2>{{ pluginsHavingUpdate|length }} Update(s) available</h2>
 
+        <p>{{ 'Update your plugins now to benefit from the latest improvements.'|translate }}</p>
+
         {{ plugins.tablePluginUpdates(pluginsHavingUpdate, updateNonce, activateNonce, 0) }}
     {% endif %}
 
diff --git a/plugins/CorePluginsAdmin/templates/themeOverview.twig b/plugins/CorePluginsAdmin/templates/themeOverview.twig
index dffb65aa25..63a5700c51 100644
--- a/plugins/CorePluginsAdmin/templates/themeOverview.twig
+++ b/plugins/CorePluginsAdmin/templates/themeOverview.twig
@@ -1,22 +1,25 @@
-{% if not isSuperUser %}
-{% elseif plugin.canBeUpdated %}
-    <a href="{{ linkTo({'action':'updatePlugin', 'pluginName': plugin.name, 'nonce': updateNonce}) }}"
-       class="update"
-       >{{ 'CoreUpdater_UpdateTitle'|translate }}</a>
-{% elseif plugin.isInstalled %}
-    <span class="install">{{ 'General_Installed'|translate }}</span>
-{% else %}
-    <a href="{{ linkTo({'action': 'installPlugin', 'pluginName': plugin.name, 'nonce': installNonce}) }}"
-       class="install">Install</a>
+{% if isSuperUser %}
+    {% if plugin.canBeUpdated %}
+        <a href="{{ linkTo({'action':'updatePlugin', 'pluginName': plugin.name, 'nonce': updateNonce}) }}"
+           class="update"
+           >{{ 'CoreUpdater_UpdateTitle'|translate }}</a>
+    {% elseif plugin.isInstalled %}
+        <span class="install">{{ 'General_Installed'|translate }}</span>
+    {% else %}
+        <a href="{{ linkTo({'action': 'installPlugin', 'pluginName': plugin.name, 'nonce': installNonce}) }}"
+           class="install">Install</a>
+    {% endif %}
 {% endif %}
 
-<h3 class="header">
+<h3 class="header" title="Click for more details">
     <a href="javascript:return;" data-pluginName="{{ plugin.name }}" class="more">{{ plugin.name }}</a>
 </h3>
+
 <p class="description">{{ plugin.description }}</p>
 
-<a href="javascript:return;" data-pluginName="{{ plugin.name }}" class="more"><img
+<a href="javascript:return;" data-pluginName="{{ plugin.name }}" class="more"><img  title="Click for more details"
             class="preview" src="{{ plugin.screenshots|first }}?w=250&h=250"/></a>
+
 {% if plugin.canBeUpdated %}
     <p class="updateAvailableNotice">You can update this theme to version {{ plugin.latestVersion }}</p>
 {% endif %}
\ No newline at end of file
diff --git a/plugins/CorePluginsAdmin/templates/themes.twig b/plugins/CorePluginsAdmin/templates/themes.twig
index b364c0fabf..c88e3d8f5f 100644
--- a/plugins/CorePluginsAdmin/templates/themes.twig
+++ b/plugins/CorePluginsAdmin/templates/themes.twig
@@ -6,12 +6,14 @@
 <div style="max-width:980px;">
 
     {% if activatedPluginName %}
-        <div id="feedback-success">You have successfully activated plugin {{ activatedPluginName }}</div>
+        <div id="feedback-success"><strong>Well done!</strong> You have successfully activated theme {{ activatedPluginName }}</div>
     {% endif %}
 
     {% if pluginsHavingUpdate|length %}
         <h2>{{ pluginsHavingUpdate|length }} Update(s) available</h2>
 
+        <p>{{ 'Update your themes to enjoy the latest version.'|translate }}</p>
+
         {{ plugins.tablePluginUpdates(pluginsHavingUpdate, updateNonce, true) }}
     {% endif %}