-
Notifications
You must be signed in to change notification settings - Fork 346
Safely auto-configure wp-config.php
when some configuration is missing
#2539
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
base: trunk
Are you sure you want to change the base?
Changes from all commits
fcbc5db
1569d23
cee8ba8
ac1668e
846af18
3ff280e
b44e409
75d0346
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ import { exec } from 'node:child_process'; | |
import { | ||
mkdirSync, | ||
readdirSync, | ||
readFileSync, | ||
writeFileSync, | ||
symlinkSync, | ||
unlinkSync, | ||
|
@@ -131,6 +132,129 @@ describe.each(blueprintVersions)( | |
expect(response.text).toContain(oldestSupportedVersion); | ||
}); | ||
|
||
test('should add missing constants to wp-config.php', async () => { | ||
const tmpDir = await mkdtemp( | ||
path.join(tmpdir(), 'playground-test-') | ||
); | ||
|
||
const args: RunCLIArgs = { | ||
...suiteCliArgs, | ||
command: 'server', | ||
'mount-before-install': [ | ||
{ | ||
hostPath: tmpDir, | ||
vfsPath: '/wordpress', | ||
}, | ||
], | ||
mode: 'create-new-site', | ||
}; | ||
|
||
const newSiteArgs: RunCLIArgs = | ||
version === 2 | ||
? { | ||
...args, | ||
'experimental-blueprints-v2-runner': true, | ||
mode: 'create-new-site', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm confused why this test passes given my comment above. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @adamziel Calling |
||
} | ||
: args; | ||
|
||
const existingSiteArgs: RunCLIArgs = | ||
version === 2 | ||
? { | ||
...args, | ||
'experimental-blueprints-v2-runner': true, | ||
mode: 'apply-to-existing-site', | ||
} | ||
: { | ||
...args, | ||
skipWordPressSetup: true, | ||
}; | ||
|
||
// Create a new site so we can load it as an existing site later. | ||
cliServer = await runCLI(newSiteArgs); | ||
const wpConfigPath = path.join(tmpDir, 'wp-config.php'); | ||
let wpConfig = readFileSync(wpConfigPath, 'utf8'); | ||
expect(wpConfig).toContain( | ||
"define( 'DB_NAME', 'database_name_here' );" | ||
); | ||
expect(wpConfig).not.toContain( | ||
'BEGIN: Added by WordPress Playground.' | ||
); | ||
expect(wpConfig).not.toContain( | ||
'END: Added by WordPress Playground.' | ||
); | ||
|
||
// Remove the "DB_NAME" constant. | ||
writeFileSync( | ||
wpConfigPath, | ||
wpConfig.replace("'DB_NAME'", "'UNKNOWN_CONSTANT'") | ||
); | ||
wpConfig = readFileSync(wpConfigPath, 'utf8'); | ||
expect(wpConfig).not.toContain( | ||
"define( 'DB_NAME', 'database_name_here' );" | ||
); | ||
|
||
// Use the existing site and confirm the missing constant is added. | ||
cliServer = await runCLI(existingSiteArgs); | ||
wpConfig = readFileSync(wpConfigPath, 'utf8'); | ||
expect(wpConfig).toContain( | ||
"define( 'DB_NAME', 'database_name_here' );" | ||
); | ||
expect(wpConfig).toContain('BEGIN: Added by WordPress Playground.'); | ||
expect(wpConfig).toContain('END: Added by WordPress Playground.'); | ||
|
||
// Ensure the "--wp-config-default-constants" argument works as well. | ||
try { | ||
cliServer = await runCLI({ | ||
...existingSiteArgs, | ||
wpConfigDefaultConstants: { | ||
DB_NAME: 'test_database_name', | ||
CUSTOM_CONSTANT: 'test_custom_constant', | ||
}, | ||
}); | ||
// eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
} catch (_) { | ||
// The boot will fail due to incorrect database name, | ||
// but the wp-config.php file should be updated. | ||
} | ||
|
||
wpConfig = readFileSync(wpConfigPath, 'utf8'); | ||
expect(wpConfig).not.toContain( | ||
"define( 'DB_NAME', 'database_name_here' );" | ||
); | ||
expect(wpConfig).toContain( | ||
"define( 'DB_NAME', 'test_database_name' );" | ||
); | ||
expect(wpConfig).toContain( | ||
"define( 'CUSTOM_CONSTANT', 'test_custom_constant' );" | ||
); | ||
expect(wpConfig).toContain('BEGIN: Added by WordPress Playground.'); | ||
expect(wpConfig).toContain('END: Added by WordPress Playground.'); | ||
|
||
// Ensure the injected constants are removed when no longer needed. | ||
writeFileSync( | ||
wpConfigPath, | ||
wpConfig.replace("'UNKNOWN_CONSTANT'", "'DB_NAME'") | ||
); | ||
await runCLI(existingSiteArgs); | ||
wpConfig = readFileSync(wpConfigPath, 'utf8'); | ||
expect(wpConfig).toContain( | ||
"define( 'DB_NAME', 'database_name_here' );" | ||
); | ||
expect(wpConfig).not.toContain( | ||
"define( 'DB_NAME', 'test_database_name' );" | ||
); | ||
expect(wpConfig).not.toContain( | ||
"define( 'CUSTOM_CONSTANT', 'test_custom_constant' );" | ||
); | ||
expect(wpConfig).not.toContain( | ||
'BEGIN: Added by WordPress Playground.' | ||
); | ||
expect(wpConfig).not.toContain( | ||
'END: Added by WordPress Playground.' | ||
); | ||
}); | ||
|
||
test('should run blueprint', async () => { | ||
cliServer = await runCLI({ | ||
...suiteCliArgs, | ||
|
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.
There's a good chance the WordPress site doesn't exist at this point. We'll need to run this at the right time during the Blueprint v2 lifecycle, e.g. after resolving the site. We could use this
ensureWpConfig()
call ATM or inject an initial Blueprint step.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.
@adamziel I think that all of this logic makes sense only with the
apply-to-existing-site
mode, right? For a new site, we always seem to copy thewp-config-sample.php
file. If none ofwp-config.php
andwp-config-sample.php
exists, this will be a noop.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.
I'm not convinced – the comment above this method call even says:
Which suggests we'll see data structures looking like existing sites that don't have these constants defined. Although, in those cases, I suppose the database won't be configured so they're not going to boot anyway. Hm. 🤔 Perhaps you're right and that only matters for new sites, after all?
Even then, we need to do it at the right time of the Blueprint v2 lifecycle – we'll typically download an unzip a WordPress release as a part of
await this.runBlueprintV2(args);
.Uh oh!
There was an error while loading. Please reload this page.
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.
@adamziel
Actually, what I want to say is that
ensureWpConfig
matters only for existing sites (--mode=apply-to-existing-site
). That is—you have a site dump in your directory, and maybe thewp-config.php
doesn't have some required configuration. This dump is not specified by Blueprint (v2), we can only apply some Blueprint to it.I guess the question is whether the
ensureWpConfig
should be a Playground feature, or a Blueprint v2 feature. My thinking at the moment is that it is a runtime (Playground) feature. If a Blueprint v2 references or creates an invalidwp-config.php
, then it would be an invalid Blueprint.With both v1 and v2, this can also happen:
When you create a new site from a Blueprint (even from a ZIP with missing config constants), then it wouldn't touch it.
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.
I'd like to learn more about the types of import you had in mind in here.
My thinking is we're talking about two separate data structures:
site
– a WordPress directory structure configured so that it's ready to run viaphp -S
site export
– a WordPress directory structure that requires processing before it can runI'd say
ensureWpConfig
is not needed for asite
at all. We're either running that site or making a change to it, e.g. by installing a plugin. For ansite export
, however, we need to do some processing before we're able to run it. I assume most site exports were produced by an export tool that has an accompanying import tool.We can process exports compatible with the Blueprint v2 bundle format, but I don't think there's any generalized way we can process an arbitrarily mangled site export – perhaps we should just bale out on it. Are we doing that now? Or would that be a change that would also break Studio?
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.
@adamziel All of this makes me wonder if the most "correct" solution for Playground may indeed be to only throw an error and do nothing else when a constant is missing, just like
php -S
would do.Maybe this is not on the table now, and it would surely mean moving this logic to Studio, but I do wonder if there are other use cases where auto-backfilling missing constants is needed, or whether we've just gotten here due to some historic behaviors in
wp-now
or somewhere. Originally, the fallbacks were done at runtime via an MU plugin, but now, when the config is processed statically, the logic could maybe live in Studio as well. I guess ultimately, the question is whether this is needed in Playground in a broader sense.Otherwise, as for the next steps, I agree with your suggestions 👍
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.
@adamziel As for this part, I think we currently only copy
wp-config-sample.php
for new sites. Maybe we should update it to use some more reasonable defaults (e.g., for the salts and stuff).Uh oh!
There was an error while loading. Please reload this page.
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.
Yeah perhaps that's just a historic wp-now issue. It seems like we're really processing a site import, aren't we? Perhaps we could export a repair function for Studio to call on their own if needed. Or just move it there entirely by proposing a Studio PR.
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.
@adamziel I agree. If we don't see any other use cases, maybe we really should consider contributing this to Studio. Thinking this through:
The WP_Config_Transformer logic will be needed in Blueprints v2 (and v1 runner as well, while it's around). If we move it to PHP toolkit, is there a way to reuse it from there in Studio and Blueprints v1?
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.
There probably is, but let's not overcomplicate it for now and just do a copy&paste until we set up some infrastructure for reusing PHP toolkit.