From 93db9612cce6a50832ee480f494462f30c7b38b4 Mon Sep 17 00:00:00 2001
From: Thomas Steur <thomas.steur@googlemail.com>
Date: Thu, 27 Nov 2014 00:07:28 +0100
Subject: [PATCH] fixes #6727 lazy load plugin settings
---
core/Db.php | 12 ++++-
core/Plugin/Settings.php | 24 ++++++++--
.../Framework/TestCase/SystemTestCase.php | 11 +++++
.../Integration/Plugin/SettingsTest.php | 45 +++++++++++++++++++
4 files changed, 88 insertions(+), 4 deletions(-)
diff --git a/core/Db.php b/core/Db.php
index 556bd2ada5..fa26f34f02 100644
--- a/core/Db.php
+++ b/core/Db.php
@@ -46,7 +46,7 @@ class Db
return Tracker::getDatabase();
}
- if (self::$connection === null) {
+ if (!self::hasDatabaseObject()) {
self::createDatabaseObject();
}
@@ -103,6 +103,16 @@ class Db
self::$connection = $db;
}
+ /**
+ * Detect whether a database object is initialized / created or not.
+ *
+ * @internal
+ */
+ public static function hasDatabaseObject()
+ {
+ return isset(self::$connection);
+ }
+
/**
* Disconnects and destroys the database connection.
*
diff --git a/core/Plugin/Settings.php b/core/Plugin/Settings.php
index 57f8c813fb..0b0bb0953a 100644
--- a/core/Plugin/Settings.php
+++ b/core/Plugin/Settings.php
@@ -58,6 +58,9 @@ abstract class Settings implements StorageInterface
private $introduction;
private $pluginName;
+ // for lazy loading of setting values
+ private $settingValuesLoaded = false;
+
/**
* Constructor.
*/
@@ -76,7 +79,6 @@ abstract class Settings implements StorageInterface
}
$this->init();
- $this->loadSettings();
}
/**
@@ -126,7 +128,7 @@ abstract class Settings implements StorageInterface
});
$settings2 = $settings;
-
+
uasort($settings, function ($setting1, $setting2) use ($settings2) {
/** @var Setting $setting1 */ /** @var Setting $setting2 */
@@ -166,6 +168,8 @@ abstract class Settings implements StorageInterface
*/
public function save()
{
+ $this->loadSettingsIfNotDoneYet();
+
Option::set($this->getOptionKey(), serialize($this->settingsValues));
$pluginName = $this->getPluginName();
@@ -195,6 +199,7 @@ abstract class Settings implements StorageInterface
Option::delete($this->getOptionKey());
$this->settingsValues = array();
+ $this->settingValuesLoaded = false;
}
/**
@@ -210,6 +215,7 @@ abstract class Settings implements StorageInterface
{
$this->checkIsValidSetting($setting->getName());
$this->checkHasEnoughReadPermission($setting);
+ $this->loadSettingsIfNotDoneYet();
if (array_key_exists($setting->getKey(), $this->settingsValues)) {
@@ -247,6 +253,7 @@ abstract class Settings implements StorageInterface
settype($value, $setting->type);
}
+ $this->loadSettingsIfNotDoneYet();
$this->settingsValues[$setting->getKey()] = $value;
}
@@ -259,6 +266,7 @@ abstract class Settings implements StorageInterface
public function removeSettingValue(Setting $setting)
{
$this->checkHasEnoughWritePermission($setting);
+ $this->loadSettingsIfNotDoneYet();
$key = $setting->getKey();
@@ -298,6 +306,16 @@ abstract class Settings implements StorageInterface
return 'Plugin_' . $this->pluginName . '_Settings';
}
+ private function loadSettingsIfNotDoneYet()
+ {
+ if ($this->settingValuesLoaded) {
+ return;
+ }
+
+ $this->settingValuesLoaded = true;
+ $this->loadSettings();
+ }
+
private function loadSettings()
{
$values = Option::get($this->getOptionKey());
@@ -412,7 +430,7 @@ abstract class Settings implements StorageInterface
$setting->validate = function ($value) use ($setting, $pluginName) {
$errorMsg = Piwik::translate('CoreAdminHome_PluginSettingsValueNotAllowed',
- array($setting->title, $pluginName));
+ array($setting->title, $pluginName));
if (is_array($value) && $setting->type == Settings::TYPE_ARRAY) {
foreach ($value as $val) {
diff --git a/tests/PHPUnit/Framework/TestCase/SystemTestCase.php b/tests/PHPUnit/Framework/TestCase/SystemTestCase.php
index 879fc915e2..d0a7c340ca 100755
--- a/tests/PHPUnit/Framework/TestCase/SystemTestCase.php
+++ b/tests/PHPUnit/Framework/TestCase/SystemTestCase.php
@@ -589,4 +589,15 @@ abstract class SystemTestCase extends PHPUnit_Framework_TestCase
{
self::assertThat($url, new ResponseCode($expectedResponseCode), $message);
}
+
+ public function assertNotDbConnectionCreated($message = 'A database connection was created but should not.')
+ {
+ self::assertFalse(Db::hasDatabaseObject(), $message);
+ }
+
+ public function assertDbConnectionCreated($message = 'A database connection was not created but should.')
+ {
+ self::assertTrue(Db::hasDatabaseObject(), $message);
+ }
+
}
diff --git a/tests/PHPUnit/Integration/Plugin/SettingsTest.php b/tests/PHPUnit/Integration/Plugin/SettingsTest.php
index 763ce4f09a..3c146f5d31 100644
--- a/tests/PHPUnit/Integration/Plugin/SettingsTest.php
+++ b/tests/PHPUnit/Integration/Plugin/SettingsTest.php
@@ -9,6 +9,7 @@
namespace Piwik\Tests\Integration\Plugin;
use Piwik\Access;
+use Piwik\Db;
use Piwik\Plugin\Settings as PluginSettings;
use Piwik\Settings\Setting;
use Piwik\Tests\Framework\Mock\FakeAccess;
@@ -42,6 +43,7 @@ class SettingsTest extends IntegrationTestCase
{
parent::setUp();
Access::setSingletonInstance(null);
+ Db::destroyDatabaseObject();
$this->settings = $this->createSettingsInstance();
}
@@ -56,6 +58,49 @@ class SettingsTest extends IntegrationTestCase
parent::tearDown();
}
+ public function test_constructor_shouldNotEstablishADatabaseConnection()
+ {
+ Db::destroyDatabaseObject();
+
+ $this->assertNotDbConnectionCreated();
+
+ $this->createSettingsInstance();
+
+ $this->assertNotDbConnectionCreated();
+ }
+
+ public function test_constructor_shouldEstablishADatabaseConnection_AsSoonAsWeGetAValue()
+ {
+ $this->setSuperUser();
+ Db::destroyDatabaseObject();
+
+ $setting = $this->buildUserSetting('testSetting', 'Test Setting');
+ $settings = $this->createSettingsInstance();
+ $settings->addSetting($setting);
+
+ $this->assertNotDbConnectionCreated();
+
+ $settings->getSettingValue($setting);
+
+ $this->assertDbConnectionCreated();
+ }
+
+ public function test_constructor_shouldEstablishADatabaseConnection_AsSoonAsWeSetAValue()
+ {
+ $this->setSuperUser();
+ Db::destroyDatabaseObject();
+
+ $setting = $this->buildUserSetting('testSetting', 'Test Setting');
+ $settings = $this->createSettingsInstance();
+ $settings->addSetting($setting);
+
+ $this->assertNotDbConnectionCreated();
+
+ $settings->setSettingValue($setting, '5');
+
+ $this->assertDbConnectionCreated();
+ }
+
/**
* @expectedException \Exception
* @expectedExceptionMessage A setting with name "myname" does already exist for plugin "ExampleSettingsPlugin"
--
GitLab