remove legacyDrush check - #387
Conversation
📝 WalkthroughWalkthrough
ChangesDrush legacy handling
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/Drupal/Tests/Driver/Unit/DrushDriverMethodsTest.php (2)
71-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the complete default cache-clear sequence.
Verify
cache-clear drushoccurs beforecache:rebuild, not just that a rebuild exists.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Drupal/Tests/Driver/Unit/DrushDriverMethodsTest.php` around lines 71 - 79, Update testCacheClearRebuilds to assert the complete default cache-clear command sequence: verify cache-clear drush appears before cache:rebuild in the recorded driver invocations, while retaining coverage that the invocations are non-empty.
31-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that bootstrap does not invoke Drush.
Also assert
$driver->invocationsis empty afterbootstrap(). This locks in removal of the version probe that could throw during bootstrap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Drupal/Tests/Driver/Unit/DrushDriverMethodsTest.php` around lines 31 - 36, Update testBootstrapMarksAsBootstrapped to assert that $driver->invocations is empty immediately after bootstrap(), while preserving the existing bootstrapped-state assertions. This verifies bootstrap no longer invokes Drush or performs the removed version probe.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/Drupal/Tests/Driver/Unit/DrushDriverMethodsTest.php`:
- Around line 71-79: Update testCacheClearRebuilds to assert the complete
default cache-clear command sequence: verify cache-clear drush appears before
cache:rebuild in the recorded driver invocations, while retaining coverage that
the invocations are non-empty.
- Around line 31-36: Update testBootstrapMarksAsBootstrapped to assert that
$driver->invocations is empty immediately after bootstrap(), while preserving
the existing bootstrapped-state assertions. This verifies bootstrap no longer
invokes Drush or performs the removed version probe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d5fd86e5-f754-4e40-bcf1-b6c381e2cd87
📒 Files selected for processing (3)
src/Drupal/Driver/DrushDriver.phptests/Drupal/Tests/Driver/Unit/DrushDriverMethodsTest.phptests/Drupal/Tests/Driver/Unit/DrushDriverTest.php
💤 Files with no reviewable changes (1)
- tests/Drupal/Tests/Driver/Unit/DrushDriverTest.php
|
Failing rector change appears to be due to a new version of rector; I can fix it in this MR, but it may be out of scope, so I won't fix it until requested. |
|
@ptmkenny |
|
@ptmkenny |
|
@ptmkenny |
|
@AlexSkrypnyk All passed this time! |
I'm on Drupal 11 with the latest Drush. My behat tests run in GHA sometimes fail because I get a RuntimeException() in bootstrap() when the runner is setting up wireguard, which causes isLegacyDrush() to resolve TRUE as it fails closed.
However, DrupalDriver now only supports Drupal 10/11, so isLegacyDrush() seems to just be dead code that can accidentally be triggered by a Drupal 11 site, which definitely can't use legacy drush.
Prepared with assistance from Claude Opus.
Summary by CodeRabbit
Bug Fixes
Tests