Allow PHP version to be changed from Site Settings #225

danielbachhuber · 2024-06-09T21:35:54Z

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

Create a new site.
New site should launch with a default version of PHP 8.0.
Navigate to the Studio settings for the site.
Edit the PHP version to a different PHP version.
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 · 2024-06-09T21:38:35Z

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 · 2024-06-10T06:24:10Z

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.

* Add phpVersion to updateSiteDetails * Update tests for changing the PHP version

matt-west · 2024-06-10T15:24:32Z

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 · 2024-06-10T16:35:09Z

@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 · 2024-06-10T18:28:48Z

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 · 2024-06-11T08:08:01Z

@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 · 2024-06-11T11:52:52Z

src/site-server.ts

@@ -99,6 +101,7 @@ export class SiteServer {
 			...this.details,
 			url: this.server.url,
 			port: this.server.options.port,
+			phpVersion: this.server.options.phpVersion || DEFAULT_PHP_VERSION,


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,

src/constants.ts

* Use `DEFAULT_PHP_VERSION` constant from `wp-now` * Use available PHP versions of Playground * Add `web-streams-polyfill` package for unit tests * Polyfill `ReadableStream` in unit tests Web streams are used by `php-wasm`, so we need to polyfull them if we import the library in unit tests.

danielbachhuber · 2024-06-11T18:38:26Z

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

* Mock `matchMedia` * Expose label in `SettingsRow` component * Add test to cover the case of changing PHP version * Revert "Expose label in `SettingsRow` component" This reverts commit bc37161. * Update change PHP version test case

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 · 2024-06-12T11:09:06Z

src/components/content-tab-settings.tsx

@@ -30,6 +32,7 @@ export function ContentTabSettings( { selectedSite }: ContentTabSettingsProps )
 	// Empty strings account for legacy sites lacking a stored password.
 	const storedPassword = decodePassword( selectedSite.adminPassword ?? '' );
 	const password = storedPassword === '' ? 'password' : storedPassword;
+	const phpVersion = useGetPhpVersion( selectedSite );


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.

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

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

#239

matt-west · 2024-06-12T12:45:05Z

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 · 2024-06-12T13:59:33Z

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 · 2024-06-12T14:04:40Z

@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.

* Remove `getPhpVersion` hook and IPC handler * Use `??` operator instead of `||` when setting the php version * Add inline comment in Jest setup The comment clarifies why we need the polyfill. Related to: #231 (comment) * Update `ContentTabSettings` unit tests

* Bump default PHP version to `8.1` This change is driven by the fact that version `8.0` already reached its end-of-life by 2024. https://www.php.net/supported-versions.php * Address failure in `createSite` unit test

wojtekn · 2024-06-17T08:58:12Z

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 · 2024-06-17T10:00:09Z

@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 · 2024-06-18T01:26:54Z

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 · 2024-06-18T11:40:00Z

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

fluiddot · 2024-06-18T11:50:20Z

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 · 2024-06-18T15:42:50Z

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

Thank you!

fluiddot · 2024-06-18T16:10:02Z

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 · 2024-06-18T16:14:07Z

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!

danielbachhuber added 3 commits June 9, 2024 12:44

Display the PHP version on Site Settings screen

7e4bcdb

Add a modal for editing the PHP version

f81552f

First pass at saving selected PHP version

7d93cb5

danielbachhuber self-assigned this Jun 9, 2024

danielbachhuber mentioned this pull request Jun 9, 2024

Feature Request: Allow selecting the PHP version #32

Closed

derekblank mentioned this pull request Jun 10, 2024

Update site details to allow user to set PHP version #226

Merged

1 task

Update site details to allow user to set PHP version (#226)

9e856ec

* Add phpVersion to updateSiteDetails * Update tests for changing the PHP version

wojtekn requested a review from a team June 10, 2024 11:06

Abstract available PHP versions to a constant

5006fce

danielbachhuber requested a review from matt-west June 10, 2024 14:51

fluiddot mentioned this pull request Jun 11, 2024

Use PHP constants from wp-now #231

Merged

1 task

fluiddot reviewed Jun 11, 2024

View reviewed changes

fluiddot mentioned this pull request Jun 11, 2024

Add unit tests to cover changing PHP version functionality #233

Merged

1 task

danielbachhuber marked this pull request as ready for review June 11, 2024 18:38

fluiddot approved these changes Jun 12, 2024

View reviewed changes

This was referenced Jun 12, 2024

Remove getPhpVersion hook and IPC handler #239

Merged

Bump default PHP version to 8.1 #240

Merged

fluiddot self-assigned this Jun 13, 2024

fluiddot added 2 commits June 13, 2024 16:25

Bump default PHP version to 8.1 (#240)

f47f515

* Bump default PHP version to `8.1` This change is driven by the fact that version `8.0` already reached its end-of-life by 2024. https://www.php.net/supported-versions.php * Address failure in `createSite` unit test

wojtekn merged commit ab0cc21 into trunk Jun 17, 2024
11 checks passed

wojtekn deleted the improve/php-version-picker branch June 17, 2024 08:59

fluiddot mentioned this pull request Jun 17, 2024

Always show scrollbars in tab panels when content overflows #255

Merged

1 task

fluiddot mentioned this pull request Jun 17, 2024

Pick right PHP version for sites created before introducing the PHP selector #262

Merged

1 task

lajennylove mentioned this pull request Jun 18, 2024

Permission issues using a theme that uses composer #269

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow PHP version to be changed from Site Settings #225

Allow PHP version to be changed from Site Settings #225

danielbachhuber commented Jun 9, 2024 •

edited

Loading

danielbachhuber commented Jun 9, 2024 •

edited

Loading

derekblank commented Jun 10, 2024

matt-west commented Jun 10, 2024

danielbachhuber commented Jun 10, 2024

katinthehatsite commented Jun 10, 2024

matt-west commented Jun 11, 2024

fluiddot left a comment

fluiddot Jun 11, 2024

danielbachhuber commented Jun 11, 2024

fluiddot left a comment

fluiddot Jun 12, 2024

fluiddot Jun 12, 2024

fluiddot Jun 12, 2024

matt-west commented Jun 12, 2024

fluiddot commented Jun 12, 2024

matt-west commented Jun 12, 2024

wojtekn commented Jun 17, 2024

fluiddot commented Jun 17, 2024

lajennylove commented Jun 18, 2024

danielbachhuber commented Jun 18, 2024

fluiddot commented Jun 18, 2024

lajennylove commented Jun 18, 2024

fluiddot commented Jun 18, 2024

lajennylove commented Jun 18, 2024

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

Allow PHP version to be changed from Site Settings #225

Allow PHP version to be changed from Site Settings #225

Conversation

danielbachhuber commented Jun 9, 2024 • edited Loading

Proposed Changes

Testing Instructions

Pre-merge Checklist

danielbachhuber commented Jun 9, 2024 • edited Loading

derekblank commented Jun 10, 2024

matt-west commented Jun 10, 2024

danielbachhuber commented Jun 10, 2024

katinthehatsite commented Jun 10, 2024

matt-west commented Jun 11, 2024

fluiddot left a comment

Choose a reason for hiding this comment

fluiddot Jun 11, 2024

Choose a reason for hiding this comment

danielbachhuber commented Jun 11, 2024

fluiddot left a comment

Choose a reason for hiding this comment

fluiddot Jun 12, 2024

Choose a reason for hiding this comment

fluiddot Jun 12, 2024

Choose a reason for hiding this comment

fluiddot Jun 12, 2024

Choose a reason for hiding this comment

matt-west commented Jun 12, 2024

fluiddot commented Jun 12, 2024

matt-west commented Jun 12, 2024

wojtekn commented Jun 17, 2024

fluiddot commented Jun 17, 2024

lajennylove commented Jun 18, 2024

danielbachhuber commented Jun 18, 2024

fluiddot commented Jun 18, 2024

lajennylove commented Jun 18, 2024

fluiddot commented Jun 18, 2024

lajennylove commented Jun 18, 2024

danielbachhuber commented Jun 9, 2024 •

edited

Loading

danielbachhuber commented Jun 9, 2024 •

edited

Loading