From a0c6f2e5e071a287717e58f32c5b3cf6e219f321 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 7 Jul 2015 15:37:56 +0200 Subject: [PATCH] Use the decorator pattern --- core/command/app/checkcode.php | 14 ++- lib/private/app/codechecker/codechecker.php | 10 +- ...precationlist.php => deprecationcheck.php} | 33 ++++-- lib/private/app/codechecker/emptycheck.php | 65 +++++++++++ .../{ichecklist.php => icheck.php} | 2 +- lib/private/app/codechecker/nodevisitor.php | 4 +- lib/private/app/codechecker/privatecheck.php | 104 ++++++++++++++++++ ...vatelist.php => strongcomparisoncheck.php} | 59 +++------- tests/lib/app/codechecker/codecheckertest.php | 16 +-- ...nlisttest.php => deprecationchecktest.php} | 24 ++-- tests/lib/app/codechecker/mock/testlist.php | 15 ++- tests/lib/app/codechecker/nodevisitortest.php | 10 +- .../codechecker/strongcomparisonchecktest.php | 70 ++++++++++++ 13 files changed, 334 insertions(+), 92 deletions(-) rename lib/private/app/codechecker/{deprecationlist.php => deprecationcheck.php} (88%) create mode 100644 lib/private/app/codechecker/emptycheck.php rename lib/private/app/codechecker/{ichecklist.php => icheck.php} (98%) create mode 100644 lib/private/app/codechecker/privatecheck.php rename lib/private/app/codechecker/{privatelist.php => strongcomparisoncheck.php} (60%) rename tests/lib/app/codechecker/{deprecationlisttest.php => deprecationchecktest.php} (70%) create mode 100644 tests/lib/app/codechecker/strongcomparisonchecktest.php diff --git a/core/command/app/checkcode.php b/core/command/app/checkcode.php index a533ce767f..3f4fb95868 100644 --- a/core/command/app/checkcode.php +++ b/core/command/app/checkcode.php @@ -24,8 +24,10 @@ namespace OC\Core\Command\App; use OC\App\CodeChecker\CodeChecker; -use OC\App\CodeChecker\DeprecationList; -use OC\App\CodeChecker\PrivateList; +use OC\App\CodeChecker\DeprecationCheck; +use OC\App\CodeChecker\EmptyCheck; +use OC\App\CodeChecker\PrivateCheck; +use OC\App\CodeChecker\StrongComparisonCheck; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Input\InputArgument; use Symfony\Component\Console\Input\InputInterface; @@ -52,12 +54,14 @@ class CheckCode extends Command { protected function execute(InputInterface $input, OutputInterface $output) { $appId = $input->getArgument('app-id'); + $checkList = new EmptyCheck(); if ($input->getOption('deprecated')) { - $list = new DeprecationList(); + $checkList = new DeprecationCheck($checkList); + $checkList = new StrongComparisonCheck($checkList); } else { - $list = new PrivateList(); + $checkList = new PrivateCheck($checkList); } - $codeChecker = new CodeChecker($list); + $codeChecker = new CodeChecker($checkList); $codeChecker->listen('CodeChecker', 'analyseFileBegin', function($params) use ($output) { if(OutputInterface::VERBOSITY_VERBOSE <= $output->getVerbosity()) { diff --git a/lib/private/app/codechecker/codechecker.php b/lib/private/app/codechecker/codechecker.php index b2ee9d6d71..ae16b15617 100644 --- a/lib/private/app/codechecker/codechecker.php +++ b/lib/private/app/codechecker/codechecker.php @@ -49,11 +49,11 @@ class CodeChecker extends BasicEmitter { /** @var Parser */ private $parser; - /** @var ICheckList */ - protected $list; + /** @var ICheck */ + protected $checkList; - public function __construct(ICheckList $list) { - $this->list = $list; + public function __construct(ICheck $checkList) { + $this->checkList = $checkList; $this->parser = new Parser(new Lexer); } @@ -114,7 +114,7 @@ class CodeChecker extends BasicEmitter { $code = file_get_contents($file); $statements = $this->parser->parse($code); - $visitor = new NodeVisitor($this->list); + $visitor = new NodeVisitor($this->checkList); $traverser = new NodeTraverser; $traverser->addVisitor($visitor); diff --git a/lib/private/app/codechecker/deprecationlist.php b/lib/private/app/codechecker/deprecationcheck.php similarity index 88% rename from lib/private/app/codechecker/deprecationlist.php rename to lib/private/app/codechecker/deprecationcheck.php index d0abc75a30..bac59e4dd7 100644 --- a/lib/private/app/codechecker/deprecationlist.php +++ b/lib/private/app/codechecker/deprecationcheck.php @@ -21,19 +21,30 @@ namespace OC\App\CodeChecker; -class DeprecationList implements ICheckList { +class DeprecationCheck implements ICheck { + /** @var ICheck */ + protected $check; + + /** + * @param ICheck $check + */ + public function __construct(ICheck $check) { + $this->check = $check; + } + /** * @return string */ public function getDescription() { - return 'deprecated'; + $innerDescription = $this->check->getDescription(); + return 'deprecated' . (($innerDescription === '') ? '' : ' ' . $innerDescription); } /** * @return array E.g.: `'ClassName' => 'oc version',` */ public function getClasses() { - return [ + return array_merge([ 'OCP\Config' => '8.0.0', 'OCP\Contacts' => '8.1.0', 'OCP\DB' => '8.1.0', @@ -41,14 +52,14 @@ class DeprecationList implements ICheckList { 'OCP\JSON' => '8.1.0', 'OCP\Response' => '8.1.0', 'OCP\AppFramework\IApi' => '8.0.0', - ]; + ], $this->check->getClasses()); } /** * @return array E.g.: `'ClassName::CONSTANT_NAME' => 'oc version',` */ public function getConstants() { - return [ + return array_merge([ 'OCP::PERMISSION_CREATE' => '8.0.0', 'OCP::PERMISSION_READ' => '8.0.0', 'OCP::PERMISSION_UPDATE' => '8.0.0', @@ -56,14 +67,14 @@ class DeprecationList implements ICheckList { 'OCP::PERMISSION_SHARE' => '8.0.0', 'OCP::PERMISSION_ALL' => '8.0.0', 'OCP::FILENAME_INVALID_CHARS' => '8.0.0', - ]; + ], $this->check->getConstants()); } /** * @return array E.g.: `'functionName' => 'oc version',` */ public function getFunctions() { - return [ + return array_merge([ 'OCP::image_path' => '8.0.0', 'OCP::mimetype_icon' => '8.0.0', 'OCP::preview_icon' => '8.0.0', @@ -72,14 +83,14 @@ class DeprecationList implements ICheckList { 'OCP::relative_modified_date' => '8.0.0', 'OCP::simple_file_size' => '8.0.0', 'OCP::html_select_options' => '8.0.0', - ]; + ], $this->check->getFunctions()); } /** * @return array E.g.: `'ClassName::methodName' => 'oc version',` */ public function getMethods() { - return [ + return array_merge([ 'OCP\App::register' => '8.1.0', 'OCP\App::addNavigationEntry' => '8.1.0', 'OCP\App::setActiveNavigationEntry' => '8.1.0', @@ -140,13 +151,13 @@ class DeprecationList implements ICheckList { 'OCP\Util::imagePath' => '8.1.0', 'OCP\Util::isValidFileName' => '8.1.0', 'OCP\Util::generateRandomBytes' => '8.1.0', - ]; + ], $this->check->getMethods()); } /** * @return bool */ public function checkStrongComparisons() { - return true; + return $this->check->checkStrongComparisons(); } } diff --git a/lib/private/app/codechecker/emptycheck.php b/lib/private/app/codechecker/emptycheck.php new file mode 100644 index 0000000000..78a0d30379 --- /dev/null +++ b/lib/private/app/codechecker/emptycheck.php @@ -0,0 +1,65 @@ + + * + * @copyright Copyright (c) 2015, ownCloud, Inc. + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License, version 3, + * along with this program. If not, see + * + */ +namespace OC\App\CodeChecker; + +class EmptyCheck implements ICheck { + /** + * @return string + */ + public function getDescription() { + return ''; + } + + /** + * @return array E.g.: `'ClassName' => 'oc version',` + */ + public function getClasses() { + return []; + } + + /** + * @return array E.g.: `'ClassName::CONSTANT_NAME' => 'oc version',` + */ + public function getConstants() { + return []; + } + + /** + * @return array E.g.: `'functionName' => 'oc version',` + */ + public function getFunctions() { + return []; + } + + /** + * @return array E.g.: `'ClassName::methodName' => 'oc version',` + */ + public function getMethods() { + return []; + } + + /** + * @return bool + */ + public function checkStrongComparisons() { + return false; + } +} diff --git a/lib/private/app/codechecker/ichecklist.php b/lib/private/app/codechecker/icheck.php similarity index 98% rename from lib/private/app/codechecker/ichecklist.php rename to lib/private/app/codechecker/icheck.php index c5cc82e6bb..7625a105b0 100644 --- a/lib/private/app/codechecker/ichecklist.php +++ b/lib/private/app/codechecker/icheck.php @@ -20,7 +20,7 @@ */ namespace OC\App\CodeChecker; -interface ICheckList { +interface ICheck { /** * @return string */ diff --git a/lib/private/app/codechecker/nodevisitor.php b/lib/private/app/codechecker/nodevisitor.php index 8ec4a0320f..80acb9f241 100644 --- a/lib/private/app/codechecker/nodevisitor.php +++ b/lib/private/app/codechecker/nodevisitor.php @@ -43,9 +43,9 @@ class NodeVisitor extends NodeVisitorAbstract { protected $errorMessages; /** - * @param ICheckList $list + * @param ICheck $list */ - public function __construct(ICheckList $list) { + public function __construct(ICheck $list) { $this->blackListDescription = $list->getDescription(); $this->blackListedClassNames = []; diff --git a/lib/private/app/codechecker/privatecheck.php b/lib/private/app/codechecker/privatecheck.php new file mode 100644 index 0000000000..e7497f3dbf --- /dev/null +++ b/lib/private/app/codechecker/privatecheck.php @@ -0,0 +1,104 @@ + + * @author Morris Jobke + * @author Thomas Müller + * + * @copyright Copyright (c) 2015, ownCloud, Inc. + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License, version 3, + * along with this program. If not, see + * + */ + +namespace OC\App\CodeChecker; + +class PrivateCheck implements ICheck { + /** @var ICheck */ + protected $check; + + /** + * @param ICheck $check + */ + public function __construct(ICheck $check) { + $this->check = $check; + } + + /** + * @return string + */ + public function getDescription() { + $innerDescription = $this->check->getDescription(); + return 'private' . (($innerDescription === '') ? '' : ' ' . $innerDescription); + } + + /** + * @return array + */ + public function getClasses() { + return array_merge([ + // classes replaced by the public api + 'OC_API' => '6.0.0', + 'OC_App' => '6.0.0', + 'OC_AppConfig' => '6.0.0', + 'OC_Avatar' => '6.0.0', + 'OC_BackgroundJob' => '6.0.0', + 'OC_Config' => '6.0.0', + 'OC_DB' => '6.0.0', + 'OC_Files' => '6.0.0', + 'OC_Helper' => '6.0.0', + 'OC_Hook' => '6.0.0', + 'OC_Image' => '6.0.0', + 'OC_JSON' => '6.0.0', + 'OC_L10N' => '6.0.0', + 'OC_Log' => '6.0.0', + 'OC_Mail' => '6.0.0', + 'OC_Preferences' => '6.0.0', + 'OC_Search_Provider' => '6.0.0', + 'OC_Search_Result' => '6.0.0', + 'OC_Request' => '6.0.0', + 'OC_Response' => '6.0.0', + 'OC_Template' => '6.0.0', + 'OC_User' => '6.0.0', + 'OC_Util' => '6.0.0', + ], $this->check->getClasses()); + } + + /** + * @return array + */ + public function getConstants() { + return $this->check->getConstants(); + } + + /** + * @return array + */ + public function getFunctions() { + return $this->check->getFunctions(); + } + + /** + * @return array + */ + public function getMethods() { + return $this->check->getMethods(); + } + + /** + * @return bool + */ + public function checkStrongComparisons() { + return $this->check->checkStrongComparisons(); + } +} diff --git a/lib/private/app/codechecker/privatelist.php b/lib/private/app/codechecker/strongcomparisoncheck.php similarity index 60% rename from lib/private/app/codechecker/privatelist.php rename to lib/private/app/codechecker/strongcomparisoncheck.php index e44926ada7..bfbde1acbe 100644 --- a/lib/private/app/codechecker/privatelist.php +++ b/lib/private/app/codechecker/strongcomparisoncheck.php @@ -23,83 +23,56 @@ namespace OC\App\CodeChecker; -use OC\Hooks\BasicEmitter; -use PhpParser\Lexer; -use PhpParser\Node; -use PhpParser\Node\Name; -use PhpParser\NodeTraverser; -use PhpParser\Parser; -use RecursiveCallbackFilterIterator; -use RecursiveDirectoryIterator; -use RecursiveIteratorIterator; -use RegexIterator; -use SplFileInfo; +class StrongComparisonCheck implements ICheck { + /** @var ICheck */ + protected $check; + + /** + * @param ICheck $check + */ + public function __construct(ICheck $check) { + $this->check = $check; + } -class PrivateList implements ICheckList { /** * @return string */ public function getDescription() { - return 'private'; + return $this->check->getDescription(); } /** * @return array */ public function getClasses() { - return [ - // classes replaced by the public api - 'OC_API', - 'OC_App', - 'OC_AppConfig', - 'OC_Avatar', - 'OC_BackgroundJob', - 'OC_Config', - 'OC_DB', - 'OC_Files', - 'OC_Helper', - 'OC_Hook', - 'OC_Image', - 'OC_JSON', - 'OC_L10N', - 'OC_Log', - 'OC_Mail', - 'OC_Preferences', - 'OC_Search_Provider', - 'OC_Search_Result', - 'OC_Request', - 'OC_Response', - 'OC_Template', - 'OC_User', - 'OC_Util', - ]; + return $this->check->getClasses(); } /** * @return array */ public function getConstants() { - return []; + return $this->check->getConstants(); } /** * @return array */ public function getFunctions() { - return []; + return $this->check->getFunctions(); } /** * @return array */ public function getMethods() { - return []; + return $this->check->getMethods(); } /** * @return bool */ public function checkStrongComparisons() { - return false; + return true; } } diff --git a/tests/lib/app/codechecker/codecheckertest.php b/tests/lib/app/codechecker/codecheckertest.php index 93bf0b32c5..cdbb7c17da 100644 --- a/tests/lib/app/codechecker/codecheckertest.php +++ b/tests/lib/app/codechecker/codecheckertest.php @@ -8,7 +8,9 @@ namespace Test\App\CodeChecker; -use OC; +use OC\App\CodeChecker\CodeChecker; +use OC\App\CodeChecker\EmptyCheck; +use OC\App\CodeChecker\PrivateCheck; use Test\TestCase; class CodeCheckerTest extends TestCase { @@ -20,10 +22,10 @@ class CodeCheckerTest extends TestCase { * @param string $fileToVerify */ public function testFindInvalidUsage($expectedErrorToken, $expectedErrorCode, $fileToVerify) { - $checker = new OC\App\CodeChecker\CodeChecker( - new OC\App\CodeChecker\PrivateList() + $checker = new CodeChecker( + new PrivateCheck(new EmptyCheck()) ); - $errors = $checker->analyseFile(OC::$SERVERROOT . "/tests/data/app/code-checker/$fileToVerify"); + $errors = $checker->analyseFile(\OC::$SERVERROOT . "/tests/data/app/code-checker/$fileToVerify"); $this->assertEquals(1, count($errors)); $this->assertEquals($expectedErrorCode, $errors[0]['errorCode']); @@ -46,10 +48,10 @@ class CodeCheckerTest extends TestCase { * @param string $fileToVerify */ public function testPassValidUsage($fileToVerify) { - $checker = new OC\App\CodeChecker\CodeChecker( - new OC\App\CodeChecker\PrivateList() + $checker = new CodeChecker( + new PrivateCheck(new EmptyCheck()) ); - $errors = $checker->analyseFile(OC::$SERVERROOT . "/tests/data/app/code-checker/$fileToVerify"); + $errors = $checker->analyseFile(\OC::$SERVERROOT . "/tests/data/app/code-checker/$fileToVerify"); $this->assertEquals(0, count($errors)); } diff --git a/tests/lib/app/codechecker/deprecationlisttest.php b/tests/lib/app/codechecker/deprecationchecktest.php similarity index 70% rename from tests/lib/app/codechecker/deprecationlisttest.php rename to tests/lib/app/codechecker/deprecationchecktest.php index 2293d31a1f..2cf64a9d18 100644 --- a/tests/lib/app/codechecker/deprecationlisttest.php +++ b/tests/lib/app/codechecker/deprecationchecktest.php @@ -6,12 +6,14 @@ * See the COPYING-README file. */ -namespace Test\App; +namespace Test\App\CodeChecker; -use OC; +use OC\App\CodeChecker\CodeChecker; +use OC\App\CodeChecker\DeprecationCheck; +use OC\App\CodeChecker\EmptyCheck; use Test\TestCase; -class DeprecationListTest extends TestCase { +class DeprecationCheckTest extends TestCase { /** * @dataProvider providesFilesToCheck @@ -20,10 +22,10 @@ class DeprecationListTest extends TestCase { * @param string $fileToVerify */ public function testFindInvalidUsage($expectedErrorToken, $expectedErrorCode, $fileToVerify) { - $checker = new OC\App\CodeChecker\CodeChecker( - new OC\App\CodeChecker\DeprecationList() + $checker = new CodeChecker( + new DeprecationCheck(new EmptyCheck()) ); - $errors = $checker->analyseFile(OC::$SERVERROOT . "/tests/data/app/code-checker/$fileToVerify"); + $errors = $checker->analyseFile(\OC::$SERVERROOT . "/tests/data/app/code-checker/$fileToVerify"); $this->assertEquals(1, count($errors)); $this->assertEquals($expectedErrorCode, $errors[0]['errorCode']); @@ -32,8 +34,6 @@ class DeprecationListTest extends TestCase { public function providesFilesToCheck() { return [ - ['==', 1005, 'test-equal.php'], - ['!=', 1005, 'test-not-equal.php'], ['OCP\AppFramework\IApi', 1006, 'test-deprecated-use.php'], ['OCP\AppFramework\IApi', 1006, 'test-deprecated-use-alias.php'], ['AppFramework\IApi', 1001, 'test-deprecated-use-sub.php'], @@ -46,16 +46,18 @@ class DeprecationListTest extends TestCase { * @param string $fileToVerify */ public function testPassValidUsage($fileToVerify) { - $checker = new OC\App\CodeChecker\CodeChecker( - new OC\App\CodeChecker\DeprecationList() + $checker = new CodeChecker( + new DeprecationCheck(new EmptyCheck()) ); - $errors = $checker->analyseFile(OC::$SERVERROOT . "/tests/data/app/code-checker/$fileToVerify"); + $errors = $checker->analyseFile(\OC::$SERVERROOT . "/tests/data/app/code-checker/$fileToVerify"); $this->assertEquals(0, count($errors)); } public function validFilesData() { return [ + ['test-equal.php'], + ['test-not-equal.php'], ['test-extends.php'], ['test-implements.php'], ['test-static-call.php'], diff --git a/tests/lib/app/codechecker/mock/testlist.php b/tests/lib/app/codechecker/mock/testlist.php index c4985e8e62..5f5a9fcc38 100644 --- a/tests/lib/app/codechecker/mock/testlist.php +++ b/tests/lib/app/codechecker/mock/testlist.php @@ -21,15 +21,24 @@ namespace Test\App\CodeChecker\Mock; -use OC\App\CodeChecker\ICheckList; +use OC\App\CodeChecker\ICheck; -class TestList implements ICheckList { +class TestList implements ICheck { + /** @var ICheck */ + protected $check; + + /** + * @param ICheck $check + */ + public function __construct(ICheck $check) { + $this->check = $check; + } /** * @return string */ public function getDescription() { - return 'deprecated'; + return 'testing'; } /** diff --git a/tests/lib/app/codechecker/nodevisitortest.php b/tests/lib/app/codechecker/nodevisitortest.php index 1207ca6a04..0b5aea2832 100644 --- a/tests/lib/app/codechecker/nodevisitortest.php +++ b/tests/lib/app/codechecker/nodevisitortest.php @@ -8,7 +8,9 @@ namespace Test\AppCodeChecker; -use OC; +use OC\App\CodeChecker\CodeChecker; +use OC\App\CodeChecker\EmptyCheck; +use Test\App\CodeChecker\Mock\TestList; use Test\TestCase; class NodeVisitorTest extends TestCase { @@ -56,10 +58,10 @@ class NodeVisitorTest extends TestCase { * @param string $fileToVerify */ public function testMethodsToCheck($expectedErrors, $fileToVerify) { - $checker = new OC\App\CodeChecker\CodeChecker( - new \Test\App\CodeChecker\Mock\TestList() + $checker = new CodeChecker( + new TestList(new EmptyCheck()) ); - $errors = $checker->analyseFile(OC::$SERVERROOT . "/tests/data/app/code-checker/$fileToVerify"); + $errors = $checker->analyseFile(\OC::$SERVERROOT . "/tests/data/app/code-checker/$fileToVerify"); $this->assertCount(sizeof($expectedErrors), $errors); diff --git a/tests/lib/app/codechecker/strongcomparisonchecktest.php b/tests/lib/app/codechecker/strongcomparisonchecktest.php new file mode 100644 index 0000000000..c73eae286a --- /dev/null +++ b/tests/lib/app/codechecker/strongcomparisonchecktest.php @@ -0,0 +1,70 @@ + + * This file is licensed under the Affero General Public License version 3 or + * later. + * See the COPYING-README file. + */ + +namespace Test\App\CodeChecker; + +use OC\App\CodeChecker\CodeChecker; +use OC\App\CodeChecker\EmptyCheck; +use OC\App\CodeChecker\StrongComparisonCheck; +use Test\TestCase; + +class StrongComparisonCheckTest extends TestCase { + + /** + * @dataProvider providesFilesToCheck + * @param string $expectedErrorToken + * @param int $expectedErrorCode + * @param string $fileToVerify + */ + public function testFindInvalidUsage($expectedErrorToken, $expectedErrorCode, $fileToVerify) { + $checker = new CodeChecker( + new StrongComparisonCheck(new EmptyCheck()) + ); + $errors = $checker->analyseFile(\OC::$SERVERROOT . "/tests/data/app/code-checker/$fileToVerify"); + + $this->assertEquals(1, count($errors)); + $this->assertEquals($expectedErrorCode, $errors[0]['errorCode']); + $this->assertEquals($expectedErrorToken, $errors[0]['disallowedToken']); + } + + public function providesFilesToCheck() { + return [ + ['==', 1005, 'test-equal.php'], + ['!=', 1005, 'test-not-equal.php'], + ]; + } + + /** + * @dataProvider validFilesData + * @param string $fileToVerify + */ + public function testPassValidUsage($fileToVerify) { + $checker = new CodeChecker( + new StrongComparisonCheck(new EmptyCheck()) + ); + $errors = $checker->analyseFile(\OC::$SERVERROOT . "/tests/data/app/code-checker/$fileToVerify"); + + $this->assertEquals(0, count($errors)); + } + + public function validFilesData() { + return [ + ['test-deprecated-use.php'], + ['test-deprecated-use-alias.php'], + ['test-deprecated-use-sub.php'], + ['test-deprecated-use-sub-alias.php'], + ['test-extends.php'], + ['test-implements.php'], + ['test-static-call.php'], + ['test-const.php'], + ['test-new.php'], + ['test-use.php'], + ['test-identical-operator.php'], + ]; + } +}