From c14061fe3bbf5491862d2bdb2edd39ded26dc724 Mon Sep 17 00:00:00 2001 From: Renaat Debleu Date: Fri, 13 Oct 2023 19:38:05 +0200 Subject: [PATCH] code review --- .github/workflows/cron.yml | 24 ++++++++-------- .github/workflows/main.yml | 36 ++++++++++++------------ db/access.php | 8 +++--- lib.php | 22 +++++++-------- phpunit.xml | 41 --------------------------- tests/delete_test.php | 2 +- tests/form_test.php | 4 +-- tests/generator/lib.php | 2 +- tests/mock_test.php | 2 +- tests/other_test.php | 6 ++-- tests/phpunit.xml | 51 ++++++++++++++++++++++++++++++++++ tests/privacy/privacy_test.php | 2 +- 12 files changed, 105 insertions(+), 95 deletions(-) delete mode 100644 phpunit.xml create mode 100644 tests/phpunit.xml diff --git a/.github/workflows/cron.yml b/.github/workflows/cron.yml index f3c77c6..06ea30f 100644 --- a/.github/workflows/cron.yml +++ b/.github/workflows/cron.yml @@ -52,52 +52,52 @@ jobs: MOODLE_BRANCH: ${{ matrix.moodle-branch }} - name: phplint - if: ${{ always() }} + if: ${{ !cancelled() }} run: moodle-plugin-ci --ansi phplint - name: phpcpd - if: ${{ always() }} + if: ${{ !cancelled() }} run: moodle-plugin-ci --ansi phpcpd - name: phpmd - if: ${{ always() }} + if: ${{ !cancelled() }} run: moodle-plugin-ci --ansi phpmd - name: phpdoc - if: ${{ always() }} + if: ${{ !cancelled() }} run: moodle-plugin-ci --ansi phpdoc - name: codechecker - if: ${{ always() }} + if: ${{ !cancelled() }} run: moodle-plugin-ci --ansi codechecker - name: validate - if: ${{ always() }} + if: ${{ !cancelled() }} run: moodle-plugin-ci --ansi validate - name: savepoints - if: ${{ always() }} + if: ${{ !cancelled() }} run: moodle-plugin-ci --ansi savepoints - name: grunt - if: ${{ always() }} + if: ${{ !cancelled() }} run: moodle-plugin-ci --ansi grunt - name: mustache - if: ${{ always() }} + if: ${{ !cancelled() }} run: moodle-plugin-ci --ansi mustache - name: phpunit continue-on-error: true - if: ${{ always() }} + if: ${{ !cancelled() }} run: moodle-plugin-ci --ansi phpunit --coverage-text --coverage-clover - name: behat - if: ${{ always() }} + if: ${{ !cancelled() }} run: moodle-plugin-ci --ansi behat --profile=chrome - name: coveralls - if: ${{ always() }} + if: ${{ !cancelled() }} run: moodle-plugin-ci coveralls-upload env: COVERALLS_REPO_TOKEN: ${{ secrets.GITHUB_TOKEN }} diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index bf6c6f9..24d0ee8 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -30,7 +30,7 @@ jobs: strategy: fail-fast: false matrix: - moodle-branch: ['MOODLE_401_STABLE', 'MOODLE_402_STABLE', 'master'] + moodle-branch: ['MOODLE_401_STABLE', 'MOODLE_402_STABLE', 'MOODLE_403_STABLE', 'master'] php: ['8.1'] database: ['mariadb', 'pgsql'] include: @@ -60,7 +60,7 @@ jobs: with: php-version: ${{ matrix.php }} ini-values: max_input_vars=5000 - coverage: xdebug + coverage: pcov - name: composer run: | @@ -80,68 +80,68 @@ jobs: working-directory: moodle/local/aws/sdk/GuzzleHttp - name: phplint - if: ${{ always() }} + if: ${{ !cancelled() }} run: moodle-plugin-ci --ansi phplint - name: phpcpd - if: ${{ always() }} + if: ${{ !cancelled() }} run: moodle-plugin-ci --ansi phpcpd - name: phpmd - if: ${{ always() }} + if: ${{ !cancelled() }} run: moodle-plugin-ci --ansi phpmd - name: phpdoc - if: ${{ always() }} + if: ${{ !cancelled() }} run: moodle-plugin-ci --ansi phpdoc - name: codechecker - if: ${{ always() }} + if: ${{ !cancelled() }} run: moodle-plugin-ci --ansi codechecker - name: validate - if: ${{ always() }} + if: ${{ !cancelled() }} run: moodle-plugin-ci --ansi validate - name: savepoints - if: ${{ always() }} + if: ${{ !cancelled() }} run: moodle-plugin-ci --ansi savepoints - name: grunt - if: ${{ always() }} + if: ${{ !cancelled() }} run: moodle-plugin-ci --ansi grunt - name: mustache - if: ${{ always() }} + if: ${{ !cancelled() }} run: moodle-plugin-ci --ansi mustache - name: phpunit - if: ${{ always() }} + if: ${{ !cancelled() }} run: moodle-plugin-ci --ansi phpunit --coverage-text --coverage-clover - name: privacy run: vendor/bin/phpunit --colors --no-coverage --testsuite tool_dataprivacy_testsuite,tool_policy_testsuite,core_privacy_testsuite - if: ${{ always() }} + if: ${{ !cancelled() }} working-directory: moodle - name: firefox - if: ${{ always() }} + if: ${{ !cancelled() }} run: moodle-plugin-ci --ansi behat - name: chrome - if: ${{ always() }} + if: ${{ !cancelled() }} run: moodle-plugin-ci --ansi behat --profile chrome - name: firefox classic - if: ${{ always() }} + if: ${{ !cancelled() }} run: moodle-plugin-ci --ansi behat --suite classic - name: chrome classic - if: ${{ always() }} + if: ${{ !cancelled() }} run: moodle-plugin-ci --ansi behat --suite classic --profile=chrome - name: coveralls - if: ${{ always() }} + if: ${{ !cancelled() }} run: moodle-plugin-ci coveralls-upload env: COVERALLS_REPO_TOKEN: ${{ secrets.GITHUB_TOKEN }} diff --git a/db/access.php b/db/access.php index 203e161..16e4302 100644 --- a/db/access.php +++ b/db/access.php @@ -27,10 +27,10 @@ $capabilities = [ 'repository/s3bucket:view' => ['captype' => 'read', 'contextlevel' => CONTEXT_MODULE, - 'archetypes' => ['editingteacher' => CAP_ALLOW, 'manager' => CAP_ALLOW]], + 'archetypes' => ['editingteacher' => CAP_ALLOW, 'manager' => CAP_ALLOW], ], 'repository/s3bucket:addinstance' => ['captype' => 'write', 'contextlevel' => CONTEXT_SYSTEM, - 'archetypes' => ['manager' => CAP_ALLOW]], + 'archetypes' => ['manager' => CAP_ALLOW], ], 'repository/s3bucket:addinstance' => ['captype' => 'write', 'contextlevel' => CONTEXT_USER, - 'archetypes' => ['editingteacher' => CAP_ALLOW, 'manager' => CAP_ALLOW]], + 'archetypes' => ['editingteacher' => CAP_ALLOW, 'manager' => CAP_ALLOW], ], 'repository/s3bucket:addinstance' => ['captype' => 'write', 'contextlevel' => CONTEXT_COURSE, - 'archetypes' => ['editingteacher' => CAP_ALLOW, 'manager' => CAP_ALLOW]]]; + 'archetypes' => ['editingteacher' => CAP_ALLOW, 'manager' => CAP_ALLOW], ], ]; diff --git a/lib.php b/lib.php index 4e948f7..c29226c 100644 --- a/lib.php +++ b/lib.php @@ -59,7 +59,7 @@ public function get_listing($path = '.', $page = 1) { 'Prefix' => $path, 'MaxKeys' => 1000, 'EncodingType' => 'url', - 'Delimiter' => '/']; + 'Delimiter' => '/', ]; $results = $files = []; $s3 = $this->create_s3(); try { @@ -75,7 +75,7 @@ public function get_listing($path = '.', $page = 1) { 'thumbnail' => $diricon, 'thumbnail_height' => 64, 'thumbnail_width' => 64, - 'path' => $item['Prefix']]; + 'path' => $item['Prefix'], ]; } $filesearch = 'Contents[?StorageClass != \'DEEP_ARCHIVE\''; @@ -93,7 +93,7 @@ public function get_listing($path = '.', $page = 1) { 'thumbnail_height' => 64, 'thumbnail_width' => 64, 'source' => $item['Key'], - 'thumbnail' => $OUTPUT->image_url(file_extension_icon($pathinfo['basename'], 64))->out(false)]; + 'thumbnail' => $OUTPUT->image_url(file_extension_icon($pathinfo['basename'], 64))->out(false), ]; } } return [ @@ -102,7 +102,7 @@ public function get_listing($path = '.', $page = 1) { 'manage' => false, 'dynload' => true, 'nologin' => true, - 'nosearch' => false]; + 'nosearch' => false, ]; } /** @@ -121,7 +121,7 @@ public function search($q, $page = 1) { 'FetchOwner' => false, 'MaxKeys' => 1000, 'EncodingType' => 'url', - 'Delimiter' => '/']; + 'Delimiter' => '/', ]; $results = $files = []; $s3 = $this->create_s3(); try { @@ -138,7 +138,7 @@ public function search($q, $page = 1) { 'thumbnail' => $diricon, 'thumbnail_height' => 64, 'thumbnail_width' => 64, - 'path' => $item['Prefix']]; + 'path' => $item['Prefix'], ]; } $filesearch = 'Contents[?StorageClass != \'DEEP_ARCHIVE\''; @@ -155,7 +155,7 @@ public function search($q, $page = 1) { 'thumbnail_height' => 64, 'thumbnail_width' => 64, 'source' => $item['Key'], - 'thumbnail' => $OUTPUT->image_url(file_extension_icon($pathinfo['basename'], 64))->out(false)]; + 'thumbnail' => $OUTPUT->image_url(file_extension_icon($pathinfo['basename'], 64))->out(false), ]; } return ['list' => $files, 'dynload' => true, 'pages' => 0, 'page' => $page]; } @@ -186,7 +186,7 @@ public function send_otherfile($reference, $lifetime) { $options = [ 'Bucket' => $this->get_option('bucket_name'), 'Key' => $reference, - 'ResponseContentDisposition' => 'attachment']; + 'ResponseContentDisposition' => 'attachment', ]; try { $result = $s3->getCommand('GetObject', $options); $req = $s3->createPresignedRequest($result, $lifetime); @@ -251,7 +251,7 @@ public function get_file($filepath, $file = '') { $options = [ 'Bucket' => $this->get_option('bucket_name'), 'Key' => $filepath, - 'SaveAs' => $path]; + 'SaveAs' => $path, ]; try { $s3->getObject($options); } catch (\Exception $e) { @@ -391,7 +391,7 @@ private function create_s3() { $arr = self::addproxy([ 'credentials' => ['key' => $accesskey, 'secret' => $this->get_option('secret_key')], 'use_path_style_endpoint' => true, - 'region' => $this->get_option('endpoint')]); + 'region' => $this->get_option('endpoint'), ]); $this->_s3client = \Aws\S3\S3Client::factory($arr); } return $this->_s3client; @@ -419,7 +419,7 @@ private static function addproxy($settings) { $day = new DateTime(); $result = new \Aws\Result([ 'CommonPrefixes' => [['Prefix' => '2020_dir']], - 'Contents' => [['Key' => '2020_f.jpg', 'Size' => 15, 'StorageClass' => 'STANDARD', 'LastModified' => $day]]]); + 'Contents' => [['Key' => '2020_f.jpg', 'Size' => 15, 'StorageClass' => 'STANDARD', 'LastModified' => $day]], ]); $mock->append($result, $result); $settings['handler'] = $mock; } diff --git a/phpunit.xml b/phpunit.xml deleted file mode 100644 index d1102eb..0000000 --- a/phpunit.xml +++ /dev/null @@ -1,41 +0,0 @@ - - - - - - - - - - - . - - - - - . - - - . - version.php - tests/coverage.php - - - - diff --git a/tests/delete_test.php b/tests/delete_test.php index 6c1e429..64cad78 100644 --- a/tests/delete_test.php +++ b/tests/delete_test.php @@ -51,7 +51,7 @@ public function test_deletebucket() { $this->assertEquals($cnt + 1, $DB->count_records('repository_instances')); $fs = get_file_storage(); $filerecord = ['component' => 'user', 'filearea' => 'draft', 'contextid' => \context_user::instance($USER->id)->id, - 'itemid' => file_get_unused_draft_itemid(), 'filename' => $reference, 'filepath' => '/']; + 'itemid' => file_get_unused_draft_itemid(), 'filename' => $reference, 'filepath' => '/', ]; $this->assertEquals(0, $DB->count_records('files_reference')); $fs->create_file_from_reference($filerecord, $repoid, $reference); $this->assertEquals(1, $DB->count_records('files_reference')); diff --git a/tests/form_test.php b/tests/form_test.php index e2fb861..a01ba92 100644 --- a/tests/form_test.php +++ b/tests/form_test.php @@ -59,7 +59,7 @@ public function setUp(): void { 'endpoint' => 'eu-west-2', 'secret_key' => 'secret', 'bucket_name' => 'test', - 'access_key' => 'abc']; + 'access_key' => 'abc', ]; $this->SetAdminUser(); } @@ -80,7 +80,7 @@ public function test_type_config_form() { 'plugin' => 's3bucket', 'action' => 'show', 'pluginname' => 'repository_s3bucket', - 'contextid' => $context->id]; + 'contextid' => $context->id, ]; $mform = new \repository_type_form('', $para); $this->assertEquals([], \repository_s3bucket::type_form_validation($mform, null, [])); ob_start(); diff --git a/tests/generator/lib.php b/tests/generator/lib.php index 06e53a8..2a042a3 100644 --- a/tests/generator/lib.php +++ b/tests/generator/lib.php @@ -59,7 +59,7 @@ protected function prepare_record(array $record) { 'access_key' => 'access', 'secret_key' => 'secret', 'endpoint' => 'us-east-1', - 'bucket_name' => 'testrepo']; + 'bucket_name' => 'testrepo', ]; return array_merge($arr, $record); } } diff --git a/tests/mock_test.php b/tests/mock_test.php index 09c8dda..560e33a 100644 --- a/tests/mock_test.php +++ b/tests/mock_test.php @@ -69,7 +69,7 @@ public function test_listobjects() { 'Prefix' => '.', 'MaxKeys' => 1000, 'EncodingType' => 'url', - 'Delimiter' => '/']; + 'Delimiter' => '/', ]; $client = $this->getMockBuilder('Aws\S3\S3Client') ->disableOriginalConstructor() ->setMethods(['listObjectsV2', 'getPaginator', 'getObject']) diff --git a/tests/other_test.php b/tests/other_test.php index fd96065..d48ea01 100644 --- a/tests/other_test.php +++ b/tests/other_test.php @@ -62,7 +62,7 @@ public function setUp(): void { 'endpoint' => 'eu-north-1', 'secret_key' => 'secret', 'bucket_name' => 'test', - 'access_key' => 'abc']; + 'access_key' => 'abc', ]; $this->SetAdminUser(); } @@ -82,7 +82,7 @@ public function test_sendfiles3() { $repo = new \repository_s3bucket($this->repo); $fs = get_file_storage(); $filerecord = ['component' => 'user', 'filearea' => 'draft', 'contextid' => \context_user::instance($USER->id)->id, - 'itemid' => file_get_unused_draft_itemid(), 'filename' => 'filename.jpg', 'filepath' => '/']; + 'itemid' => file_get_unused_draft_itemid(), 'filename' => 'filename.jpg', 'filepath' => '/', ]; $file = $fs->create_file_from_string($filerecord, 'test content'); $this->expectException('repository_exception'); $repo->send_file($file); @@ -184,7 +184,7 @@ public function test_getfile() { $repo->set_option($this->data); $draft = file_get_unused_draft_itemid(); $filerecord = ['component' => 'user', 'filearea' => 'draft', 'contextid' => $context->id, - 'itemid' => $draft, 'filename' => 'filename.txt', 'filepath' => '/']; + 'itemid' => $draft, 'filename' => 'filename.txt', 'filepath' => '/', ]; get_file_storage()->create_file_from_string($filerecord, 'test content'); $result = $repo->get_file('/filename.txt'); $this->assertEquals('/filename.txt', $result['url']); diff --git a/tests/phpunit.xml b/tests/phpunit.xml new file mode 100644 index 0000000..b7029b2 --- /dev/null +++ b/tests/phpunit.xml @@ -0,0 +1,51 @@ + + + + + + + + + + + + + + + . + + + + + + classes + tests/generator + externallib.php + lib.php + locallib.php + renderer.php + rsslib.php + + + + diff --git a/tests/privacy/privacy_test.php b/tests/privacy/privacy_test.php index 9ab4753..e8dec58 100644 --- a/tests/privacy/privacy_test.php +++ b/tests/privacy/privacy_test.php @@ -24,7 +24,7 @@ namespace repository_s3bucket\privacy; -use \core_privacy\tests\provider_testcase; +use core_privacy\tests\provider_testcase; /** * Privacy tests.