[Breaking] Refactor PHP.ini management, remove php.setPhpIniPath() and php.setPhpIniEntry() - #1423
Conversation
adamziel
changed the title
…ld (without the SQLite integration plugin) Uses Playground CLI to produce a minified WordPress build, replacing the custom bash-based WordPress installation flow that relied on wget, unzip, sed, wp-cli etc. This is to dogfood Playground CLI and put the boot protocol to a practical use. With this PR, CLI Playground produces a minified WordPress version that is later used to boot the web Playground. Depends on #1423 #1423 #1425 ## Testing instructions Rebuild the latest WordPress with ```shell rm -rf packages/playground/wordpress-builds/public/wp-6.5 npx nx bundle-wordpress:major-and-beta playground-wordpress-builds ``` Then run npm run dev and confirm the local Playground loads without any issues. Related: #1398
Replaces a series of custom steps meant to unzip and configure WordPress with passinf the build zip as a preferredVersion. To test, go to the WordPress PR previewer locally and try previewing one of the recent PRs. Confirm the installed WordPress versions is indeed not the latest stable. Do not merge until these PRs are merged: * #1423 * #1424 * #1425 * #1426 * #1427
bgrgicak
left a comment
bgrgicak
left a comment
There was a problem hiding this comment.
I left a few comments, but it looks really good otherwise.
| await php.writeFile(PHP_INI_PATH, stringify(ini)); | ||
| } | ||
|
|
||
| export async function withPHPIniValues( |
There was a problem hiding this comment.
This could use some comments. The name didn't make it clear to me that this runs a single callback with a set of custom PHP ini values.
There was a problem hiding this comment.
Good call! I'll document that in a follow-up PR.
| $log_file = WP_CONTENT_DIR . '/debug.log'; | ||
| error_reporting(E_ALL); | ||
| define('ERROR_LOG_FILE', $log_file); | ||
| ini_set('error_log', $log_file); |
There was a problem hiding this comment.
Why are we leaving this ini_set here?
There was a problem hiding this comment.
I'm not sold on storing logs in /wordpress/wp-content/debug.log by default. This choice affects a local mount and may not exist before WordPress is installed. I'm not sure what to do with this yet.
There was a problem hiding this comment.
This choice affects a local mount and may not exist before WordPress is installed.
Let's move it to internals. Others can always override it.
| ); | ||
|
|
||
| if (!this.fileExists(PHP_INI_PATH)) { | ||
| this.writeFile( |
There was a problem hiding this comment.
Why don't we use setPhpIniEntries here?
There was a problem hiding this comment.
Because PHP_INI_PATH does not exist yet and setPhpIniEntries would fail. We could perhaps change setPhpIniEntries semantics to "create the file if it's missing", but this feels off – I want BasePHP to guarantee the file will be there later on and get a clear error if it isn't found at a later stage.
There was a problem hiding this comment.
And if we ever want to do type-checking of known PHP ini settings, we could just write the blank file, and use setPhpIniEntries to write those initial settings.
3 participants
What is this PR doing?
Refactors PHP.ini management:
php.setPhpIniEntry()methods that only worked correctly when called before initializing the PHP runtime./internal/shared/php.ini. The developer is welcome to override that file as needed, but they can't customize the path. Once changing the path becomes important, let's add that feature at the Boot Protocol level.getPhpIniEntries(),setPhpIniEntries(), andwithPHPIniValues()helpers that work at any point of the PHP runtime lifecycle.php_wasm.ctobase-php.tswhere it can be customized without rebuilding the wasm module.What problem is it solving?
Working with php.ini is highly problematic as you could never be sure whether your
setPhpIni*call would be effective. Then, at the API level, thesetPhpIni*methods were also excluded from some TypeScript types but not from the others.Remaining work
Testing Instructions
Confirm the CI tests pass.