-
Notifications
You must be signed in to change notification settings - Fork 52
CLI: Implement studio site set-php-version
#2154
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📊 Performance Test ResultsComparing 67a338e vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
bcotrim
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM and works as described.
While reviewing, I wondered if we should have a studio site update command instead of separate set-* commands.
Advantages:
- Consistent with
studio create- Same option names (--php,--domain,--https) - Batch updates - One command, one restart instead of potentially 3
- Less code - Shared boilerplate (load site → lock appdata → update → restart if running)
What do you think?
I like that idea 👍 I have a whole train of PRs waiting to merge here, though (#2151, this, and #2155). I'd prefer to land all three and then circle back to refactor the |
Related issues
Proposed Changes
studio site set-php-versioncommand, which updates the appdata file and restarts the site if running.Testing Instructions
npm run cli:buildnode dist/cli/main.js site set-php-version true --path PATH_TO_SITE(wherePATH_TO_SITEis the path to a Studio site)node dist/cli/main.js site start --path PATH_TO_SITEnode dist/cli/main.js site set-php-version 8.2 --path PATH_TO_SITEPre-merge Checklist