Skip to content

Fix five real defects in helpers and console commands#434

Merged
ADmad merged 1 commit into
masterfrom
bugfixes-helpers-commands
May 11, 2026
Merged

Fix five real defects in helpers and console commands#434
ADmad merged 1 commit into
masterfrom
bugfixes-helpers-commands

Conversation

@dereuromark
Copy link
Copy Markdown
Member

@dereuromark dereuromark commented May 11, 2026

Summary

Five small, independent defects across helpers and console commands. Each is testable in isolation; combined into one PR because the diff per fix is tiny.

1. BreadcrumbsHelper::_markActiveCrumb() — single-crumb a11y

if (!$key) return; treated index 0 as falsy, so a one-item breadcrumb trail never got the active class or aria-current="page". The same guard was implicitly relied on by the lastWithLink mode when no crumb has a URL (the loop key falls back to 0), so a naive fix to === null would break that case. Split the two intents: $key only gets assigned inside the lastWithLink loop when a URL crumb is actually found.

2. FormHelper::control() — templater stack leak

templater()->push() at the top and the matching pop() at the end were unbalanced under exceptions: anything thrown by _labelOptions, _spacingOptions, …, or parent::control() left the templater in the pushed state and corrupted every subsequent control in the request. Wrapped the body in try/finally.

3. PaginatorHelper::first() / last() — string templates crashed

The docblock says templates accepts an array or a file name, and the call site explicitly branches on is_string() for load vs add. But _templateOptions() did $options['templates'] += [...] and then indexed into it as an array, which crashed for any string value. String templates now pass through _templateOptions() unchanged.

(Note: the per-template {{label}} substitution that happens for inline templates is intentionally not applied to file templates — those control their own markup. The test fixture omits {{label}} accordingly. Cross-cutting label substitution for file templates is a separate question.)

4. InstallCommand::_runNPMInstall() — cwd leak

chdir($pluginPath) was never restored. Any later relative-path work in the same process resolved against the plugin directory instead of the original cwd. Captured getcwd() before the chdir and restore it in a finally.

5. ModifyViewCommand::_modifyView() — non-idempotent

Re-running bin/cake bootstrap modify_view on an already-modified AppView.php would duplicate parent::initialize(); because the literal str_replace(" public function initialize(): void\n {\n", ...) matched the new content too. The command also silently "succeeded" when nothing was changed. Added per-transformation guards (str_contains against the target marker) and a final "nothing to do" check that returns false so the command reports Could not modify instead of pretending it patched the file.

Tests

  • BreadcrumbsHelperTest::testSingleCrumbIsMarkedActive — exact-match assertion that one-item trail gets aria-current="page" on the <li> (matches the existing non-URL path semantics).
  • PaginatorHelperTest::testFirstCustomTemplateFile — passes templates => 'paginator_templates' (a new fixture at tests/test_app/config/paginator_templates.php) and asserts the file's markup wins.
  • InstallCommandTest::testRunNPMInstallRestoresCwd — calls _runNPMInstall via reflection and asserts getcwd() unchanged afterwards.
  • ModifyViewCommandTest::testAlreadyModifiedReportsError — runs the command against the (already modified) AppView.php in the test app and asserts _writeFile is never called and the user sees an error.
  • ModifyViewCommandTest::testFileCannotBeWritten updated to first copy the skeleton in, since the idempotent path now correctly skips the write for the already-modified file.

All gates green locally: phpunit (751 tests, 1136 assertions), phpstan level 8 (clean), phpcs (clean).

- 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/`.
@dereuromark dereuromark requested a review from ADmad May 11, 2026 15:10
@ADmad ADmad merged commit 589df3d into master May 11, 2026
6 of 7 checks passed
@ADmad ADmad deleted the bugfixes-helpers-commands branch May 11, 2026 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants