[4.x] migrate-fresh: show migration output when verbose#1464
Conversation
Don't run tenants:migrate silently by default during migrate-fresh; its output is shown when the command is run with -v/--verbose. --------- Co-authored-by: lordofthebrain <f.mangelsdorf@gmail.com>
📝 WalkthroughWalkthroughMigrateFresh now conditionally suppresses tenant migration output by importing ChangesOutput suppression in migrate-fresh command
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Since CI runs tests with -v, it sets SHELL_VERBOSITY to 1, making the tenants:migrate-fresh output always verbose
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1464 +/- ##
============================================
+ Coverage 86.00% 86.71% +0.70%
- Complexity 1175 1256 +81
============================================
Files 185 186 +1
Lines 3429 3710 +281
============================================
+ Hits 2949 3217 +268
- Misses 480 493 +13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@tests/CommandsTest.php`:
- Around line 374-387: Save the original superglobal values before you overwrite
them and restore them conditionally in the finally block instead of always
unsetting; specifically, before setting $_ENV['SHELL_VERBOSITY'] and
$_SERVER['SHELL_VERBOSITY'] to 0, capture their previous values (e.g.
$prevEnvShellVerbosity = array_key_exists('SHELL_VERBOSITY', $_ENV) ?
$_ENV['SHELL_VERBOSITY'] : null and similarly $prevServerShellVerbosity), then
in the finally block: if $prevEnvShellVerbosity is null
unset($_ENV['SHELL_VERBOSITY']) else $_ENV['SHELL_VERBOSITY'] =
$prevEnvShellVerbosity; do the same for $_SERVER['SHELL_VERBOSITY'], and restore
the process environment with putenv('SHELL_VERBOSITY') when the original getenv
was false or putenv("SHELL_VERBOSITY=$shellVerbosity") when it existed; update
the finally block around the Artisan calls accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f9f9b7c5-d307-435c-a517-07ffc18033af
📒 Files selected for processing (1)
tests/CommandsTest.php
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/CommandsTest.php (1)
377-383: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRestore
$_ENV['SHELL_VERBOSITY']conditionally.Line 377 collapses “unset” and
0into the same value, so Line 383 leaves$_ENV['SHELL_VERBOSITY']defined after the test even when it was absent before. That leaks process state into later tests and can make verbosity-sensitive assertions order-dependent.Suggested fix
- $originalVerbosity = $_ENV['SHELL_VERBOSITY'] ?? 0; + $hadOriginalVerbosity = array_key_exists('SHELL_VERBOSITY', $_ENV); + $originalVerbosity = $_ENV['SHELL_VERBOSITY'] ?? null; try { $_ENV['SHELL_VERBOSITY'] = 0; Artisan::call('tenants:migrate-fresh'); $defaultOutput = Artisan::output(); } finally { - $_ENV['SHELL_VERBOSITY'] = $originalVerbosity; + if ($hadOriginalVerbosity) { + $_ENV['SHELL_VERBOSITY'] = $originalVerbosity; + } else { + unset($_ENV['SHELL_VERBOSITY']); + } }🤖 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/CommandsTest.php` around lines 377 - 383, The test setup around Artisan::call('tenants:migrate-fresh') is not restoring $_ENV['SHELL_VERBOSITY'] correctly because it treats an unset value the same as 0. Update the CommandsTest logic to record whether SHELL_VERBOSITY existed before the test, then in the finally block either restore its previous value or remove it entirely if it was originally absent. Keep the fix localized to this test helper block so later tests do not inherit leaked verbosity state.
🤖 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.
Duplicate comments:
In `@tests/CommandsTest.php`:
- Around line 377-383: The test setup around
Artisan::call('tenants:migrate-fresh') is not restoring $_ENV['SHELL_VERBOSITY']
correctly because it treats an unset value the same as 0. Update the
CommandsTest logic to record whether SHELL_VERBOSITY existed before the test,
then in the finally block either restore its previous value or remove it
entirely if it was originally absent. Keep the fix localized to this test helper
block so later tests do not inherit leaked verbosity state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0b98e734-acfc-48a1-a1c6-73673748d832
📒 Files selected for processing (1)
tests/CommandsTest.php
stancl
changed the title
There was a problem hiding this comment.
Pull request overview
This PR adjusts the tenants:migrate-fresh command so that per-tenant migration output from the nested tenants:migrate invocation is suppressed by default, but becomes visible when the command is run with -v (verbose). It also adds a regression test to validate the default-vs-verbose output behavior.
Changes:
- Update
MigrateFresh::migrateTenant()to route the nestedtenants:migrateoutput either to the current command output (verbose) or toNullOutput(non-verbose). - Add a test asserting that
"Migrating tenant {tenantKey}"only appears whentenants:migrate-freshis executed with-v. - Add a test workaround for CI verbosity inheritance when calling Artisan commands.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/Commands/MigrateFresh.php |
Conditionally forwards or suppresses nested migration output based on verbosity. |
tests/CommandsTest.php |
Adds coverage for default vs verbose output behavior for tenants:migrate-fresh. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Resubmission of #1369 (by @lordofthebrain), changes adapted to v4. Also added a test (passes with the MigrateFresh changes, fails without them).
Summary by CodeRabbit
Features
tenants:migrate-freshconsole output: per-tenant migration messages are now suppressed by default for a cleaner run.-v, per-tenant progress is shown, including theMigrating tenant {tenantKey}message.Tests
Migrating tenant {tenantKey}message only when-vis enabled.