From fd9345cda3745d026597e7b42edcd196e82dc6bb Mon Sep 17 00:00:00 2001 From: Marc Tobias Metten Date: Fri, 23 Feb 2018 01:46:18 +0100 Subject: [PATCH 1/7] PHP tests for ParameterParser --- test/php/Nominatim/ParameterParserTest.php | 214 +++++++++++++++++++++ 1 file changed, 214 insertions(+) create mode 100644 test/php/Nominatim/ParameterParserTest.php diff --git a/test/php/Nominatim/ParameterParserTest.php b/test/php/Nominatim/ParameterParserTest.php new file mode 100644 index 00000000..ec9e3fea --- /dev/null +++ b/test/php/Nominatim/ParameterParserTest.php @@ -0,0 +1,214 @@ + '1', + 'bool2' => '0', + 'bool3' => 'true', + 'bool4' => 'false', + 'bool5' => '' + ]); + + $this->assertEquals(false, $oParams->getBool('non-exists')); + $this->assertEquals(true, $oParams->getBool('non-exists', true)); + $this->assertEquals(true, $oParams->getBool('bool1')); + $this->assertEquals(false, $oParams->getBool('bool2')); + $this->assertEquals(true, $oParams->getBool('bool3')); + $this->assertEquals(true, $oParams->getBool('bool4')); + $this->assertEquals(false, $oParams->getBool('bool5')); + } + + + public function testGetInt() + { + $oParams = new ParameterParser([ + 'int1' => '5', + 'int2' => 'a', + 'int3' => '-1', + 'int4' => '', + 'int5' => 0 + ]); + + $this->assertEquals(false, $oParams->getInt('non-exists')); + $this->assertEquals(999, $oParams->getInt('non-exists', 999)); + $this->assertEquals(5, $oParams->getInt('int1')); + + try { + $this->assertEquals(false, $oParams->getInt('int2')); + } catch (Exception $e) { + $this->assertEquals($e->getMessage(), "Integer number expected for parameter 'int2'"); + } + $this->assertEquals(-1, $oParams->getInt('int3')); + $this->assertEquals(false, $oParams->getInt('int4')); + $this->assertEquals(false, $oParams->getInt('int5')); // FIXME: should be 0 instead? + } + + + public function testGetFloat() + { + $oParams = new ParameterParser([ + 'float1' => '1.0', + 'float2' => '-5', + 'float3' => '-55.', + 'float4' => 'a', + 'float5' => '', + 'float6' => 0 + ]); + + $this->assertEquals(false, $oParams->getFloat('non-exists')); + $this->assertEquals(999, $oParams->getFloat('non-exists', 999)); + $this->assertEquals(1, $oParams->getFloat('float1')); + $this->assertEquals(-5, $oParams->getFloat('float2')); + + try { + $this->assertEquals(false, $oParams->getFloat('float3')); + } catch (Exception $e) { + $this->assertEquals($e->getMessage(), "Floating-point number expected for parameter 'float3'"); + } + + try { + $this->assertEquals(false, $oParams->getFloat('float4')); + } catch (Exception $e) { + $this->assertEquals($e->getMessage(), "Floating-point number expected for parameter 'float4'"); + } + $this->assertEquals(false, $oParams->getFloat('float5')); + $this->assertEquals(false, $oParams->getFloat('float6')); // FIXME: should be 0 instead? + } + + + + public function testGetString() + { + $oParams = new ParameterParser([ + 'str1' => 'abc', + 'str2' => '', + 'str3' => '0' + ]); + + $this->assertEquals(false, $oParams->getString('non-exists')); + $this->assertEquals('default', $oParams->getString('non-exists', 'default')); + $this->assertEquals('abc', $oParams->getString('str1')); + $this->assertEquals(false, $oParams->getStringList('str2')); + $this->assertEquals(false, $oParams->getStringList('str3')); // FIXME: should be 0 instead? + } + + + + public function testGetSet() + { + $oParams = new ParameterParser([ + 'val1' => 'foo', + 'val2' => 'FOO', + 'val3' => '', + 'val4' => 0 + ]); + + $this->assertEquals(false, $oParams->getSet('non-exists', ['foo', 'bar'])); + // FIXME: unclear if the default value has to be part of the set + $this->assertEquals('default', $oParams->getSet('non-exists', ['foo', 'bar'], 'default')); + $this->assertEquals('foo', $oParams->getSet('val1', ['foo', 'bar'])); + + try { + $this->assertEquals(false, $oParams->getSet('val2', ['foo', 'bar'])); + } catch (Exception $e) { + $this->assertEquals($e->getMessage(), "Parameter 'val2' must be one of: foo, bar"); + } + $this->assertEquals(false, $oParams->getSet('val3', ['foo', 'bar'])); + $this->assertEquals(false, $oParams->getSet('val4', ['foo', 'bar'])); + } + + + public function testGetStringList() + { + $oParams = new ParameterParser([ + 'list1' => ',a,b,c,,c,d', + 'list2' => 'a', + 'list3' => '', + 'list4' => '0' + ]); + + $this->assertEquals(false, $oParams->getStringList('non-exists')); + $this->assertEquals(['a', 'b'], $oParams->getStringList('non-exists', ['a', 'b'])); + // FIXME: unclear if empty string items should be removed + $this->assertEquals(['', 'a', 'b', 'c', '', 'c', 'd'], $oParams->getStringList('list1')); + $this->assertEquals(['a'], $oParams->getStringList('list2')); + $this->assertEquals(false, $oParams->getStringList('list3')); + $this->assertEquals(false, $oParams->getStringList('list4')); + } + + public function testGetPreferredLanguages() + { + $oParams = new ParameterParser(['accept-language' => '']); + $this->assertEquals([ + 'brand' => 'brand', + 'ref' => 'ref', + 'type' => 'type', + 'name' => 'name', + 'name:default' => 'name:default', + 'short_name' => 'short_name', + 'short_name:default' => 'short_name:default', + 'official_name' => 'official_name', + 'official_name:default' => 'official_name:default', + ], $oParams->getPreferredLanguages('default')); + + $oParams = new ParameterParser(['accept-language' => 'de,en']); + $this->assertEquals([ + 'brand' => 'brand', + 'ref' => 'ref', + 'type' => 'type', + 'name' => 'name', + 'name:de' => 'name:de', + 'name:en' => 'name:en', + 'short_name' => 'short_name', + 'short_name:de' => 'short_name:de', + 'short_name:en' => 'short_name:en', + 'official_name' => 'official_name', + 'official_name:de' => 'official_name:de', + 'official_name:en' => 'official_name:en', + ], $oParams->getPreferredLanguages('default')); + + $oParams = new ParameterParser(['accept-language' => 'fr-ca,fr;q=0.8,en-ca;q=0.5,en;q=0.3']); + $this->assertEquals([ + 'short_name:fr-ca' => 'short_name:fr-ca', + 'name:fr-ca' => 'name:fr-ca', + 'short_name:fr' => 'short_name:fr', + 'name:fr' => 'name:fr', + 'short_name:en-ca' => 'short_name:en-ca', + 'name:en-ca' => 'name:en-ca', + 'short_name:en' => 'short_name:en', + 'name:en' => 'name:en', + 'short_name' => 'short_name', + 'name' => 'name', + 'brand' => 'brand', + 'official_name:fr-ca' => 'official_name:fr-ca', + 'official_name:fr' => 'official_name:fr', + 'official_name:en-ca' => 'official_name:en-ca', + 'official_name:en' => 'official_name:en', + 'official_name' => 'official_name', + 'ref' => 'ref', + 'type' => 'type', + ], $oParams->getPreferredLanguages('default')); + } +} From 146779340c1e0b29ff2f2e2a6d1f2ec2251d59f4 Mon Sep 17 00:00:00 2001 From: Marc Tobias Metten Date: Sat, 24 Feb 2018 18:14:34 +0100 Subject: [PATCH 2/7] use setExpectedException to make sure exceptions are really thrown --- test/php/Nominatim/ParameterParserTest.php | 82 +++++++++++----------- 1 file changed, 42 insertions(+), 40 deletions(-) diff --git a/test/php/Nominatim/ParameterParserTest.php b/test/php/Nominatim/ParameterParserTest.php index ec9e3fea..91ef10a5 100644 --- a/test/php/Nominatim/ParameterParserTest.php +++ b/test/php/Nominatim/ParameterParserTest.php @@ -45,59 +45,60 @@ class ParameterParserTest extends \PHPUnit_Framework_TestCase { $oParams = new ParameterParser([ 'int1' => '5', - 'int2' => 'a', - 'int3' => '-1', - 'int4' => '', - 'int5' => 0 + 'int2' => '-1', + 'int3' => '', + 'int4' => 0 ]); $this->assertEquals(false, $oParams->getInt('non-exists')); $this->assertEquals(999, $oParams->getInt('non-exists', 999)); $this->assertEquals(5, $oParams->getInt('int1')); - try { - $this->assertEquals(false, $oParams->getInt('int2')); - } catch (Exception $e) { - $this->assertEquals($e->getMessage(), "Integer number expected for parameter 'int2'"); - } - $this->assertEquals(-1, $oParams->getInt('int3')); - $this->assertEquals(false, $oParams->getInt('int4')); - $this->assertEquals(false, $oParams->getInt('int5')); // FIXME: should be 0 instead? + $this->assertEquals(-1, $oParams->getInt('int2')); + $this->assertEquals(false, $oParams->getInt('int3')); + $this->assertEquals(false, $oParams->getInt('int4')); // FIXME: should be 0 instead? + } + + + public function testGetIntWithEmpytString() + { + $this->setExpectedException(Exception::class, "Integer number expected for parameter 'int5'"); + (new ParameterParser(['int5' => 'a']))->getInt('int5'); } public function testGetFloat() { + $oParams = new ParameterParser([ 'float1' => '1.0', 'float2' => '-5', - 'float3' => '-55.', - 'float4' => 'a', - 'float5' => '', - 'float6' => 0 + 'float3' => '', + 'float4' => 0 ]); $this->assertEquals(false, $oParams->getFloat('non-exists')); $this->assertEquals(999, $oParams->getFloat('non-exists', 999)); $this->assertEquals(1, $oParams->getFloat('float1')); $this->assertEquals(-5, $oParams->getFloat('float2')); - - try { - $this->assertEquals(false, $oParams->getFloat('float3')); - } catch (Exception $e) { - $this->assertEquals($e->getMessage(), "Floating-point number expected for parameter 'float3'"); - } - - try { - $this->assertEquals(false, $oParams->getFloat('float4')); - } catch (Exception $e) { - $this->assertEquals($e->getMessage(), "Floating-point number expected for parameter 'float4'"); - } - $this->assertEquals(false, $oParams->getFloat('float5')); - $this->assertEquals(false, $oParams->getFloat('float6')); // FIXME: should be 0 instead? + $this->assertEquals(false, $oParams->getFloat('float3')); + $this->assertEquals(false, $oParams->getFloat('float4')); // FIXME: should be 0 instead? } + public function testGetFloatWithString() + { + $this->setExpectedException(Exception::class, "Floating-point number expected for parameter 'float5'"); + (new ParameterParser(['float5' => 'a']))->getFloat('float5'); + } + + + public function testGetFloatWithInvalidNumber() + { + $this->setExpectedException(Exception::class, "Floating-point number expected for parameter 'float6'"); + (new ParameterParser(['float6' => '-55.']))->getFloat('float6'); + } + public function testGetString() { @@ -115,14 +116,12 @@ class ParameterParserTest extends \PHPUnit_Framework_TestCase } - public function testGetSet() { $oParams = new ParameterParser([ 'val1' => 'foo', - 'val2' => 'FOO', - 'val3' => '', - 'val4' => 0 + 'val2' => '', + 'val3' => 0 ]); $this->assertEquals(false, $oParams->getSet('non-exists', ['foo', 'bar'])); @@ -130,13 +129,15 @@ class ParameterParserTest extends \PHPUnit_Framework_TestCase $this->assertEquals('default', $oParams->getSet('non-exists', ['foo', 'bar'], 'default')); $this->assertEquals('foo', $oParams->getSet('val1', ['foo', 'bar'])); - try { - $this->assertEquals(false, $oParams->getSet('val2', ['foo', 'bar'])); - } catch (Exception $e) { - $this->assertEquals($e->getMessage(), "Parameter 'val2' must be one of: foo, bar"); - } + $this->assertEquals(false, $oParams->getSet('val2', ['foo', 'bar'])); $this->assertEquals(false, $oParams->getSet('val3', ['foo', 'bar'])); - $this->assertEquals(false, $oParams->getSet('val4', ['foo', 'bar'])); + } + + + public function testGetSetWithValueNotInSet() + { + $this->setExpectedException(Exception::class, "Parameter 'val4' must be one of: foo, bar"); + (new ParameterParser(['val4' => 'faz']))->getSet('val4', ['foo', 'bar']); } @@ -158,6 +159,7 @@ class ParameterParserTest extends \PHPUnit_Framework_TestCase $this->assertEquals(false, $oParams->getStringList('list4')); } + public function testGetPreferredLanguages() { $oParams = new ParameterParser(['accept-language' => '']); From d9cd8c6fff18b763a8d404ae203f2723ab65aa4b Mon Sep 17 00:00:00 2001 From: Marc Tobias Metten Date: Wed, 28 Feb 2018 23:22:45 +0100 Subject: [PATCH 3/7] use assertSame to check array order, 0 vs false --- test/php/Nominatim/ParameterParserTest.php | 160 ++++++++++----------- 1 file changed, 80 insertions(+), 80 deletions(-) diff --git a/test/php/Nominatim/ParameterParserTest.php b/test/php/Nominatim/ParameterParserTest.php index 91ef10a5..51b18139 100644 --- a/test/php/Nominatim/ParameterParserTest.php +++ b/test/php/Nominatim/ParameterParserTest.php @@ -31,13 +31,13 @@ class ParameterParserTest extends \PHPUnit_Framework_TestCase 'bool5' => '' ]); - $this->assertEquals(false, $oParams->getBool('non-exists')); - $this->assertEquals(true, $oParams->getBool('non-exists', true)); - $this->assertEquals(true, $oParams->getBool('bool1')); - $this->assertEquals(false, $oParams->getBool('bool2')); - $this->assertEquals(true, $oParams->getBool('bool3')); - $this->assertEquals(true, $oParams->getBool('bool4')); - $this->assertEquals(false, $oParams->getBool('bool5')); + $this->assertSame(false, $oParams->getBool('non-exists')); + $this->assertSame(true, $oParams->getBool('non-exists', true)); + $this->assertSame(true, $oParams->getBool('bool1')); + $this->assertSame(false, $oParams->getBool('bool2')); + $this->assertSame(true, $oParams->getBool('bool3')); + $this->assertSame(true, $oParams->getBool('bool4')); + $this->assertSame(false, $oParams->getBool('bool5')); } @@ -50,13 +50,13 @@ class ParameterParserTest extends \PHPUnit_Framework_TestCase 'int4' => 0 ]); - $this->assertEquals(false, $oParams->getInt('non-exists')); - $this->assertEquals(999, $oParams->getInt('non-exists', 999)); - $this->assertEquals(5, $oParams->getInt('int1')); + $this->assertSame(false, $oParams->getInt('non-exists')); + $this->assertSame(999, $oParams->getInt('non-exists', 999)); + $this->assertSame(5, $oParams->getInt('int1')); - $this->assertEquals(-1, $oParams->getInt('int2')); - $this->assertEquals(false, $oParams->getInt('int3')); - $this->assertEquals(false, $oParams->getInt('int4')); // FIXME: should be 0 instead? + $this->assertSame(-1, $oParams->getInt('int2')); + $this->assertSame(false, $oParams->getInt('int3')); // FIXME: should be 0 instead? + $this->assertSame(0, $oParams->getInt('int4')); } @@ -77,12 +77,12 @@ class ParameterParserTest extends \PHPUnit_Framework_TestCase 'float4' => 0 ]); - $this->assertEquals(false, $oParams->getFloat('non-exists')); - $this->assertEquals(999, $oParams->getFloat('non-exists', 999)); - $this->assertEquals(1, $oParams->getFloat('float1')); - $this->assertEquals(-5, $oParams->getFloat('float2')); - $this->assertEquals(false, $oParams->getFloat('float3')); - $this->assertEquals(false, $oParams->getFloat('float4')); // FIXME: should be 0 instead? + $this->assertSame(false, $oParams->getFloat('non-exists')); + $this->assertSame(999, $oParams->getFloat('non-exists', 999)); + $this->assertSame(1.0, $oParams->getFloat('float1')); + $this->assertSame(-5.0, $oParams->getFloat('float2')); + $this->assertSame(false, $oParams->getFloat('float3')); // FIXME: should be 0 instead? + $this->assertSame(0.0, $oParams->getFloat('float4')); } @@ -108,11 +108,11 @@ class ParameterParserTest extends \PHPUnit_Framework_TestCase 'str3' => '0' ]); - $this->assertEquals(false, $oParams->getString('non-exists')); - $this->assertEquals('default', $oParams->getString('non-exists', 'default')); - $this->assertEquals('abc', $oParams->getString('str1')); - $this->assertEquals(false, $oParams->getStringList('str2')); - $this->assertEquals(false, $oParams->getStringList('str3')); // FIXME: should be 0 instead? + $this->assertSame(false, $oParams->getString('non-exists')); + $this->assertSame('default', $oParams->getString('non-exists', 'default')); + $this->assertSame('abc', $oParams->getString('str1')); + $this->assertSame(false, $oParams->getStringList('str2')); + $this->assertSame(false, $oParams->getStringList('str3')); // FIXME: should be 0 instead? } @@ -124,13 +124,13 @@ class ParameterParserTest extends \PHPUnit_Framework_TestCase 'val3' => 0 ]); - $this->assertEquals(false, $oParams->getSet('non-exists', ['foo', 'bar'])); + $this->assertSame(false, $oParams->getSet('non-exists', ['foo', 'bar'])); // FIXME: unclear if the default value has to be part of the set - $this->assertEquals('default', $oParams->getSet('non-exists', ['foo', 'bar'], 'default')); - $this->assertEquals('foo', $oParams->getSet('val1', ['foo', 'bar'])); + $this->assertSame('default', $oParams->getSet('non-exists', ['foo', 'bar'], 'default')); + $this->assertSame('foo', $oParams->getSet('val1', ['foo', 'bar'])); - $this->assertEquals(false, $oParams->getSet('val2', ['foo', 'bar'])); - $this->assertEquals(false, $oParams->getSet('val3', ['foo', 'bar'])); + $this->assertSame(false, $oParams->getSet('val2', ['foo', 'bar'])); + $this->assertSame(0, $oParams->getSet('val3', ['foo', 'bar'])); } @@ -150,67 +150,67 @@ class ParameterParserTest extends \PHPUnit_Framework_TestCase 'list4' => '0' ]); - $this->assertEquals(false, $oParams->getStringList('non-exists')); - $this->assertEquals(['a', 'b'], $oParams->getStringList('non-exists', ['a', 'b'])); + $this->assertSame(false, $oParams->getStringList('non-exists')); + $this->assertSame(['a', 'b'], $oParams->getStringList('non-exists', ['a', 'b'])); // FIXME: unclear if empty string items should be removed - $this->assertEquals(['', 'a', 'b', 'c', '', 'c', 'd'], $oParams->getStringList('list1')); - $this->assertEquals(['a'], $oParams->getStringList('list2')); - $this->assertEquals(false, $oParams->getStringList('list3')); - $this->assertEquals(false, $oParams->getStringList('list4')); + $this->assertSame(['', 'a', 'b', 'c', '', 'c', 'd'], $oParams->getStringList('list1')); + $this->assertSame(['a'], $oParams->getStringList('list2')); + $this->assertSame(false, $oParams->getStringList('list3')); + $this->assertSame(false, $oParams->getStringList('list4')); } public function testGetPreferredLanguages() { $oParams = new ParameterParser(['accept-language' => '']); - $this->assertEquals([ - 'brand' => 'brand', - 'ref' => 'ref', - 'type' => 'type', - 'name' => 'name', - 'name:default' => 'name:default', - 'short_name' => 'short_name', - 'short_name:default' => 'short_name:default', - 'official_name' => 'official_name', - 'official_name:default' => 'official_name:default', - ], $oParams->getPreferredLanguages('default')); + $this->assertSame([ + 'short_name:default' => 'short_name:default', + 'name:default' => 'name:default', + 'short_name' => 'short_name', + 'name' => 'name', + 'brand' => 'brand', + 'official_name:default' => 'official_name:default', + 'official_name' => 'official_name', + 'ref' => 'ref', + 'type' => 'type' + ], $oParams->getPreferredLanguages('default')); $oParams = new ParameterParser(['accept-language' => 'de,en']); - $this->assertEquals([ - 'brand' => 'brand', - 'ref' => 'ref', - 'type' => 'type', - 'name' => 'name', - 'name:de' => 'name:de', - 'name:en' => 'name:en', - 'short_name' => 'short_name', - 'short_name:de' => 'short_name:de', - 'short_name:en' => 'short_name:en', - 'official_name' => 'official_name', - 'official_name:de' => 'official_name:de', - 'official_name:en' => 'official_name:en', - ], $oParams->getPreferredLanguages('default')); + $this->assertSame([ + 'short_name:de' => 'short_name:de', + 'name:de' => 'name:de', + 'short_name:en' => 'short_name:en', + 'name:en' => 'name:en', + 'short_name' => 'short_name', + 'name' => 'name', + 'brand' => 'brand', + 'official_name:de' => 'official_name:de', + 'official_name:en' => 'official_name:en', + 'official_name' => 'official_name', + 'ref' => 'ref', + 'type' => 'type' + ], $oParams->getPreferredLanguages('default')); $oParams = new ParameterParser(['accept-language' => 'fr-ca,fr;q=0.8,en-ca;q=0.5,en;q=0.3']); - $this->assertEquals([ - 'short_name:fr-ca' => 'short_name:fr-ca', - 'name:fr-ca' => 'name:fr-ca', - 'short_name:fr' => 'short_name:fr', - 'name:fr' => 'name:fr', - 'short_name:en-ca' => 'short_name:en-ca', - 'name:en-ca' => 'name:en-ca', - 'short_name:en' => 'short_name:en', - 'name:en' => 'name:en', - 'short_name' => 'short_name', - 'name' => 'name', - 'brand' => 'brand', - 'official_name:fr-ca' => 'official_name:fr-ca', - 'official_name:fr' => 'official_name:fr', - 'official_name:en-ca' => 'official_name:en-ca', - 'official_name:en' => 'official_name:en', - 'official_name' => 'official_name', - 'ref' => 'ref', - 'type' => 'type', - ], $oParams->getPreferredLanguages('default')); + $this->assertSame([ + 'short_name:fr-ca' => 'short_name:fr-ca', + 'name:fr-ca' => 'name:fr-ca', + 'short_name:fr' => 'short_name:fr', + 'name:fr' => 'name:fr', + 'short_name:en-ca' => 'short_name:en-ca', + 'name:en-ca' => 'name:en-ca', + 'short_name:en' => 'short_name:en', + 'name:en' => 'name:en', + 'short_name' => 'short_name', + 'name' => 'name', + 'brand' => 'brand', + 'official_name:fr-ca' => 'official_name:fr-ca', + 'official_name:fr' => 'official_name:fr', + 'official_name:en-ca' => 'official_name:en-ca', + 'official_name:en' => 'official_name:en', + 'official_name' => 'official_name', + 'ref' => 'ref', + 'type' => 'type', + ], $oParams->getPreferredLanguages('default')); } } From 123a3c0347226b9b968fe75960fb660d7b6dda15 Mon Sep 17 00:00:00 2001 From: marc tobias Date: Tue, 6 Mar 2018 13:33:19 +0100 Subject: [PATCH 4/7] ParameterParser: getInt with empty string value throws exception --- lib/ParameterParser.php | 2 +- test/php/Nominatim/ParameterParserTest.php | 20 ++++++++++---------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/ParameterParser.php b/lib/ParameterParser.php index 2eed1629..7d8d5dc8 100644 --- a/lib/ParameterParser.php +++ b/lib/ParameterParser.php @@ -23,7 +23,7 @@ class ParameterParser public function getInt($sName, $bDefault = false) { - if (!isset($this->aParams[$sName]) || strlen($this->aParams[$sName]) == 0) { + if (!isset($this->aParams[$sName])) { return $bDefault; } diff --git a/test/php/Nominatim/ParameterParserTest.php b/test/php/Nominatim/ParameterParserTest.php index 51b18139..ddc06851 100644 --- a/test/php/Nominatim/ParameterParserTest.php +++ b/test/php/Nominatim/ParameterParserTest.php @@ -16,11 +16,6 @@ class ParameterParserTest extends \PHPUnit_Framework_TestCase { - protected function setUp() - { - } - - public function testGetBool() { $oParams = new ParameterParser([ @@ -46,8 +41,7 @@ class ParameterParserTest extends \PHPUnit_Framework_TestCase $oParams = new ParameterParser([ 'int1' => '5', 'int2' => '-1', - 'int3' => '', - 'int4' => 0 + 'int3' => 0 ]); $this->assertSame(false, $oParams->getInt('non-exists')); @@ -55,15 +49,21 @@ class ParameterParserTest extends \PHPUnit_Framework_TestCase $this->assertSame(5, $oParams->getInt('int1')); $this->assertSame(-1, $oParams->getInt('int2')); - $this->assertSame(false, $oParams->getInt('int3')); // FIXME: should be 0 instead? - $this->assertSame(0, $oParams->getInt('int4')); + $this->assertSame(0, $oParams->getInt('int3')); + } + + + public function testGetIntWithNonNumber() + { + $this->setExpectedException(Exception::class, "Integer number expected for parameter 'int4'"); + (new ParameterParser(['int4' => 'a']))->getInt('int4'); } public function testGetIntWithEmpytString() { $this->setExpectedException(Exception::class, "Integer number expected for parameter 'int5'"); - (new ParameterParser(['int5' => 'a']))->getInt('int5'); + (new ParameterParser(['int5' => '']))->getInt('int5'); } From 47258f40ea77b9d73c7bfcca1a3489d9adeb30f3 Mon Sep 17 00:00:00 2001 From: marc tobias Date: Tue, 6 Mar 2018 13:35:27 +0100 Subject: [PATCH 5/7] ParameterParser: getFloat with empty string value throws exception --- lib/ParameterParser.php | 2 +- test/php/Nominatim/ParameterParserTest.php | 13 ++++++++----- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/lib/ParameterParser.php b/lib/ParameterParser.php index 7d8d5dc8..feddc3aa 100644 --- a/lib/ParameterParser.php +++ b/lib/ParameterParser.php @@ -36,7 +36,7 @@ class ParameterParser public function getFloat($sName, $bDefault = false) { - if (!isset($this->aParams[$sName]) || strlen($this->aParams[$sName]) == 0) { + if (!isset($this->aParams[$sName])) { return $bDefault; } diff --git a/test/php/Nominatim/ParameterParserTest.php b/test/php/Nominatim/ParameterParserTest.php index ddc06851..78739534 100644 --- a/test/php/Nominatim/ParameterParserTest.php +++ b/test/php/Nominatim/ParameterParserTest.php @@ -73,20 +73,23 @@ class ParameterParserTest extends \PHPUnit_Framework_TestCase $oParams = new ParameterParser([ 'float1' => '1.0', 'float2' => '-5', - 'float3' => '', - 'float4' => 0 + 'float3' => 0 ]); $this->assertSame(false, $oParams->getFloat('non-exists')); $this->assertSame(999, $oParams->getFloat('non-exists', 999)); $this->assertSame(1.0, $oParams->getFloat('float1')); $this->assertSame(-5.0, $oParams->getFloat('float2')); - $this->assertSame(false, $oParams->getFloat('float3')); // FIXME: should be 0 instead? - $this->assertSame(0.0, $oParams->getFloat('float4')); + $this->assertSame(0.0, $oParams->getFloat('float3')); } + public function testGetFloatWithEmptyString() + { + $this->setExpectedException(Exception::class, "Floating-point number expected for parameter 'float4'"); + (new ParameterParser(['float4' => '']))->getFloat('float4'); + } - public function testGetFloatWithString() + public function testGetFloatWithTextString() { $this->setExpectedException(Exception::class, "Floating-point number expected for parameter 'float5'"); (new ParameterParser(['float5' => 'a']))->getFloat('float5'); From 3ef4c4fbe7da4b8feb9566d3455badffec55e151 Mon Sep 17 00:00:00 2001 From: marc tobias Date: Tue, 6 Mar 2018 14:51:48 +0100 Subject: [PATCH 6/7] ParameterParser: getStringList removes empty strings --- lib/ParameterParser.php | 3 ++- test/php/Nominatim/ParameterParserTest.php | 5 ++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/ParameterParser.php b/lib/ParameterParser.php index feddc3aa..c9a97c25 100644 --- a/lib/ParameterParser.php +++ b/lib/ParameterParser.php @@ -74,7 +74,8 @@ class ParameterParser $sValue = $this->getString($sName); if ($sValue) { - return explode(',', $sValue); + // removes all NULL, FALSE and Empty Strings but leaves 0 (zero) values + return array_values(array_filter(explode(',', $sValue), 'strlen')); } return $aDefault; diff --git a/test/php/Nominatim/ParameterParserTest.php b/test/php/Nominatim/ParameterParserTest.php index 78739534..105f9d34 100644 --- a/test/php/Nominatim/ParameterParserTest.php +++ b/test/php/Nominatim/ParameterParserTest.php @@ -115,7 +115,7 @@ class ParameterParserTest extends \PHPUnit_Framework_TestCase $this->assertSame('default', $oParams->getString('non-exists', 'default')); $this->assertSame('abc', $oParams->getString('str1')); $this->assertSame(false, $oParams->getStringList('str2')); - $this->assertSame(false, $oParams->getStringList('str3')); // FIXME: should be 0 instead? + $this->assertSame(false, $oParams->getStringList('str3')); // sadly PHP magic treats 0 as false when returned } @@ -155,8 +155,7 @@ class ParameterParserTest extends \PHPUnit_Framework_TestCase $this->assertSame(false, $oParams->getStringList('non-exists')); $this->assertSame(['a', 'b'], $oParams->getStringList('non-exists', ['a', 'b'])); - // FIXME: unclear if empty string items should be removed - $this->assertSame(['', 'a', 'b', 'c', '', 'c', 'd'], $oParams->getStringList('list1')); + $this->assertSame(['a', 'b', 'c', 'c', 'd'], $oParams->getStringList('list1')); $this->assertSame(['a'], $oParams->getStringList('list2')); $this->assertSame(false, $oParams->getStringList('list3')); $this->assertSame(false, $oParams->getStringList('list4')); From 7fd46dcee923b507d366004148eb3077387560b2 Mon Sep 17 00:00:00 2001 From: marc tobias Date: Tue, 6 Mar 2018 14:53:23 +0100 Subject: [PATCH 7/7] ParameterParser: getSet default value doesnt have to be part of the set --- test/php/Nominatim/ParameterParserTest.php | 1 - 1 file changed, 1 deletion(-) diff --git a/test/php/Nominatim/ParameterParserTest.php b/test/php/Nominatim/ParameterParserTest.php index 105f9d34..4265cffb 100644 --- a/test/php/Nominatim/ParameterParserTest.php +++ b/test/php/Nominatim/ParameterParserTest.php @@ -128,7 +128,6 @@ class ParameterParserTest extends \PHPUnit_Framework_TestCase ]); $this->assertSame(false, $oParams->getSet('non-exists', ['foo', 'bar'])); - // FIXME: unclear if the default value has to be part of the set $this->assertSame('default', $oParams->getSet('non-exists', ['foo', 'bar'], 'default')); $this->assertSame('foo', $oParams->getSet('val1', ['foo', 'bar']));