Allow PHP version to be changed from Site Settings

[danielbachhuber]

Related #32

Proposed Changes

Allows the PHP version to be changed from Site Settings:

CleanShot.2024-06-10.at.07.34.42.mp4

Also restarts the server after the PHP version is changed.

Testing Instructions

1. Create a new site.
2. New site should launch with a default version of PHP 8.0.
3. Navigate to the Studio settings for the site.
4. Edit the PHP version to a different PHP version.
5. Verify the server restarts and the site is running the new PHP version.

Pre-merge Checklist

• Have you checked for TypeScript, React or other console errors?

[danielbachhuber]

Not correctly saving/applying the PHP version quite yet. My intent is to have the PHP version saved in appdata-v1.json first, and then the server restarted from that data. I don't feel like I fully grok the data flow though.

Also need to handle invalid or outdated PHP version stored in appdata-v1.json (e.g. what should happen if "5.6" is present?)

Happy to take suggestions on the tests to add.

[derekblank]

Not correctly saving/applying the PHP version quite yet. My intent is to have the PHP version saved in appdata-v1.json first, and then the server restarted from that data.

The PHP version is being saved correctly to appdata-v1.json, but it looks like the change wasn't being applied to update the site details in the UI (and when starting the server). #226 should fix this issue.

Also need to handle invalid or outdated PHP version stored in appdata-v1.json (e.g. what should happen if "5.6" is present?)

Potentially, we could just fallback to using DEFAULT_PHP_VERSION if the phpVersion value in appdata-v1.json is not one of the values listed in const availablePhpVersions = [ '7.4', '8.0', '8.1', '8.2' ];. Perhaps this array should be stored in constants, too.

[matt-west]

Thanks @danielbachhuber!

We had originally planned to add the PHP version picker to the add/edit site form under a new "Advanced settings" section. (Moving the local path field there too.) Clicking the edit link on the site settings screen would launch the full edit site modal.

Was that plan changed? (I may have missed any discussion there.)

I prefer the dedicated modal when editing the PHP version for an existing site. Assuming we're okay with maintaining the additional UI in the app. I think users should be able to set the PHP version when creating a new site too though.

[danielbachhuber]

@matt-west I decided to add this implementation for now because I saw there was a lot of demand for a PHP version switcher and didn't want to go down the rabbit hole adding an overly complex form.

[katinthehatsite]

I decided to add this implementation for now because I saw there was a lot of demand for a PHP version switcher

I have not looked at the PR yet but I think it would be good to have PHP version shipped before / around WC EU so that the app could be showcased with this feature. It has been requested and mentioned in multiple demo calls, P2s and conversations and was generally marked as one of the main blockers for not using Studio compared to Local.

[matt-west]

@danielbachhuber Sounds good. We can add the Advanced settings dropdown to the new site form later.

[fluiddot]

I tested the PR and works as expected 🎊 . The only limitation I found is that demo sites don't use the PHP version set. As far as I've tested, they are always created with version 8.1.29. I wouldn't consider it a blocker but I presume users will expect that the PHP version matches between the local and demo site.

[fluiddot]

Nitpick: The phpVersion can be a supported value or undefined.

Suggested change

	phpVersion: this.server.options.phpVersion || DEFAULT_PHP_VERSION,
	phpVersion: this.server.options.phpVersion ?? DEFAULT_PHP_VERSION,

[danielbachhuber]

@Automattic/yolo I think this is ready for a review.

[fluiddot]

It works as expected 🎊, nice work @danielbachhuber @derekblank 🏅 !

I added some comments that shouldn't block the PR but that I plan to address in a separate PR.

Additionally, I noticed a minor issue regarding the window size after we added this new element. As you can see in the screenshot, the settings tab's content now overflows and causes the content to scroll. If I'm not wrong, we've avoided in the past that this tab have a scroll, so we could consider increasing the minimum window height to do so.

cc @matt-west

[fluiddot]

I think the hook useGetPhpVersion is not necessary because the PHP version is already coming from the site details in selectedSite.

	const phpVersion = selectedSite.phpVersion ?? DEFAULT_PHP_VERSION;

Not sure if this approach has been followed based on the other hook useGetWpVersion. If so, the WordPress version needs to be retrieved from the WordPress files as it's not referenced in the site details. That's the main reason for needing that hook.

[fluiddot]

I'll add these changes in a separate PR to avoid blocking this one.

[fluiddot]

I'll add these changes in a separate PR to avoid blocking this one.

#239

[matt-west]

While we've tried to avoid scrolling on the settings screen in the past, we were always going to reach the point that it was needed. I’m happy with letting that screen scroll now.

[fluiddot]

I wonder if we should warn the user about site restarting when changing the PHP version on a running site. This is not a destructive action but it might be unexpected by the user. WDYT?

[matt-west]

@fluiddot I'm not sure that's necessary. I feel there's an expectation that changing the PHP version in that modal would apply the change right away.

[wojtekn]

While we've tried to avoid scrolling on the settings screen in the past, we were always going to reach the point that it was needed. I’m happy with letting that screen scroll now.

@matt-west it's okay, but it seems we need to make the scrollbar visible all the time, similarly to the sidebar. Now, the main content sidebar scrollbar appears only if the user starts scrolling.

