Problem/Motivation
Running drush security:review without --store on a stock installation produces three PHP warnings and a malformed result row:
[warning] Undefined array key "result" SecurityReviewCommands.php:167
[warning] Undefined array key "result" SecurityReviewCommands.php:200
[warning] Undefined array key "findings" SecurityReviewCommands.php:229
--------------------------------------------------------------------------- ---------
Message Status
--------------------------------------------------------------------------- ---------
success
Untrusted roles do not have administrative or trusted Drupal permissions. success
The first row has an empty message and a status of success.
The status is the important part. That row is `account_creation`, which was never run. It is reported as `success` — indistinguishable in both table and JSON output from a check that ran and genuinely passed. For a security review tool, a check that did not execute yet reports as passing is a meaningful correctness problem, not just log noise.
This affects every installation out of the box, because security_review.install skips `account_creation` on install (and `security_review_post_update_add_account_creation_skip` does the same for existing sites). Every site therefore has at least one skipped check, so every site hits this on the first un-stored Drush run.
Steps to reproduce
- Install Drupal 11.4.4 (standard profile) and enable Security Review 4.0.x-dev.
- Run drush security:review.
- Observe three Undefined array key warnings and a blank row with status success.
- Run drush security:review --store — the warnings do not appear.
Verified on Drupal 11.4.4, PHP 8.3, Drush 13.3.3, 4.0.x-dev at fbb4a1b.
Proposed resolution
Two changes, both small:
Move the skipped-check filter out of the if ($store) block so config-skipped checks are excluded from the result set on every run, not only when storing. This addresses the cause.
Guard the three remaining dereferences against an empty $result, consistent with the existing guard on line 196. This addresses the class of problem — any check with an invalid or missing stored result currently produces warnings and a false success, and lastResult() can return [] for reasons other than skipping (an interrupted run, a corrupted or manually cleared key/value entry).
Change 2 matters independently of change 1: without it, --lastrun on a site that has never stored results reaches the same code path.
Open question
Should skipped checks be omitted from Drush output, or shown with a distinct skipped status?
Currently the behavior is inconsistent. They are hidden with `--store` and shown as a blank row without it. Omitting them matches the existing `--store` behavior and is the smaller change. Showing them explicitly is arguably better for CI use.
Let me know which you prefer. I'll default to omission (matching current `--store` behavior) unless you'd rather have the explicit status.
Remaining tasks
- Determine the open question above
- Implement the fix
- Add test coverage; SecurityReviewDrushCommandTest currently has no case for a skipped check in the un-stored path
- Verify `--check=`, `--skip=` and `--lastrun` paths
User interface changes
None.
API changes
None.
Data model changes
None.
Issue fork security_review-3613691
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments