Merge pull request #434 from FriendsOfCake/bugfixes-helpers-commands

ADmad · web-flow · commit 589df3d7b875 · 2026-05-11T22:09:28.000+05:30
Fix five real defects in helpers and console commands
diff --git 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
@@ -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
@@ -112,16 +112,17 @@ 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;
                 }
             }
         } else {
             $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
@@ -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
@@ -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
@@ -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
@@ -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(
+            ["<error>Could not modify `$filePath`.</error>"],
+            $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(
             ['<info>Modifying view...</info>'],
diff --git 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
@@ -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
@@ -0,0 +1,5 @@
+<?php
+return [
+    'first' => '<a data-from-file="1" href="{{url}}">{{text}}</a>',
+    'last' => '<a data-from-file="1" href="{{url}}">{{text}}</a>',
+];

Original file line number	Diff line number	Diff line change
`@@ -112,16 +112,17 @@ protected function _markActiveCrumb(): void`
`112`	`112`
`113`	`113`	`$key = null;`
`114`	`114`	`if ($this->getConfig('ariaCurrent') === 'lastWithLink') {`
`115`		`- foreach (array_reverse($this->crumbs, true) as $key => $crumb) {`
	`115`	`+ foreach (array_reverse($this->crumbs, true) as $i => $crumb) {`
`116`	`116`	`if (isset($crumb['url'])) {`
	`117`	`+ $key = $i;`
`117`	`118`	`break;`
`118`	`119`	`}`
`119`	`120`	`}`
`120`	`121`	`} else {`
`121`	`122`	`$key = count($this->crumbs) - 1;`
`122`	`123`	`}`
`123`	`124`
`124`		`- if (!$key) {`
	`125`	`+ if ($key === null) {`
`125`	`126`	`return;`
`126`	`127`	`}`
`127`	`128`