Build/test tools: copy the local environment configuration synchronously - #12697
Build/test tools: copy the local environment configuration synchronously#12697lancewillett wants to merge 3 commits into
Conversation
…sly. On a fresh checkout, the asynchronous copy of .env.example can race with dotenv loading .env. This leaves configuration values unset for the process. Use copyFileSync() to ensure .env exists before dotenv loads it. Fixes #65716. Props lance.willett@automattic.com
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
Pull request overview
Ensures the local environment startup script reliably loads configuration on fresh checkouts by making the initial .env.example → .env creation step synchronous before dotenv.config() executes, avoiding a race that can leave env vars unset.
Changes:
- Replace async
copyFile()withcopyFileSync()for initial.envcreation. - Wrap the copy operation in a
try/catchto preserveCOPYFILE_EXCLbehavior (do not overwrite existing.env).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| copyFileSync( '.env.example', '.env', constants.COPYFILE_EXCL ); | ||
| } catch ( e ) { | ||
| console.log( '.env file already exists. .env.example was not copied.' ); | ||
| }); | ||
| } |
There was a problem hiding this comment.
This is good feedback. A better approach is probably to check whether the file already exists at the destination and copy if it doesn't, the try/catch should be safe to remove then, any errors that surface are useful information.
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
|
Reviewing. |
Luc45
left a comment
There was a problem hiding this comment.
Looks good. Fix is minimal and precise.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
tools/local-env/scripts/start.js:14
- The try/catch treats every copy failure as “.env already exists” and then continues. That can silently mask real problems (e.g. missing
.env.example, permission errors), leaving the process running with an unexpected or empty configuration. Consider only swallowing the expectedEEXISTerror fromCOPYFILE_EXCL, and rethrow anything else.
try {
copyFileSync( '.env.example', '.env', constants.COPYFILE_EXCL );
} catch ( e ) {
console.log( '.env file already exists. .env.example was not copied.' );
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
tools/local-env/scripts/start.js:14
- The catch block currently treats any
copyFileSync()failure as “.env already exists”. WithCOPYFILE_EXCL,EEXISTis expected when.envis present, but other errors (e.g. missing.env.example(ENOENT), permissions (EACCES), etc.) should be surfaced to avoid masking real setup problems. Consider only swallowingEEXISTand rethrowing other errors.
try {
copyFileSync( '.env.example', '.env', constants.COPYFILE_EXCL );
} catch ( e ) {
console.log( '.env file already exists. .env.example was not copied.' );
}
Trac ticket: https://core.trac.wordpress.org/ticket/65716
Summary
On a fresh checkout,
start.jsbegins copying.env.exampleasynchronously, then immediately loads.env. The read can finish first, leaving configuration values unset for that process.This changes the initial copy to
copyFileSync(), ensuring.envexists beforedotenv.config()runs. Existing.envfiles remain untouched becauseCOPYFILE_EXCLis preserved.Branch behavior
The race is latent on trunk because the old-PHP authentication helper is no longer present.
On 6.9, missing
LOCAL_DB_VERSIONcan make PHP 7.2 or 7.3 select--default-authentication-plugin=mysql_native_password. MySQL 8.4 removed this option and exits during startup.This should be backported to 6.9 after landing on trunk.
Testing
.envbefore Docker startup.npm run env:installcompleted.