From fa67ec5b6be83be95850615da45226a54bc6a951 Mon Sep 17 00:00:00 2001 From: mscherer Date: Mon, 11 May 2026 14:18:39 +0200 Subject: [PATCH] Fix five real defects in helpers and console commands. - BreadcrumbsHelper: a single-crumb trail (key 0) was never marked active because `!$key` treated index 0 as falsy. Split the "no match found" path (key stays null in `lastWithLink` mode when no crumb has a URL) from the "match at index 0" path. - FormHelper::control(): the `$this->templater()->push()` / `pop()` pair was unbalanced under exceptions. Anything thrown by the option mutators or the parent `control()` call leaked the pushed state into subsequent controls. Wrapped the body in `try/finally`. - PaginatorHelper::first()/last(): the docblock advertises `templates` as accepting an array or a file name, but `_templateOptions()` unconditionally did array operations (`+=`, key access) on it and crashed for string values. String templates now pass through unchanged so the templater can `load()` the file. - InstallCommand::_runNPMInstall(): chdir'd into the plugin directory and never restored the previous cwd. Any later relative-path work in the same process resolved against the wrong directory. Captured `getcwd()` and restore it in a `finally` block. - ModifyViewCommand::_modifyView(): re-running the command was non-idempotent. The literal `str_replace` for the `initialize()` body always matched on the second run too, duplicating `parent::initialize();`. Added guards so each transformation is skipped when its target marker is already present; the command now correctly reports an error when no effective change can be made. Adds regression tests for each fix, including a paginator template file fixture under `tests/test_app/config/`. --- src/Command/InstallCommand.php | 47 ++++++----- src/Command/ModifyViewCommand.php | 42 ++++++---- src/View/Helper/BreadcrumbsHelper.php | 5 +- src/View/Helper/FormHelper.php | 80 ++++++++++--------- src/View/Helper/PaginatorHelper.php | 6 ++ tests/TestCase/Command/InstallCommandTest.php | 22 +++++ .../Command/ModifyViewCommandTest.php | 51 +++++++++++- .../View/Helper/BreadcrumbsHelperTest.php | 20 +++++ .../View/Helper/PaginatorHelperTest.php | 18 +++++ tests/test_app/config/paginator_templates.php | 5 ++ 10 files changed, 218 insertions(+), 78 deletions(-) create mode 100644 tests/test_app/config/paginator_templates.php diff --git a/src/Command/InstallCommand.php b/src/Command/InstallCommand.php index 94112ff7..04775d1c 100644 --- a/src/Command/InstallCommand.php +++ b/src/Command/InstallCommand.php @@ -190,33 +190,40 @@ protected function _deleteNodeModules(): bool protected function _runNPMInstall(array &$output, int &$return, ConsoleIo $io, bool $useLatest = false): void { $pluginPath = Plugin::path('BootstrapUI'); + $previousCwd = getcwd(); if (!$this->_changeWorkingDirectory($pluginPath)) { $io->error("Could not change into plugin directory `$pluginPath`."); $this->abort(); } - $args = []; - if ($useLatest) { - $args[] = '--package-lock false'; - } - switch ($io->level()) { - case ConsoleIo::QUIET: - if ($this->_isWindows()) { - $null = 'NUL'; - } else { - $null = '/dev/null'; - } - - $args[] = "--silent > $null"; - break; + try { + $args = []; + if ($useLatest) { + $args[] = '--package-lock false'; + } + switch ($io->level()) { + case ConsoleIo::QUIET: + if ($this->_isWindows()) { + $null = 'NUL'; + } else { + $null = '/dev/null'; + } + + $args[] = "--silent > $null"; + break; + + case ConsoleIo::VERBOSE: + $args[] = '--verbose'; + break; + } + $args = implode(' ', $args); - case ConsoleIo::VERBOSE: - $args[] = '--verbose'; - break; + exec("npm install $args", $output, $return); + } finally { + if ($previousCwd !== false) { + $this->_changeWorkingDirectory($previousCwd); + } } - $args = implode(' ', $args); - - exec("npm install $args", $output, $return); } /** diff --git a/src/Command/ModifyViewCommand.php b/src/Command/ModifyViewCommand.php index f8a825b4..7d481e2d 100644 --- a/src/Command/ModifyViewCommand.php +++ b/src/Command/ModifyViewCommand.php @@ -60,21 +60,33 @@ protected function _modifyView(string $filePath): bool return false; } - $content = str_replace( - 'use Cake\\View\\View', - 'use BootstrapUI\\View\\UIView', - $content, - ); - $content = str_replace( - 'class AppView extends View', - 'class AppView extends UIView', - $content, - ); - $content = str_replace( - " public function initialize(): void\n {\n", - " public function initialize(): void\n {\n parent::initialize();\n", - $content, - ); + $original = $content; + + if (!str_contains($content, 'use BootstrapUI\\View\\UIView')) { + $content = str_replace( + 'use Cake\\View\\View', + 'use BootstrapUI\\View\\UIView', + $content, + ); + } + if (!str_contains($content, 'class AppView extends UIView')) { + $content = str_replace( + 'class AppView extends View', + 'class AppView extends UIView', + $content, + ); + } + if (!str_contains($content, 'parent::initialize();')) { + $content = str_replace( + " public function initialize(): void\n {\n", + " public function initialize(): void\n {\n parent::initialize();\n", + $content, + ); + } + + if ($content === $original) { + return false; + } return $this->_writeFile($filePath, $content); } diff --git a/src/View/Helper/BreadcrumbsHelper.php b/src/View/Helper/BreadcrumbsHelper.php index 7439577b..d9d0545e 100644 --- a/src/View/Helper/BreadcrumbsHelper.php +++ b/src/View/Helper/BreadcrumbsHelper.php @@ -112,8 +112,9 @@ protected function _markActiveCrumb(): void $key = null; if ($this->getConfig('ariaCurrent') === 'lastWithLink') { - foreach (array_reverse($this->crumbs, true) as $key => $crumb) { + foreach (array_reverse($this->crumbs, true) as $i => $crumb) { if (isset($crumb['url'])) { + $key = $i; break; } } @@ -121,7 +122,7 @@ protected function _markActiveCrumb(): void $key = count($this->crumbs) - 1; } - if (!$key) { + if ($key === null) { return; } diff --git a/src/View/Helper/FormHelper.php b/src/View/Helper/FormHelper.php index 0c18ab77..e045820b 100644 --- a/src/View/Helper/FormHelper.php +++ b/src/View/Helper/FormHelper.php @@ -479,50 +479,52 @@ public function control(string $fieldName, array $options = []): string $options['templates'] = []; } - switch ($options['type']) { - case 'checkbox': - case 'radio': - case 'select': - case 'range': - $function = '_' . $options['type'] . 'Options'; - $options = $this->{$function}($fieldName, $options); - break; - - default: - $options = $this->_labelOptions($fieldName, $options); - break; - } + try { + switch ($options['type']) { + case 'checkbox': + case 'radio': + case 'select': + case 'range': + $function = '_' . $options['type'] . 'Options'; + $options = $this->{$function}($fieldName, $options); + break; + + default: + $options = $this->_labelOptions($fieldName, $options); + break; + } - $options = $this->_spacingOptions($fieldName, $options); - $options = $this->_containerOptions($fieldName, $options); - $options = $this->_feedbackStyleOptions($fieldName, $options); - $options = $this->_ariaOptions($fieldName, $options); - $options = $this->_placeholderOptions($fieldName, $options); - $options = $this->_helpOptions($fieldName, $options); - $options = $this->_tooltipOptions($fieldName, $options); + $options = $this->_spacingOptions($fieldName, $options); + $options = $this->_containerOptions($fieldName, $options); + $options = $this->_feedbackStyleOptions($fieldName, $options); + $options = $this->_ariaOptions($fieldName, $options); + $options = $this->_placeholderOptions($fieldName, $options); + $options = $this->_helpOptions($fieldName, $options); + $options = $this->_tooltipOptions($fieldName, $options); - if ( - isset($options['append']) || - isset($options['prepend']) - ) { - $options['injectErrorClass'] = $this->getConfig('templates.errorClass'); - } - - unset( - $options['formGroupPosition'], - $options['feedbackStyle'], - $options['spacing'], - $options['inline'], - $options['nestedInput'], - $options['switch'], - ); + if ( + isset($options['append']) || + isset($options['prepend']) + ) { + $options['injectErrorClass'] = $this->getConfig('templates.errorClass'); + } - $result = parent::control($fieldName, $options); + unset( + $options['formGroupPosition'], + $options['feedbackStyle'], + $options['spacing'], + $options['inline'], + $options['nestedInput'], + $options['switch'], + ); - $result = $this->_postProcessElement($result, $fieldName, $options); + $result = parent::control($fieldName, $options); - if ($newTemplates) { - $this->templater()->pop(); + $result = $this->_postProcessElement($result, $fieldName, $options); + } finally { + if ($newTemplates) { + $this->templater()->pop(); + } } return $result; diff --git a/src/View/Helper/PaginatorHelper.php b/src/View/Helper/PaginatorHelper.php index a6e7c6d7..c2da5cd2 100644 --- a/src/View/Helper/PaginatorHelper.php +++ b/src/View/Helper/PaginatorHelper.php @@ -290,6 +290,12 @@ protected function _templateOptions(string $name, array $options): array $label = $options['label']; unset($options['label']); + // When `templates` is a file name (string), pass it through to the + // caller untouched so it can load the file via the templater. + if (is_string($options['templates'])) { + return $options; + } + $options['templates'] += [ "{$name}" => $this->getConfig("templates.{$name}", ''), "{$name}Active" => $this->getConfig("templates.{$name}Active", ''), diff --git a/tests/TestCase/Command/InstallCommandTest.php b/tests/TestCase/Command/InstallCommandTest.php index 971347da..1e0bbf18 100644 --- a/tests/TestCase/Command/InstallCommandTest.php +++ b/tests/TestCase/Command/InstallCommandTest.php @@ -14,6 +14,7 @@ use Cake\Core\Plugin; use Cake\TestSuite\TestCase; use Cake\Utility\Filesystem; +use ReflectionMethod; use SplFileInfo; class InstallCommandTest extends TestCase @@ -760,6 +761,27 @@ public function testLinkPluginAssetsFailure() ); } + /** + * `_runNPMInstall()` previously left the process cwd inside the plugin + * directory because it called `chdir()` without restoring it. Any later + * relative-path work in the same process would then resolve incorrectly. + */ + public function testRunNPMInstallRestoresCwd() + { + $cwdBefore = getcwd(); + $this->assertNotFalse($cwdBefore); + + $command = new InstallCommand(); + $io = new ConsoleIo(new StubConsoleOutput(), new StubConsoleOutput()); + $output = []; + $return = 0; + + $method = new ReflectionMethod(InstallCommand::class, '_runNPMInstall'); + $method->invokeArgs($command, [&$output, &$return, $io, false]); + + $this->assertSame($cwdBefore, getcwd(), 'Working directory was not restored after _runNPMInstall.'); + } + public function testHelp() { $this->exec('bootstrap install --help'); diff --git a/tests/TestCase/Command/ModifyViewCommandTest.php b/tests/TestCase/Command/ModifyViewCommandTest.php index 5fd9301d..ebce44c2 100644 --- a/tests/TestCase/Command/ModifyViewCommandTest.php +++ b/tests/TestCase/Command/ModifyViewCommandTest.php @@ -141,6 +141,39 @@ public function testPathIsNotAFile() ); } + public function testAlreadyModifiedReportsError() + { + // Running the command on a file that has already been modified + // must be a no-op (no duplicate `parent::initialize();`) and report + // an error instead of silently succeeding. + /** @var \BootstrapUI\Command\ModifyViewCommand|\PHPUnit\Framework\MockObject\MockObject $command */ + $command = $this + ->getMockBuilder(ModifyViewCommand::class) + ->onlyMethods(['_writeFile']) + ->getMock(); + + $command + ->expects($this->never()) + ->method('_writeFile'); + + $out = new StubConsoleOutput(); + $err = new StubConsoleOutput(); + $io = new ConsoleIo($out, $err); + + try { + $result = $command->run([], $io); + } catch (StopException $exception) { + $result = $exception->getCode(); + } + + $filePath = APP . 'View' . DS . 'AppView.php'; + $this->assertEquals(Command::CODE_ERROR, $result); + $this->assertEquals( + ["Could not modify `$filePath`."], + $err->messages(), + ); + } + public function testFileCannotBeRead() { /** @var \BootstrapUI\Command\ModifyViewCommand|\PHPUnit\Framework\MockObject\MockObject $command */ @@ -179,6 +212,22 @@ public function testFileCannotBeRead() public function testFileCannotBeWritten() { + // _modifyView only calls _writeFile when the file content actually + // needs changes, so swap in an unmodified skeleton for this test. + $comparisonsPath = + Plugin::path('BootstrapUI') . 'tests' . DS . 'comparisons' . DS . 'Command' . DS . 'ModifyView' . DS; + + $filePath = APP . 'View' . DS . 'AppView.php'; + + copy( + $filePath, + APP . 'View' . DS . 'AppView.php.backup', + ); + copy( + $comparisonsPath . 'AppView.skeleton.php', + $filePath, + ); + /** @var \BootstrapUI\Command\ModifyViewCommand|\PHPUnit\Framework\MockObject\MockObject $command */ $command = $this ->getMockBuilder(ModifyViewCommand::class) @@ -200,8 +249,6 @@ public function testFileCannotBeWritten() $result = $exception->getCode(); } - $filePath = APP . 'View' . DS . 'AppView.php'; - $this->assertEquals(Command::CODE_ERROR, $result); $this->assertEquals( ['Modifying view...'], diff --git a/tests/TestCase/View/Helper/BreadcrumbsHelperTest.php b/tests/TestCase/View/Helper/BreadcrumbsHelperTest.php index d3063f21..b8c5403f 100644 --- a/tests/TestCase/View/Helper/BreadcrumbsHelperTest.php +++ b/tests/TestCase/View/Helper/BreadcrumbsHelperTest.php @@ -275,4 +275,24 @@ public function testNoCrumbs() $result = $this->Breadcrumbs->render(); $this->assertEmpty($result); } + + public function testSingleCrumbIsMarkedActive() + { + $result = $this->Breadcrumbs + ->add('only') + ->render(); + + $expected = [ + 'nav' => ['aria-label' => 'breadcrumb'], + 'ol' => ['class' => 'breadcrumb'], + ['li' => ['class' => 'breadcrumb-item active', 'aria-current' => 'page']], + ['span' => true], + 'only', + '/span', + '/li', + '/ol', + '/nav', + ]; + $this->assertHtml($expected, $result); + } } diff --git a/tests/TestCase/View/Helper/PaginatorHelperTest.php b/tests/TestCase/View/Helper/PaginatorHelperTest.php index 524cafe9..a8794275 100644 --- a/tests/TestCase/View/Helper/PaginatorHelperTest.php +++ b/tests/TestCase/View/Helper/PaginatorHelperTest.php @@ -474,6 +474,24 @@ public function testFirstCustomTemplate() $this->assertHtml($expected, $result); } + /** + * The `templates` option is documented as accepting a file name in addition + * to an array. Passing a string previously crashed in `_templateOptions` + * which tried to array-merge it. + */ + public function testFirstCustomTemplateFile() + { + $result = $this->Paginator->first('«', [ + 'templates' => 'paginator_templates', + ]); + $expected = [ + ['a' => ['data-from-file' => '1', 'href' => '/Clients/index']], + '«', + '/a', + ]; + $this->assertHtml($expected, $result); + } + public function testLast() { $result = $this->Paginator->last(); diff --git a/tests/test_app/config/paginator_templates.php b/tests/test_app/config/paginator_templates.php new file mode 100644 index 00000000..26910930 --- /dev/null +++ b/tests/test_app/config/paginator_templates.php @@ -0,0 +1,5 @@ + '{{text}}', + 'last' => '{{text}}', +];