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

  1. Install Drupal 11.4.4 (standard profile) and enable Security Review 4.0.x-dev.
  2. Run drush security:review.
  3. Observe three Undefined array key warnings and a blank row with status success.
  4. 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.

Command icon 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

jumpsuitgreen created an issue.