[fluiddot]

@matt-west it's okay, but it seems we need to make the scrollbar visible all the time, similarly to the sidebar. Now, the main content sidebar scrollbar appears only if the user starts scrolling.

FYI I've applied this fix in #255.

[lajennylove]

It works as expected 🎊, nice work @danielbachhuber @derekblank 🏅 !

I added some comments that shouldn't block the PR but that I plan to address in a separate PR.

Additionally, I noticed a minor issue regarding the window size after we added this new element. As you can see in the screenshot, the settings tab's content now overflows and causes the content to scroll. If I'm not wrong, we've avoided in the past that this tab have a scroll, so we could consider increasing the minimum window height to do so.

cc @matt-west

Hey there guys!

I don't know if this is related to the new implementation regarding the PHP selector, but as soon as I tried to use it I found permission issues. I tried to chown the permissions but it was having the same issue anyway.

To replicate:

• Switch to PHP 8.2
• Clone this Sage/Vite theme https://github.com/lajennylove/sage-vite which requires to do the following

composer install
yarn 
yarn build

• And then activate the theme, or just live preview it to avoid having issues.

This is the complete log that I got.

Warning: require(/var/www/html/wp-content/cache/acorn/framework/cache/packages.php): Failed to open stream: Permission denied in /var/www/html/wp-content/themes/sage-vite/vendor/illuminate/filesystem/Filesystem.php on line 123

Fatal error: Uncaught Error: Failed opening required '/var/www/html/wp-content/cache/acorn/framework/cache/packages.php' (include_path='.:') in /var/www/html/wp-content/themes/sage-vite/vendor/illuminate/filesystem/Filesystem.php:123 Stack trace: #0 /var/www/html/wp-content/themes/sage-vite/vendor/illuminate/filesystem/Filesystem.php(124): Illuminate\Filesystem\Filesystem::Illuminate\Filesystem\{closure}() #1 /var/www/html/wp-content/themes/sage-vite/vendor/roots/acorn/src/Illuminate/Foundation/PackageManifest.php(111): Illuminate\Filesystem\Filesystem->getRequire('/var/www/html/w...') #2 /var/www/html/wp-content/themes/sage-vite/vendor/roots/acorn/src/Illuminate/Foundation/PackageManifest.php(90): Illuminate\Foundation\PackageManifest->getManifest() #3 /var/www/html/wp-content/themes/sage-vite/vendor/roots/acorn/src/Illuminate/Foundation/PackageManifest.php(79): Illuminate\Foundation\PackageManifest->config('aliases') #4 /var/www/html/wp-content/themes/sage-vite/vendor/roots/acorn/src/Roots/Acorn/Bootstrap/RegisterFacades.php(25): Illuminate\Foundation\PackageManifest->aliases() #5 /var/www/html/wp-content/themes/sage-vite/vendor/roots/acorn/src/Illuminate/Foundation/Application.php(263): Roots\Acorn\Bootstrap\RegisterFacades->bootstrap(Object(Roots\Acorn\Application)) #6 /var/www/html/wp-content/themes/sage-vite/vendor/roots/acorn/src/Illuminate/Foundation/Http/Kernel.php(186): Illuminate\Foundation\Application->bootstrapWith(Array) #7 /var/www/html/wp-content/themes/sage-vite/vendor/roots/acorn/src/Roots/Acorn/Bootloader.php(187): Illuminate\Foundation\Http\Kernel->bootstrap(Object(Illuminate\Http\Request)) #8 /var/www/html/wp-content/themes/sage-vite/vendor/roots/acorn/src/Roots/Acorn/Bootloader.php(103): Roots\Acorn\Bootloader->bootHttp() #9 /var/www/html/wp-content/themes/sage-vite/functions.php(43): Roots\Acorn\Bootloader->boot() #10 /var/www/html/wp-settings.php(663): include('/var/www/html/w...') #11 /var/www/html/wp-config.php(96): require_once('/var/www/html/w...') #12 /var/www/html/wp-load.php(50): require_once('/var/www/html/w...') #13 /var/www/html/wp-admin/admin.php(34): require_once('/var/www/html/w...') #14 /var/www/html/wp-admin/customize.php(13): require_once('/var/www/html/w...') #15 {main} thrown in /var/www/html/wp-content/themes/sage-vite/vendor/illuminate/filesystem/Filesystem.php on line 123

I hope this is the right place to add this if not I can move it somewhere else.

Best regards.

[danielbachhuber]

@lajennylove Can you open a new GitHub issue for the issue? Thanks!

[fluiddot]

Thanks @lajennylove for reporting the issue. As Daniel mentioned, it would be great if you could open a GitHub issue with this information. Thanks 🙇 !

[lajennylove]

Hey @danielbachhuber and @fluiddot I already sent the report about the issue I hope it helps.

Thank you!

[fluiddot]

Hey @danielbachhuber and @fluiddot I already sent the report about the issue I hope it helps.

Thank you!

Great, thank you @lajennylove for reporting the issue.

[lajennylove]

Hey @danielbachhuber and @fluiddot I already sent the report about the issue I hope it helps.
Thank you!

Great, thank you @lajennylove for reporting the issue.

Anytime!