From 96ebf936e489ca03c61d406b151e5e8b78250e61 Mon Sep 17 00:00:00 2001 From: "J. King" Date: Thu, 28 Sep 2017 09:01:43 -0400 Subject: [PATCH] CS fixes --- lib/Database.php | 16 +++++++++++----- lib/Db/SQLite3/Driver.php | 2 +- lib/Misc/Context.php | 2 +- lib/Misc/ValueInfo.php | 10 +++++----- lib/REST/AbstractHandler.php | 2 +- lib/REST/NextCloudNews/V1_2.php | 4 ++-- tests/Misc/TestValueInfo.php | 2 +- tests/REST/NextCloudNews/TestNCNV1_2.php | 8 ++++---- tests/lib/Misc/StrClass.php | 2 +- 9 files changed, 27 insertions(+), 21 deletions(-) diff --git a/lib/Database.php b/lib/Database.php index 3a97177..757188d 100644 --- a/lib/Database.php +++ b/lib/Database.php @@ -232,7 +232,7 @@ class Database { // normalize folder's parent, if there is one $parent = array_key_exists("parent", $data) ? $this->folderValidateId($user, $data['parent'])['id'] : null; // validate the folder name and parent (if specified); this also checks for duplicates - $name = array_key_exists("name", $data) ? $data['name'] : ""; + $name = array_key_exists("name", $data) ? $data['name'] : ""; $this->folderValidateName($name, true, $parent); // actually perform the insert return $this->db->prepare("INSERT INTO arsse_folders(owner,parent,name) values(?,?,?)", "str", "int", "str")->run($user, $parent, $name)->lastId(); @@ -322,7 +322,7 @@ class Database { protected function folderValidateId(string $user, $id = null, bool $subject = false): array { // if the specified ID is not a non-negative integer (or null), this will always fail - if(!ValueInfo::id($id, true)) { + if (!ValueInfo::id($id, true)) { throw new Db\ExceptionInput("typeViolation", ["action" => $this->caller(), "field" => "folder", 'type' => "int >= 0"]); } // if a null or zero ID is specified this is a no-op @@ -358,7 +358,9 @@ class Database { if ($id==$parent) { throw new Db\ExceptionInput("circularDependence", $errData); } - // make sure both that the prospective parent exists, and that the it is not one of its children (a circular dependence) + // make sure both that the prospective parent exists, and that the it is not one of its children (a circular dependence); + // also make sure that a folder with the same prospective name and parent does not already exist: if the parent is null, + // SQL will happily accept duplicates (null is not unique), so we must do this check ourselves $p = $this->db->prepare( "WITH RECURSIVE target as (select ? as user, ? as source, ? as dest, ? as rename), @@ -368,7 +370,7 @@ class Database { ((select dest from target) is null or exists(select id from arsse_folders join target on owner is user and id is dest)) as extant, not exists(select id from folders where id is (select dest from target)) as valid, not exists(select id from arsse_folders join target on parent is dest and name is coalesce((select rename from target),(select name from arsse_folders join target on id is source))) as available - ", "str", "int", "int","str" + ", "str", "int", "int", "str" )->run($user, $id, $parent, $name)->getRow(); if (!$p['extant']) { // if the parent doesn't exist or doesn't below to the user, throw an exception @@ -377,6 +379,7 @@ class Database { // if using the desired parent would create a circular dependence, throw a different exception throw new Db\ExceptionInput("circularDependence", $errData); } elseif (!$p['available']) { + // if a folder with the same parent and name already exists, throw another different exception throw new Db\ExceptionInput("constraintViolation", ["action" => $this->caller(), "field" => (is_null($name) ? "parent" : "name")]); } return $parent; @@ -390,7 +393,10 @@ class Database { throw new Db\ExceptionInput("whitespace", ["action" => $this->caller(), "field" => "name"]); } elseif (!($info & ValueInfo::VALID)) { throw new Db\ExceptionInput("typeViolation", ["action" => $this->caller(), "field" => "name", 'type' => "string"]); - } elseif($checkDuplicates) { + } elseif ($checkDuplicates) { + // make sure that a folder with the same prospective name and parent does not already exist: if the parent is null, + // SQL will happily accept duplicates (null is not unique), so we must do this check ourselves + $parent = $parent ? $parent : null; if ($this->db->prepare("SELECT exists(select id from arsse_folders where parent is ? and name is ?)", "int", "str")->run($parent, $name)->getValue()) { throw new Db\ExceptionInput("constraintViolation", ["action" => $this->caller(), "field" => "name"]); } diff --git a/lib/Db/SQLite3/Driver.php b/lib/Db/SQLite3/Driver.php index b47339e..d65ff2e 100644 --- a/lib/Db/SQLite3/Driver.php +++ b/lib/Db/SQLite3/Driver.php @@ -20,7 +20,7 @@ class Driver extends \JKingWeb\Arsse\Db\AbstractDriver { // check to make sure required extension is loaded if (!class_exists("SQLite3")) { throw new Exception("extMissing", self::driverName()); // @codeCoverageIgnore - } + } // if no database file is specified in the configuration, use a suitable default $dbFile = Arsse::$conf->dbSQLite3File ?? \JKingWeb\Arsse\BASE."arsse.db"; $mode = \SQLITE3_OPEN_READWRITE | \SQLITE3_OPEN_CREATE; diff --git a/lib/Misc/Context.php b/lib/Misc/Context.php index 01e0528..ca37b3d 100644 --- a/lib/Misc/Context.php +++ b/lib/Misc/Context.php @@ -37,7 +37,7 @@ class Context { protected function cleanArray(array $spec): array { $spec = array_values($spec); for ($a = 0; $a < sizeof($spec); $a++) { - if(ValueInfo::id($spec[$a])) { + if (ValueInfo::id($spec[$a])) { $spec[$a] = (int) $spec[$a]; } else { $spec[$a] = 0; diff --git a/lib/Misc/ValueInfo.php b/lib/Misc/ValueInfo.php index dd784b1..040243d 100644 --- a/lib/Misc/ValueInfo.php +++ b/lib/Misc/ValueInfo.php @@ -13,7 +13,7 @@ class ValueInfo { const EMPTY = 1 << 2; const WHITE = 1 << 3; - static public function int($value): int { + public static function int($value): int { $out = 0; if (is_null($value)) { // check if the input is null @@ -40,7 +40,7 @@ class ValueInfo { // mark validity $out += self::VALID; // mark zeroness - if($value==0) { + if ($value==0) { $out += self::ZERO; } // mark negativeness @@ -50,7 +50,7 @@ class ValueInfo { return $out; } - static public function str($value): int { + public static function str($value): int { $out = 0; // check if the input is null if (is_null($value)) { @@ -75,7 +75,7 @@ class ValueInfo { return $out; } - static public function id($value, bool $allowNull = false): bool { + public static function id($value, bool $allowNull = false): bool { $info = self::int($value); if ($allowNull && ($info & self::NULL)) { // null (and allowed) return true; @@ -89,4 +89,4 @@ class ValueInfo { return true; } } -} \ No newline at end of file +} diff --git a/lib/REST/AbstractHandler.php b/lib/REST/AbstractHandler.php index fbc42d4..8c0988d 100644 --- a/lib/REST/AbstractHandler.php +++ b/lib/REST/AbstractHandler.php @@ -50,7 +50,7 @@ abstract class AbstractHandler implements Handler { } break; case "string": - if(is_bool($value)) { + if (is_bool($value)) { $out[$key] = var_export($value, true); } elseif (!is_scalar($value)) { break; diff --git a/lib/REST/NextCloudNews/V1_2.php b/lib/REST/NextCloudNews/V1_2.php index caf3d40..8ece274 100644 --- a/lib/REST/NextCloudNews/V1_2.php +++ b/lib/REST/NextCloudNews/V1_2.php @@ -79,7 +79,7 @@ class V1_2 extends \JKingWeb\Arsse\REST\AbstractHandler { // dispatch try { return $this->$func($req->paths, $data); - // @codeCoverageIgnoreStart + // @codeCoverageIgnoreStart } catch (Exception $e) { // if there was a REST exception return 400 return new Response(400); @@ -340,7 +340,7 @@ class V1_2 extends \JKingWeb\Arsse\REST\AbstractHandler { switch ($e->getCode()) { case 10239: // feed does not exist return new Response(404); - case 10237: // feed ID invalid + case 10237: // feed ID invalid return new Response(422); default: // other errors related to input return new Response(400); // @codeCoverageIgnore diff --git a/tests/Misc/TestValueInfo.php b/tests/Misc/TestValueInfo.php index 6fac75c..1a75099 100644 --- a/tests/Misc/TestValueInfo.php +++ b/tests/Misc/TestValueInfo.php @@ -197,7 +197,7 @@ class TestValueInfo extends Test\AbstractTest { ]; foreach ($tests as $test) { list($value, $exp, $expNull) = $test; - $this->assertSame($exp, I::id($value), "Non-null test failed for value: ".var_export($value, true)); + $this->assertSame($exp, I::id($value), "Non-null test failed for value: ".var_export($value, true)); $this->assertSame($expNull, I::id($value, true), "Null test failed for value: ".var_export($value, true)); } } diff --git a/tests/REST/NextCloudNews/TestNCNV1_2.php b/tests/REST/NextCloudNews/TestNCNV1_2.php index 2466599..713267b 100644 --- a/tests/REST/NextCloudNews/TestNCNV1_2.php +++ b/tests/REST/NextCloudNews/TestNCNV1_2.php @@ -500,14 +500,14 @@ class TestNCNV1_2 extends Test\AbstractTest { Phake::when(Arsse::$db)->subscriptionAdd(Arsse::$user->id, "http://example.com/news.atom")->thenReturn(2112)->thenThrow(new ExceptionInput("constraintViolation")); // error on the second call Phake::when(Arsse::$db)->subscriptionAdd(Arsse::$user->id, "http://example.org/news.atom")->thenReturn(42)->thenThrow(new ExceptionInput("constraintViolation")); // error on the second call Phake::when(Arsse::$db)->subscriptionPropertiesGet(Arsse::$user->id, 2112)->thenReturn($this->feeds['db'][0]); - Phake::when(Arsse::$db)->subscriptionPropertiesGet(Arsse::$user->id, 42)->thenReturn($this->feeds['db'][1]); - Phake::when(Arsse::$db)->subscriptionPropertiesGet(Arsse::$user->id, 47)->thenReturn($this->feeds['db'][2]); + Phake::when(Arsse::$db)->subscriptionPropertiesGet(Arsse::$user->id, 42)->thenReturn($this->feeds['db'][1]); + Phake::when(Arsse::$db)->subscriptionPropertiesGet(Arsse::$user->id, 47)->thenReturn($this->feeds['db'][2]); Phake::when(Arsse::$db)->editionLatest(Arsse::$user->id, (new Context)->subscription(2112))->thenReturn(0); Phake::when(Arsse::$db)->editionLatest(Arsse::$user->id, (new Context)->subscription(42))->thenReturn(4758915); Phake::when(Arsse::$db)->editionLatest(Arsse::$user->id, (new Context)->subscription(47))->thenReturn(2112); Phake::when(Arsse::$db)->subscriptionPropertiesSet(Arsse::$user->id, 2112, ['folder' => 3])->thenThrow(new ExceptionInput("idMissing")); // folder ID 3 does not exist - Phake::when(Arsse::$db)->subscriptionPropertiesSet(Arsse::$user->id, 42, ['folder' => 8])->thenReturn(true); - Phake::when(Arsse::$db)->subscriptionPropertiesSet(Arsse::$user->id, 47, ['folder' => -1])->thenThrow(new ExceptionInput("typeViolation")); // folder ID -1 is invalid + Phake::when(Arsse::$db)->subscriptionPropertiesSet(Arsse::$user->id, 42, ['folder' => 8])->thenReturn(true); + Phake::when(Arsse::$db)->subscriptionPropertiesSet(Arsse::$user->id, 47, ['folder' => -1])->thenThrow(new ExceptionInput("typeViolation")); // folder ID -1 is invalid // set up a mock for a bad feed which succeeds the second time Phake::when(Arsse::$db)->subscriptionAdd(Arsse::$user->id, "http://example.net/news.atom")->thenThrow(new \JKingWeb\Arsse\Feed\Exception("http://example.net/news.atom", new \PicoFeed\Client\InvalidUrlException()))->thenReturn(47); // add the subscriptions diff --git a/tests/lib/Misc/StrClass.php b/tests/lib/Misc/StrClass.php index d93f2c5..905194f 100644 --- a/tests/lib/Misc/StrClass.php +++ b/tests/lib/Misc/StrClass.php @@ -12,4 +12,4 @@ class StrClass { public function __toString() { return $this->str; } -} \ No newline at end of file +}