From 9d1b92f758caa2b28e817a7a6e2a73bfcb532dfd Mon Sep 17 00:00:00 2001 From: Ben Thomson Date: Sat, 7 Dec 2024 13:05:49 +0800 Subject: [PATCH] Fix remaining code smells --- modules/backend/widgets/Lists.php | 2 +- modules/cms/classes/Page.php | 2 +- modules/cms/classes/Theme.php | 2 +- modules/cms/console/CreateTheme.php | 12 +- .../system/classes/asset/PackageManager.php | 2 +- modules/system/console/CreateFactory.php | 4 +- .../system/console/asset/npm/NpmInstall.php | 1 - modules/system/console/asset/npm/NpmRun.php | 4 +- .../system/console/asset/npm/NpmUpdate.php | 3 +- .../system/console/asset/npm/NpmVersion.php | 1 - .../system/console/asset/vite/ViteWatch.php | 2 - .../tests/console/CreateCommandTest.php | 3 +- .../console/asset/mix/MixInstallTest.php | 18 +- .../console/asset/npm/NpmInstallTest.php | 8 +- .../tests/console/asset/npm/NpmRunTest.php | 4 +- .../tests/console/asset/npm/NpmUpdateTest.php | 4 +- .../console/asset/vite/ViteInstallTest.php | 19 +- .../system/tests/twig/SecurityPolicyTest.php | 191 +++++++++++------- modules/system/twig/SecurityPolicy.php | 6 +- 19 files changed, 160 insertions(+), 128 deletions(-) diff --git a/modules/backend/widgets/Lists.php b/modules/backend/widgets/Lists.php index f20ed71378..bc96bc7b7d 100644 --- a/modules/backend/widgets/Lists.php +++ b/modules/backend/widgets/Lists.php @@ -1225,7 +1225,7 @@ public function getColumnValue($record, $column) { $value = $this->getColumnValueRaw($record, $column); - $customMethod = 'eval'. studly_case($column->type) .'TypeValue'; + $customMethod = 'eval' . studly_case($column->type) . 'TypeValue'; if ($this->methodExists($customMethod)) { $value = $this->{$customMethod}($record, $column, $value); } else { diff --git a/modules/cms/classes/Page.php b/modules/cms/classes/Page.php index 47179630df..ad8af486e2 100644 --- a/modules/cms/classes/Page.php +++ b/modules/cms/classes/Page.php @@ -203,7 +203,7 @@ public static function resolveMenuItem(object $item, string $url, Theme $theme, return null; } - $controller = Controller::getController() ?: new Controller; + $controller = Controller::getController() ?: new Controller(); $pageUrl = $controller->pageUrl($item->reference, [], $routePersistence); $result = []; diff --git a/modules/cms/classes/Theme.php b/modules/cms/classes/Theme.php index 6491b275cc..a57ceb04be 100644 --- a/modules/cms/classes/Theme.php +++ b/modules/cms/classes/Theme.php @@ -684,7 +684,7 @@ public function getDatasource(): DatasourceInterface public function __get($name) { if (in_array(strtolower($name), ['id', 'path', 'dirname', 'config', 'formconfig', 'previewimageurl'])) { - $method = 'get'. ucfirst($name); + $method = 'get' . ucfirst($name); return $this->$method(); } diff --git a/modules/cms/console/CreateTheme.php b/modules/cms/console/CreateTheme.php index 84e5552b15..561f03faf1 100644 --- a/modules/cms/console/CreateTheme.php +++ b/modules/cms/console/CreateTheme.php @@ -178,8 +178,8 @@ protected function tailwindPostCreate(string $processor): void '--no-interaction' => true, '--force' => true, '--silent' => true, - '--tailwind' => true - ] + '--tailwind' => true, + ], ], // Ensure all require packages are available for the new theme and add the new theme to our npm workspaces $processor . ':install' => [ @@ -188,8 +188,8 @@ protected function tailwindPostCreate(string $processor): void 'assetPackage' => ['theme-' . $this->getNameInput()], '--no-interaction' => true, '--silent' => false, - '--disable-tty' => true - ] + '--disable-tty' => true, + ], ], // Run an initial compile to ensure styles are available for first load $processor . ':compile' => [ @@ -198,8 +198,8 @@ protected function tailwindPostCreate(string $processor): void '--package' => ['theme-' . $this->getNameInput()], '--no-interaction' => true, '--silent' => true, - ] - ] + ], + ], ]; foreach ($commands as $command => $data) { diff --git a/modules/system/classes/asset/PackageManager.php b/modules/system/classes/asset/PackageManager.php index d405c37ae4..ee63455418 100644 --- a/modules/system/classes/asset/PackageManager.php +++ b/modules/system/classes/asset/PackageManager.php @@ -351,7 +351,7 @@ public function registerPackage(string $name, string $path, string $type = 'mix' $this->packages[$type][$name] = [ 'path' => $path, 'package' => $package, - 'config' => $config + 'config' => $config, ]; } diff --git a/modules/system/console/CreateFactory.php b/modules/system/console/CreateFactory.php index 25bffba7fa..3a6da1bf24 100644 --- a/modules/system/console/CreateFactory.php +++ b/modules/system/console/CreateFactory.php @@ -1,6 +1,6 @@ -artisan('mix:install', [ 'assetPackage' => ['theme-assettest'], '--package-json' => '/some/file', - '--no-install' => true + '--no-install' => true, ]) ->expectsOutputToContain('The supplied --package-json path does not exist.') ->assertExitCode(1); @@ -58,7 +58,7 @@ public function testMixInstallSinglePackage(): void $this->artisan('mix:install', [ 'assetPackage' => ['theme-assettest'], '--package-json' => $this->jsonPath, - '--no-install' => true + '--no-install' => true, ]) ->expectsQuestion('laravel-mix was not found as a dependency in package.json, would you like to add it?', true) ->expectsOutput('Adding theme-assettest (modules/system/tests/fixtures/themes/assettest) to the workspaces.packages property in package.json') @@ -81,7 +81,7 @@ public function testMixInstallMultiplePackages(): void $this->artisan('mix:install', [ 'assetPackage' => ['theme-assettest', 'theme-npmtest'], '--package-json' => $this->jsonPath, - '--no-install' => true + '--no-install' => true, ]) ->expectsQuestion('laravel-mix was not found as a dependency in package.json, would you like to add it?', true) ->expectsOutput('Adding theme-assettest (modules/system/tests/fixtures/themes/assettest) to the workspaces.packages property in package.json') @@ -109,7 +109,7 @@ public function testMixInstallMissingPackage(): void $this->artisan('mix:install', [ 'assetPackage' => ['theme-assettest2'], '--package-json' => $this->jsonPath, - '--no-install' => true + '--no-install' => true, ]) ->assertExitCode(1); }); @@ -121,7 +121,7 @@ public function testMixInstallRelativePath(): void $this->artisan('mix:install', [ 'assetPackage' => ['theme-assettest'], '--package-json' => 'modules/system/tests/package.json', - '--no-install' => true + '--no-install' => true, ]) ->expectsQuestion('laravel-mix was not found as a dependency in package.json, would you like to add it?', true) ->expectsOutput('Adding theme-assettest (modules/system/tests/fixtures/themes/assettest) to the workspaces.packages property in package.json') @@ -135,15 +135,15 @@ public function testMixInstallIgnoredPackage(): void $packageJson = json_decode(File::get($this->jsonPath), JSON_OBJECT_AS_ARRAY); $packageJson['workspaces'] = [ 'ignoredPackages' => [ - 'modules/system/tests/fixtures/themes/assettest' - ] + 'modules/system/tests/fixtures/themes/assettest', + ], ]; File::put($this->jsonPath, json_encode($packageJson, JSON_PRETTY_PRINT)); $this->artisan('mix:install', [ 'assetPackage' => ['theme-assettest'], '--package-json' => $this->jsonPath, - '--no-install' => true + '--no-install' => true, ]) ->expectsQuestion('laravel-mix was not found as a dependency in package.json, would you like to add it?', false) ->expectsOutput('The requested package theme-assettest (modules/system/tests/fixtures/themes/assettest) is ignored, remove it from package.json to continue.') @@ -165,7 +165,7 @@ public function testMixInstallWithNpmInstall(): void $this->artisan('mix:install', [ 'assetPackage' => ['theme-assettest'], '--package-json' => $this->jsonPath, - '--disable-tty' => true + '--disable-tty' => true, ]) ->expectsQuestion('laravel-mix was not found as a dependency in package.json, would you like to add it?', true) ->expectsOutput('Adding theme-assettest (modules/system/tests/fixtures/themes/assettest) to the workspaces.packages property in package.json') diff --git a/modules/system/tests/console/asset/npm/NpmInstallTest.php b/modules/system/tests/console/asset/npm/NpmInstallTest.php index 611ab20df7..c926cd365f 100644 --- a/modules/system/tests/console/asset/npm/NpmInstallTest.php +++ b/modules/system/tests/console/asset/npm/NpmInstallTest.php @@ -56,7 +56,7 @@ public function testNpmInstallSingle(): void $this->artisan('npm:install', [ 'package' => 'theme-npmtest', 'npmArgs' => ['is-odd'], - '--disable-tty' => true + '--disable-tty' => true, ]) ->assertExitCode(0); @@ -92,7 +92,7 @@ public function testNpmInstallMultiple(): void $this->artisan('npm:install', [ 'package' => 'theme-npmtest', 'npmArgs' => ['is-odd', 'is-even'], - '--disable-tty' => true + '--disable-tty' => true, ]) ->assertExitCode(0); @@ -132,7 +132,7 @@ public function testNpmInstallSingleDev(): void 'package' => 'theme-npmtest', 'npmArgs' => ['is-odd'], '--dev' => true, - '--disable-tty' => true + '--disable-tty' => true, ]) ->assertExitCode(0); @@ -169,7 +169,7 @@ public function testNpmInstallMultipleDev(): void 'package' => 'theme-npmtest', 'npmArgs' => ['is-odd', 'is-even'], '--dev' => true, - '--disable-tty' => true + '--disable-tty' => true, ]) ->assertExitCode(0); diff --git a/modules/system/tests/console/asset/npm/NpmRunTest.php b/modules/system/tests/console/asset/npm/NpmRunTest.php index 51f699dbb0..9b81e00f81 100644 --- a/modules/system/tests/console/asset/npm/NpmRunTest.php +++ b/modules/system/tests/console/asset/npm/NpmRunTest.php @@ -32,7 +32,7 @@ public function testNpmRunScript(): void $this->artisan('npm:run', [ 'package' => 'theme-npmtest', 'script' => 'testScript', - '--disable-tty' => true + '--disable-tty' => true, ]) ->expectsOutputToContain('> echo "Winter says $((1+2))"') ->expectsOutputToContain('Winter says 3') @@ -47,7 +47,7 @@ public function testNpmRunScriptFailed(): void $this->artisan('npm:run', [ 'package' => 'theme-npmtest', 'script' => 'testMissingScript', - '--disable-tty' => true + '--disable-tty' => true, ]) ->expectsOutputToContain('Script "testMissingScript" is not defined in package "theme-npmtest"') ->assertExitCode(1); diff --git a/modules/system/tests/console/asset/npm/NpmUpdateTest.php b/modules/system/tests/console/asset/npm/NpmUpdateTest.php index 38b63d9827..b8f9df6cb0 100644 --- a/modules/system/tests/console/asset/npm/NpmUpdateTest.php +++ b/modules/system/tests/console/asset/npm/NpmUpdateTest.php @@ -55,7 +55,7 @@ public function testNpmUpdate(): void // Update the contents of package.json to include a package at an old patch $packageJson = json_decode(File::get($this->jsonPath), JSON_OBJECT_AS_ARRAY); $packageJson['dependencies'] = [ - 'is-odd' => '^3.0.0' + 'is-odd' => '^3.0.0', ]; File::put($this->jsonPath, json_encode($packageJson, JSON_PRETTY_PRINT)); @@ -63,7 +63,7 @@ public function testNpmUpdate(): void $this->artisan('npm:update', [ 'package' => 'theme-npmtest', '--save' => true, - '--disable-tty' => true + '--disable-tty' => true, ]) ->assertExitCode(0); diff --git a/modules/system/tests/console/asset/vite/ViteInstallTest.php b/modules/system/tests/console/asset/vite/ViteInstallTest.php index 7d97ccf107..da4acebf15 100644 --- a/modules/system/tests/console/asset/vite/ViteInstallTest.php +++ b/modules/system/tests/console/asset/vite/ViteInstallTest.php @@ -9,6 +9,7 @@ class ViteInstallTest extends TestCase { + protected string $fixturePath; protected string $jsonPath; protected string $lockPath; protected string $backupPath; @@ -45,7 +46,7 @@ public function testViteInstallMissingPackageJson(): void $this->artisan('vite:install', [ 'assetPackage' => ['theme-assettest'], '--package-json' => '/some/file', - '--no-install' => true + '--no-install' => true, ]) ->expectsOutputToContain('The supplied --package-json path does not exist.') ->assertExitCode(1); @@ -57,7 +58,7 @@ public function testViteInstallSinglePackage(): void $this->artisan('vite:install', [ 'assetPackage' => ['theme-assettest'], '--package-json' => $this->jsonPath, - '--no-install' => true + '--no-install' => true, ]) ->expectsQuestion('vite was not found as a dependency in package.json, would you like to add it?', true) ->expectsQuestion('laravel-vite-plugin was not found as a dependency in package.json, would you like to add it?', true) @@ -82,7 +83,7 @@ public function testViteInstallMultiplePackages(): void $this->artisan('vite:install', [ 'assetPackage' => ['theme-assettest', 'theme-npmtest'], '--package-json' => $this->jsonPath, - '--no-install' => true + '--no-install' => true, ]) ->expectsQuestion('vite was not found as a dependency in package.json, would you like to add it?', true) ->expectsQuestion('laravel-vite-plugin was not found as a dependency in package.json, would you like to add it?', true) @@ -113,7 +114,7 @@ public function testViteInstallMissingPackage(): void $this->artisan('vite:install', [ 'assetPackage' => ['theme-assettest2'], '--package-json' => $this->jsonPath, - '--no-install' => true + '--no-install' => true, ]) ->assertExitCode(1); }); @@ -125,7 +126,7 @@ public function testViteInstallRelativePath(): void $this->artisan('vite:install', [ 'assetPackage' => ['theme-assettest'], '--package-json' => 'modules/system/tests/package.json', - '--no-install' => true + '--no-install' => true, ]) ->expectsQuestion('vite was not found as a dependency in package.json, would you like to add it?', true) ->expectsQuestion('laravel-vite-plugin was not found as a dependency in package.json, would you like to add it?', true) @@ -140,8 +141,8 @@ public function testViteInstallIgnoredPackage(): void $packageJson = json_decode(File::get($this->jsonPath), JSON_OBJECT_AS_ARRAY); $packageJson['workspaces'] = [ 'ignoredPackages' => [ - 'modules/system/tests/fixtures/themes/assettest' - ] + 'modules/system/tests/fixtures/themes/assettest', + ], ]; File::put($this->jsonPath, json_encode($packageJson, JSON_PRETTY_PRINT)); @@ -149,7 +150,7 @@ public function testViteInstallIgnoredPackage(): void $this->artisan('vite:install', [ 'assetPackage' => ['theme-assettest'], '--package-json' => $this->jsonPath, - '--no-install' => true + '--no-install' => true, ]) ->expectsQuestion('vite was not found as a dependency in package.json, would you like to add it?', false) ->expectsQuestion('laravel-vite-plugin was not found as a dependency in package.json, would you like to add it?', false) @@ -172,7 +173,7 @@ public function testViteInstallWithNpmInstall(): void $this->artisan('vite:install', [ 'assetPackage' => ['theme-assettest'], '--package-json' => $this->jsonPath, - '--disable-tty' => true + '--disable-tty' => true, ]) ->expectsQuestion('vite was not found as a dependency in package.json, would you like to add it?', true) ->expectsQuestion('laravel-vite-plugin was not found as a dependency in package.json, would you like to add it?', true) diff --git a/modules/system/tests/twig/SecurityPolicyTest.php b/modules/system/tests/twig/SecurityPolicyTest.php index 7fcd8018d9..5c038fc289 100644 --- a/modules/system/tests/twig/SecurityPolicyTest.php +++ b/modules/system/tests/twig/SecurityPolicyTest.php @@ -65,11 +65,14 @@ public function testCannotWriteToAModel() { $this->expectException(\Twig\Sandbox\SecurityNotAllowedMethodError::class); - $this->renderTwigInCmsController(' - {% set modelTest = model.setAttribute("test", "value") %} - ', [ - 'model' => new \Winter\Storm\Database\Model(), - ]); + $this->renderTwigInCmsController( + ' + {% set modelTest = model.setAttribute("test", "value") %} + ', + [ + 'model' => new \Winter\Storm\Database\Model(), + ] + ); } public function testCanReadFromAModel() @@ -77,12 +80,15 @@ public function testCanReadFromAModel() $model = new \Winter\Storm\Database\Model(); $model->test = 'value'; - $result = trim($this->renderTwigInCmsController(' - {% set modelTest = model.getAttribute("test") %} - {{- modelTest -}} - ', [ - 'model' => $model, - ])); + $result = trim($this->renderTwigInCmsController( + ' + {% set modelTest = model.getAttribute("test") %} + {{- modelTest -}} + ', + [ + 'model' => $model, + ] + )); $this->assertEquals('value', $result); } @@ -95,11 +101,14 @@ public function testCannotFillAModel() $model->addFillable('test'); $model->test = 'value'; - $this->renderTwigInCmsController(' - {% set modelTest = model.fill({ test: \'value2\' }) %} - ', [ - 'model' => new \Winter\Storm\Database\Model(), - ]); + $this->renderTwigInCmsController( + ' + {% set modelTest = model.fill({ test: \'value2\' }) %} + ', + [ + 'model' => new \Winter\Storm\Database\Model(), + ] + ); } catch (\Twig\Sandbox\SecurityNotAllowedMethodError $e) { // Ensure value hasn't changed $this->assertEquals('value', $model->test); @@ -111,22 +120,28 @@ public function testCannotSaveAModel() { $this->expectException(\Twig\Sandbox\SecurityNotAllowedMethodError::class); - $this->renderTwigInCmsController(' - {% set modelTest = model.save() %} - ', [ - 'model' => new \Winter\Storm\Database\Model(), - ]); + $this->renderTwigInCmsController( + ' + {% set modelTest = model.save() %} + ', + [ + 'model' => new \Winter\Storm\Database\Model(), + ] + ); } public function testCannotPushAModel() { $this->expectException(\Twig\Sandbox\SecurityNotAllowedMethodError::class); - $this->renderTwigInCmsController(' - {% set modelTest = model.push() %} - ', [ - 'model' => new \Winter\Storm\Database\Model(), - ]); + $this->renderTwigInCmsController( + ' + {% set modelTest = model.push() %} + ', + [ + 'model' => new \Winter\Storm\Database\Model(), + ] + ); } public function testCannotUpdateAModel() @@ -137,44 +152,56 @@ public function testCannotUpdateAModel() $model->addFillable('test'); $model->test = 'value'; - $this->renderTwigInCmsController(' - {% set modelTest = model.update({ test: \'value2\' }) %} - ', [ - 'model' => $model, - ]); + $this->renderTwigInCmsController( + ' + {% set modelTest = model.update({ test: \'value2\' }) %} + ', + [ + 'model' => $model, + ] + ); } public function testCannotDeleteAModel() { $this->expectException(\Twig\Sandbox\SecurityNotAllowedMethodError::class); - $this->renderTwigInCmsController(' - {% set modelTest = model.delete() %} - ', [ - 'model' => new \Winter\Storm\Database\Model(), - ]); + $this->renderTwigInCmsController( + ' + {% set modelTest = model.delete() %} + ', + [ + 'model' => new \Winter\Storm\Database\Model(), + ] + ); } public function testCannotForceDeleteAModel() { $this->expectException(\Twig\Sandbox\SecurityNotAllowedMethodError::class); - $this->renderTwigInCmsController(' - {% set modelTest = model.forceDelete() %} - ', [ - 'model' => new \Winter\Storm\Database\Model(), - ]); + $this->renderTwigInCmsController( + ' + {% set modelTest = model.forceDelete() %} + ', + [ + 'model' => new \Winter\Storm\Database\Model(), + ] + ); } public function testCannotExtendAModelWithABehaviour() { $this->expectException(\Twig\Sandbox\SecurityNotAllowedMethodError::class); - $this->renderTwigInCmsController(' - {% set model = model.extendClassWith("Winter\Storm\Database\Behaviors\Encryptable") %} - ', [ - 'model' => new \Winter\Storm\Database\Model(), - ]); + $this->renderTwigInCmsController( + ' + {% set model = model.extendClassWith("Winter\Storm\Database\Behaviors\Encryptable") %} + ', + [ + 'model' => new \Winter\Storm\Database\Model(), + ] + ); } public function testExtendingModelBeforePassingIntoTwigShouldStillWork() @@ -184,11 +211,14 @@ public function testExtendingModelBeforePassingIntoTwigShouldStillWork() return 'foo'; }); - $result = trim($this->renderTwigInCmsController(' - {{- model.foo() -}} - ', [ - 'model' => $model, - ])); + $result = trim($this->renderTwigInCmsController( + ' + {{- model.foo() -}} + ', + [ + 'model' => $model, + ] + )); $this->assertEquals('foo', $result); } @@ -206,42 +236,51 @@ public function testCannotDeleteInDatasource() { $this->expectException(\Twig\Sandbox\SecurityNotAllowedMethodError::class); - $this->renderTwigInCmsController(' - {% set datasource = datasource.delete() %} - ', [ - 'datasource' => new FileDatasource( - base_path('modules/system/tests/fixtures/themes/test'), - new Filesystem() - ), - ]); + $this->renderTwigInCmsController( + ' + {% set datasource = datasource.delete() %} + ', + [ + 'datasource' => new FileDatasource( + base_path('modules/system/tests/fixtures/themes/test'), + new Filesystem() + ), + ] + ); } public function testCannotInsertInDatasource() { $this->expectException(\Twig\Sandbox\SecurityNotAllowedMethodError::class); - $this->renderTwigInCmsController(' - {% set datasource = datasource.insert() %} - ', [ - 'datasource' => new FileDatasource( - base_path('modules/system/tests/fixtures/themes/test'), - new Filesystem() - ), - ]); + $this->renderTwigInCmsController( + ' + {% set datasource = datasource.insert() %} + ', + [ + 'datasource' => new FileDatasource( + base_path('modules/system/tests/fixtures/themes/test'), + new Filesystem() + ), + ] + ); } public function testCannotUpdateInDatasource() { $this->expectException(\Twig\Sandbox\SecurityNotAllowedMethodError::class); - $this->renderTwigInCmsController(' - {% set datasource = datasource.update() %} - ', [ - 'datasource' => new FileDatasource( - base_path('modules/system/tests/fixtures/themes/test'), - new Filesystem() - ), - ]); + $this->renderTwigInCmsController( + ' + {% set datasource = datasource.update() %} + ', + [ + 'datasource' => new FileDatasource( + base_path('modules/system/tests/fixtures/themes/test'), + new Filesystem() + ), + ] + ); } public function testCannotChangeThemeDirectory() @@ -262,7 +301,7 @@ protected function renderTwigInCmsController(string $source, array $vars = []) 'this' => [ 'controller' => $controller, 'page' => new Page(), - 'theme' => new Theme() + 'theme' => new Theme(), ], ] + $vars); } diff --git a/modules/system/twig/SecurityPolicy.php b/modules/system/twig/SecurityPolicy.php index 3a25ae2832..6dedd2b718 100644 --- a/modules/system/twig/SecurityPolicy.php +++ b/modules/system/twig/SecurityPolicy.php @@ -7,11 +7,11 @@ use Illuminate\Database\Eloquent\Model as DbModel; use Twig\Markup; use Twig\Sandbox\SecurityNotAllowedFunctionError; -use Twig\Template; -use Twig\Sandbox\SecurityPolicyInterface; use Twig\Sandbox\SecurityNotAllowedMethodError; use Twig\Sandbox\SecurityNotAllowedPropertyError; use Twig\Sandbox\SecurityNotAllowedTagError; +use Twig\Sandbox\SecurityPolicyInterface; +use Twig\Template; use Winter\Storm\Halcyon\Datasource\DatasourceInterface; use Winter\Storm\Halcyon\Model as HalcyonModel; @@ -86,7 +86,7 @@ final class SecurityPolicy implements SecurityPolicyInterface Theme::class => [ 'setDirName', 'registerHalcyonDatasource', - 'getDatasource' + 'getDatasource', ], ];