From 0e602c195dff464a5f1361ea2f818cc13f2bda2f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?J=C3=B8rn=20=C3=85ne?= <jorn.dejong@uninett.no>
Date: Tue, 24 Sep 2019 22:28:52 +0200
Subject: [PATCH] Clean up getConfigItem (#1196)
* Ensure getConfig* functions can only return Configuration
* Cleanup after #1189
* Deprecate Configuration::getConfigList
---
lib/SimpleSAML/Auth/Simple.php | 11 ++------
lib/SimpleSAML/Configuration.php | 32 ++++++++++------------
lib/SimpleSAML/Locale/Translate.php | 7 ++---
lib/SimpleSAML/Module.php | 4 +--
lib/SimpleSAML/Stats.php | 2 +-
lib/SimpleSAML/Utils/HTTP.php | 7 ++---
tests/lib/SimpleSAML/ConfigurationTest.php | 6 ++--
7 files changed, 29 insertions(+), 40 deletions(-)
diff --git a/lib/SimpleSAML/Auth/Simple.php b/lib/SimpleSAML/Auth/Simple.php
index 2bfa424ec..efd77a70d 100644
--- a/lib/SimpleSAML/Auth/Simple.php
+++ b/lib/SimpleSAML/Auth/Simple.php
@@ -23,7 +23,7 @@ class Simple
*/
protected $authSource;
- /** @var \SimpleSAML\Configuration|null */
+ /** @var \SimpleSAML\Configuration */
protected $app_config;
/** @var \SimpleSAML\Session */
@@ -45,7 +45,7 @@ class Simple
$config = Configuration::getInstance();
}
$this->authSource = $authSource;
- $this->app_config = $config->getConfigItem('application', null);
+ $this->app_config = $config->getConfigItem('application');
if ($session === null) {
$session = Session::getSessionFromRequest();
@@ -389,12 +389,7 @@ class Simple
$port = '';
}
- if (is_null($this->app_config)) {
- // nothing more we can do here
- return $scheme.'://'.$host.$port.$path.($query ? '?'.$query : '').($fragment ? '#'.$fragment : '');
- }
-
- $base = rtrim($this->app_config->getString(
+ $base = trim($this->app_config->getString(
'baseURL',
$scheme.'://'.$host.$port
), '/');
diff --git a/lib/SimpleSAML/Configuration.php b/lib/SimpleSAML/Configuration.php
index 25627f565..2350b0665 100644
--- a/lib/SimpleSAML/Configuration.php
+++ b/lib/SimpleSAML/Configuration.php
@@ -981,23 +981,25 @@ class Configuration implements Utils\ClearableState
* is given.
*
* @param string $name The name of the option.
- * @param mixed $default A default value which will be returned if the option isn't found. The option will be
- * required if this parameter isn't given. The default value can be any value, including
- * null.
+ * @param array|null $default A default value which will be used if the option isn't found. An empty Configuration
+ * object will be returned if this parameter isn't given and the option doesn't exist.
+ * This function will only return null if $default is set to null and the option
+ * doesn't exist.
*
* @return mixed The option with the given name, or $default if the option isn't found and $default is specified.
*
* @throws \Exception If the option is not an array.
*/
- public function getConfigItem($name, $default = self::REQUIRED_OPTION)
+ public function getConfigItem($name, $default = [])
{
assert(is_string($name));
$ret = $this->getValue($name, $default);
- if ($ret === $default) {
- // the option wasn't found, or it matches the default value. In any case, return this value
- return $ret;
+ if ($ret === null) {
+ // the option wasn't found, or it is explicitly null
+ // do not instantiate a new Configuration instance, but just return null
+ return null;
}
if (!is_array($ret)) {
@@ -1022,24 +1024,18 @@ class Configuration implements Utils\ClearableState
* default value is given.
*
* @param string $name The name of the option.
- * @param mixed $default A default value which will be returned if the option isn't found. The option will be
- * required if this parameter isn't given. The default value can be any value, including
- * null.
*
- * @return mixed The option with the given name, or $default if the option isn't found and $default is specified.
+ * @return array The array of \SimpleSAML\Configuration objects.
*
* @throws \Exception If the value of this element is not an array.
+ *
+ * @deprecated Very specific function, will be removed in a future release; use getConfigItem or getArray instead
*/
- public function getConfigList($name, $default = self::REQUIRED_OPTION)
+ public function getConfigList($name)
{
assert(is_string($name));
- $ret = $this->getValue($name, $default);
-
- if ($ret === $default) {
- // the option wasn't found, or it matches the default value. In any case, return this value
- return $ret;
- }
+ $ret = $this->getValue($name, []);
if (!is_array($ret)) {
throw new \Exception(
diff --git a/lib/SimpleSAML/Locale/Translate.php b/lib/SimpleSAML/Locale/Translate.php
index a92ca7ee8..77c00f0e6 100644
--- a/lib/SimpleSAML/Locale/Translate.php
+++ b/lib/SimpleSAML/Locale/Translate.php
@@ -546,11 +546,8 @@ class Translate
// we don't have a translation for the current language, load alternative priorities
$sspcfg = Configuration::getInstance();
- $langcfg = $sspcfg->getConfigItem('language', null);
- $priorities = [];
- if ($langcfg instanceof Configuration) {
- $priorities = $langcfg->getArray('priorities', []);
- }
+ $langcfg = $sspcfg->getConfigItem('language');
+ $priorities = $langcfg->getArray('priorities', []);
if (!empty($priorities[$context['currentLanguage']])) {
foreach ($priorities[$context['currentLanguage']] as $lang) {
diff --git a/lib/SimpleSAML/Module.php b/lib/SimpleSAML/Module.php
index 2fd6426ea..61986c276 100644
--- a/lib/SimpleSAML/Module.php
+++ b/lib/SimpleSAML/Module.php
@@ -272,8 +272,8 @@ class Module
}
}
- $assetConfig = $config->getConfigItem('assets', new Configuration([], '[assets]'));
- $cacheConfig = $assetConfig->getConfigItem('caching', new Configuration([], '[assets][caching]'));
+ $assetConfig = $config->getConfigItem('assets');
+ $cacheConfig = $assetConfig->getConfigItem('caching');
$response = new BinaryFileResponse($path);
$response->setCache([
// "public" allows response caching even if the request was authenticated,
diff --git a/lib/SimpleSAML/Stats.php b/lib/SimpleSAML/Stats.php
index 3a75fc357..25ff214ac 100644
--- a/lib/SimpleSAML/Stats.php
+++ b/lib/SimpleSAML/Stats.php
@@ -54,7 +54,7 @@ class Stats
{
$config = Configuration::getInstance();
- $outputCfgs = $config->getConfigList('statistics.out', []);
+ $outputCfgs = $config->getConfigList('statistics.out');
self::$outputs = [];
foreach ($outputCfgs as $cfg) {
diff --git a/lib/SimpleSAML/Utils/HTTP.php b/lib/SimpleSAML/Utils/HTTP.php
index 2fc9f2226..52cae76ed 100644
--- a/lib/SimpleSAML/Utils/HTTP.php
+++ b/lib/SimpleSAML/Utils/HTTP.php
@@ -829,8 +829,8 @@ class HTTP
*/
/** @var \SimpleSAML\Configuration $appcfg */
- $appcfg = $cfg->getConfigItem('application', null);
- $appurl = ($appcfg instanceof Configuration) ? $appcfg->getString('baseURL', '') : '';
+ $appcfg = $cfg->getConfigItem('application');
+ $appurl = $appcfg->getString('baseURL', '');
if (!empty($appurl)) {
$protocol = parse_url($appurl, PHP_URL_SCHEME);
$hostname = parse_url($appurl, PHP_URL_HOST);
@@ -838,8 +838,7 @@ class HTTP
$port = !empty($port) ? ':'.$port : '';
} else {
// no base URL specified for app, just use the current URL
- $protocol = 'http';
- $protocol .= (self::getServerHTTPS()) ? 's' : '';
+ $protocol = self::getServerHTTPS() ? 'https' : 'http';
$hostname = self::getServerHost();
$port = self::getServerPort();
}
diff --git a/tests/lib/SimpleSAML/ConfigurationTest.php b/tests/lib/SimpleSAML/ConfigurationTest.php
index 0fde35e9f..2ffa31d74 100644
--- a/tests/lib/SimpleSAML/ConfigurationTest.php
+++ b/tests/lib/SimpleSAML/ConfigurationTest.php
@@ -523,9 +523,11 @@ class ConfigurationTest extends \SimpleSAML\Test\Utils\ClearStateTestCase
$c = Configuration::loadFromArray([
'opt' => ['a' => 42],
]);
- $this->assertEquals($c->getConfigItem('missing_opt', '--missing--'), '--missing--');
+ $this->assertNull($c->getConfigItem('missing_opt', null));
$opt = $c->getConfigItem('opt');
+ $notOpt = $c->getConfigItem('notOpt');
$this->assertInstanceOf('SimpleSAML\Configuration', $opt);
+ $this->assertInstanceOf('SimpleSAML\Configuration', $notOpt);
$this->assertEquals($opt->getValue('a'), 42);
}
@@ -556,7 +558,7 @@ class ConfigurationTest extends \SimpleSAML\Test\Utils\ClearStateTestCase
'b' => ['opt2' => 'value2'],
],
]);
- $this->assertEquals($c->getConfigList('missing_opt', '--missing--'), '--missing--');
+ $this->assertEquals($c->getConfigList('missing_opt'), []);
$opts = $c->getConfigList('opts');
$this->assertInternalType('array', $opts);
$this->assertEquals(array_keys($opts), ['a', 'b']);
--
GitLab