Safely auto-configure wp-config.php when some configuration is missing

[JanJakes]

Motivation for the change, related issues

When a required WordPress configuration is missing, such as the DB_NAME constant, it will be added to wp-config.php on Playground boot in the following form:

...

/*
 * BEGIN: Added by WordPress Playground.
 *
 * WordPress Playground detected that some required WordPress configuration was
 * missing in this file. Since the auto-configure mode was enabled, the missing
 * configuration was automatically added with sensible default values below.
 *
 * It's safe to remove this block and define the missing configuration manually,
 * or you can keep it, as it won't interfere with any existing configuration.
 */
if ( ! defined( 'DB_NAME' ) ) {
	define( 'DB_NAME', 'wordpress' );
}
/* END: Added by WordPress Playground. */

/** Sets up WordPress vars and included files. */
require_once ABSPATH . 'wp-settings.php';

When such a block already exists, it will be first deleted and then injected again, if needed. This also means that when the block is no longer needed (a user defined the required constants), it will be removed.

The default constant values can be configured via --wp-config-default-constants.

Resolves #2530.

Implementation details

Editing WP config for different reasons (default constants, defineWpConfigConsts Blueprint step) has been consolidated into a single Wp_Config_Transformer class.

Testing Instructions (or ideally a Blueprint)

1. Create a new WP site, e.g., using:

   npx nx unbuilt-jspi playground-cli server --mount-before-install ./test-site:/wordpress

2. Edit the ./test-site/wp-config.php file and remove some constants (e.g., DB_NAME).
3. Run the following command:

   npx nx unbuilt-jspi playground-cli server --mount-before-install ./existing-site:/wordpress --skip-wordpress-setup --blueprint empty-blueprint.json --wp-config-default-constants='{"DB_NAME":"my-db-name"}'

4. Check the ./test-site/wp-config.php. It should included the injected config.

Then you can also put the originally removed constant back and rerun step 3. The injected block should disappear again.

[adamziel]

auto-configure mode was enabled

What is the auto-configure mode? There's auto-mount already, do we need another auto concept?

[adamziel]

Also, note Playground defines more constants than just DB_NAME, e.g. WP_HOME and WP_SITEURL . It could be useful to either make the define behavior configurable via bootOptions or remove it from bootWordPress entirely and leave it up to the caller, e.g. via the beforeDatabaseSetup hook. This way Playground web, Playground CLI, and Studio could each do what they need.

[JanJakes]

@adamziel There are multiple nuances here at play, I guess. First, currently, when the DB_NAME constant is missing, we prefix the config with something like <?php define( 'DB_NAME', 'wordpress' ); ?><?php .... That's not very nice and clear, and it doesn't wrap the constant in an if statement, which would mean PHP warnings on some platforms that predefine the constants. So for this part, I think the proposed solution is safer, nicer, and more explicit.

The second part is when this behavior should occur. We discussed throwing an exception when some required constant is missing and adding the --auto-configure parameter to allow this type of injection. But we can still discuss what's the best (e.g., whether this should just happen by default, as it does now with the <?php define( 'DB_NAME', 'wordpress' ); ?><?php ... prefix). There were two discussions about this — a clearer summary here and a longer one here.

Also, note Playground defines more constants than just DB_NAME, e.g. WP_HOME and WP_SITEURL .

Yeah, I think there's an opportunity to unify the behavior of all of these now.

Let us know what you think!

[adamziel]

when the DB_NAME constant is missing, we prefix the config with something like <?php define( 'DB_NAME', 'wordpress' ); ?><?php .... That's not very nice and clear, and it doesn't wrap the constant in an if statement, which would mean PHP warnings on some platforms that predefine the constants. So for this part, I think the proposed solution is safer, nicer, and more explicit.

Yeah I think it would solve for a number of use-cases, maybe for most of them. My only concern is it would make misconfigured environments harder to debug by hiding the "constant already defined" warning. That may be a good trade-off and I think we can wrap the defines in an if and then discuss further.

For that further discussion –

Some environments predefine the constant, others don't. Some sites come with the constant defined, others don't. Some developers would rather see an explicit warning when their environment is misconfigured over getting a constant with a different value. Some runtimes are fine with modifying the config files, others are not. There's a lot of combinations.

I keep thinking why does the DB_NAME problem keeps coming back and one reason that jumps out at me is that we're trying to solve for all these combinations with CLI options. I wonder if putting the runtime in charge would solve this once and for all. The bootWordPress() caller could provide a callback that:

• Knows the current runtime context
• Whether the booted site defines any DB-related constant
• Modifies wp-config.php as needed – or leaves it untouched

[adamziel]

It seems like we can’t have everything so we’re just choosing a set of trade-offs. When we’re importing site with no DB_NAME defined, we can:

• Add a define() call to wp-config.php. Upsides: That’s what WordPress expects. The site will work with php -S and any other PHP server. Downsides: The file is changed (which seems fine on import), we may need to rewrite that file on export (or not, depending on the target!).
• Keep DB_NAME missing. Upsides: wp-config.php remains unchanged. Downsides: The runtime now needs to define DB_NAME so you can’t just run that site via another webserver. The export still needs to rewrite wp-config.php to produce a working site (except for atomic?).
• Add a conditional if(!defined()) { define(); } call. Upsides: It should just work on atomic. Downsides: wp-config.php is changed. Errors are obscured – a conflicting define("DB_NAME") call earlier on won’t trigger any warnings. We may still need to rewrite that config when exporting to a non-atomic, MySQL-based platform.

Some of those upsides and downsides don’t matter as much from Studio perspective, but they are relevant from Playground CLI perspective.

Note that wp-config.php may still need some rewriting if we are importing a site where DB_NAME is already defined (alongside other MySQL credentials). (edited)

Perhaps instead of a hook, we could use a config option after all. It would not solve for everything, only allow the consumer to choose their set of tradeoffs:

--wp-config-missing-constants=define|define-conditionally|do-not-define
    default: define

Every runtime would choose the most appropriate set of trade-offs. Any importing-related pre-processing of the files, e.g. removing any pre-existing constants, would be up to the runtime.

[JanJakes]

@adamziel Funny enough, I went into this issue thinking that we need a new config option, but after discussing and thinking it through, I think it may not be needed, and we could keep it simple and transparent. I'll try to explain why.

I'll start with two ideas that I find important:

1. Looking at the trade-offs, I'd like to rule out the Keep DB_NAME missing. option. I think defining the constant at runtime is obscuring the configuration, and it is non-transparent to the user—they can't see it in the config, they can't get it via WP CLI, and WP CLI and other tools or servers can't use it. On export, it's either missing, or it needs to be injected. In summary, I think it's best to be transparent and explicit and add the configuration to wp-config.php.
2. Additionally, I'd like to rule out any "rewrite on export" logic. What is an export anyway? A user of Playground CLI is free to copy-paste the files on the filesystem at any point, and we can't possibly catch that as an export action. In summary, I think it's best not to try reverting the configuration on export.

---

Considering the reasoning above, we'd be left with two options:

1. Add a define() call to wp-config.php.
2. Add a conditional if(!defined()) { define(); } call to wp-config.php.

Both of these variants have the advantage that they are "the WP way," and they are clear, transparent, and explanatory about what Playground is doing with the configuration. They will work with WP CLI, php -S, and any other PHP server, and the user can copy-paste the config file at any time, and it will just work as expected (including with their SQLite DB).

For both of these options, we can inject clear and explanatory comments. We can also wrap them in a BEGIN/END comment block (as in the PR description) that could be automatically removed when the user defines the constant manually (it's the current PR behavior). In the case of option 2, we can either inject it always or only when it's missing. All of these are just optimizations that can make the experience even better; it would work without them as well.

Now the question is, what is the difference between the two? I'll look at each of these closer, but I'd like to state an important factor beforehand—I suspect we might be ruling out option 1 because Studio wasn't eager to sanitize wp-config.php files on their side (I believe they should do so, because it's not only Playground CLI that may inject a constant that would conflict on push to Atomic—a user may do that as well, for example), so let's also be careful about that.

---

I think the variants 1 and 2 are both good solutions, and they are very close. Here's how I perceive them:

1. Add a define() call to wp-config.php

I think this is a great option. We're adding missing constants in any way, so why not add them the official WP way?

• Upsides: The most "WP way" possible. Clear, transparent, and explanatory definition of what Playground is doing with the configuration. It will work with WP CLI, php -S, and any other PHP server.
• Downsides: The wp-config.php file is modified. Studio needs to sanitize it on push to Atomic (which I think they should do anyway, and WP CLI provides commands for that).

2. Add a conditional if(!defined()) { define(); } call to wp-config.php

I think this one's great too, and it's in some ways safer. It only slightly shifts the upsides and downsides:

• Upsides: Clear, transparent, and explanatory definition of what Playground is doing with the configuration. It will work with WP CLI, php -S, and any other PHP server. No-op when everything is correctly defined (plus, as an "optimization," we don't need to inject anything in that case). No need to modify the config on push to Atomic.
• Downsides: The wp-config.php file is modified.

@adamziel I think I understand the reasoning behind your upsides and downsides for this option, but from a slightly different point of view, for me, it's more like this:

Errors are obscured – a conflicting define("DB_NAME") call earlier on won’t trigger any warnings.

I actually don't perceive this as obscuring errors. I understand the if(!defined()) { define(); } more like saying "if you don't have this constant defined, this is what Playground uses as the default". If you define it, fine. It it conflicts with an existing SQLite database, you'll get an Incorrect database name. The database was created with name '%s', but '%s' is used in the current session. error. That seems also fine.

For this case:

We may still need to rewrite that config when exporting to a non-atomic, MySQL-based platform.

Maybe some platforms use some strange configuration, but I think this shouldn't be needed. We'll add these fallbacks at the end of the wp-config.php file, after the "stop editing" line, and after where WP CLI injects constants. And since Studio doesn't pull & push to other platforms, having an error on manual import is also a viable edge case, as it will be clear from the wp-config.php what actually failed.

Which of the options is better, then? To be honest, I'm not sure. I like both, and I think until now, I was slightly favoring option 2 (not sure if under the influence of the Studio use case). I like that option 2 is a bit "safer" and that it states It's safe to remove this block and define the missing configuration manually, or you can keep it, as it won't interfere with any existing configuration., and when the user adds the constants, the block would be automatically removed.

---

In summary, I think it's perfectly fine to modify wp-config.php, as we're actually doing just that—modifying the site configuration. In fact, I think it's much better than trying to hide that we're doing that.

Additionally, I think it's fine to leave any additional complexity to external tools (e.g., if you need a site without some specific configuration, you should be handling that on your side anyway).

And if we ever need a CLI option, I think it should be just a flag switching between "error on missing configuration" vs. "inject missing configuration." Doing more feels pretty complex and possibly out of scope for Playground to me.

Finally, I would like to warn against any options that I think are obscuring what's happening or can cause unreliability:

• Inject configuration on import, remove it on export. This will obscure what configuration was used in the exported site, and it will only work with the "official" export anyway.
• Inject configuration on server boot, remove it on shutdown. This is just a worse variant of the previous one. It will obscure what configuration was used immediately when the server is shut down. It will strangely appear there when the server is running, and on a crash, it would just stay there.

[adamziel]

@JanJakes Thank you for your thorough argument. I'm on board. Let's proceed with a conditional define(). Do we need to create any migration path for pre-existing sites?

[adamziel]

I'm not sure about the naming – we now have ensureWpConfig that defines some constants and autoConfigureWpConfig that also defines some constants. Let's use this opportunity to consolidate all the constants-related logic and come up with a good, descriptive name.

[JanJakes]

@adamziel This should be ready for review. I will also check it once again with a fresh mind, but I think it should cover all that we need at the moment.

[JanJakes]

Are these good defaults? Someone was complaining that it was ugly (which I kind of agree with), but if we want to use different defaults, I think we'd need to do that also for new installs (where we only copy the wp-config-sample.php without any modifications at the moment).

[adamziel]

Yeah I think we're good to change these for new installs as long as we don't break any existing sites.

[JanJakes]

These could also get random values by default (Is manual WP install doing something like that? 🤔).

[adamziel]

Good question! AFAIR Playground was doing that up until this point – even if we got a wp-config file with these default values. Let's confirm what's the WordPress behavior and make it consistent.

One thing to be careful about – AFAIR if we change password salt on a running website, noone will be able to log in as the result of sha1($user_input . $salt) will change.

[adamziel]

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.

[JanJakes]

@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 the wp-config-sample.php file. If none of wp-config.php and wp-config-sample.php exists, this will be a noop.

[adamziel]

I'm not convinced – the comment above this method call even says:

This is needed, because some WordPress backups and exports may not include
definitions for some of the necessary constants.

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);.

[JanJakes]

@adamziel

Perhaps you're right and that only matters for new sites, after all?

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 the wp-config.php doesn't have some required configuration. This dump is not specified by Blueprint (v2), we can only apply some Blueprint to it.

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);.

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 invalid wp-config.php, then it would be an invalid Blueprint.

With both v1 and v2, this can also happen:

1. You have some constants missing in your existing site dump (e.g., Studio).
2. We add the missing constants; a DB is provided (e.g., by Studio).
3. Blueprint v1 or v2 is applied to an existing site.
4. If the Blueprint messed up the config, we won't try to fix it at this stage.

When you create a new site from a Blueprint (even from a ZIP with missing config constants), then it wouldn't touch it.

[adamziel]

ensureWpConfig matters only for existing sites

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 via php -S
• site export – a WordPress directory structure that requires processing before it can run

I'd say ensureWpConfig is not needed for a site at all. We're either running that site or making a change to it, e.g. by installing a plugin. For an site 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?

[JanJakes]

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

[JanJakes]

We need something like this for new sites, too, but I guess the existing blueprint runners take care of that.

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

[adamziel]

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.

[JanJakes]

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

[adamziel]

If we move it to PHP toolkit, is there a way to reuse it from there in Studio and Blueprints v1?

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.

[adamziel]

We never need to do that now – you can pass these values via env:

php.run({
	code: `<?php echo getenv("VALUE"); `,
	env: { VALUE: 'yay!' }
});

[JanJakes]

@adamziel Ah, nice! Will that preserve types and work with arrays, etc.?

[adamziel]

no, you need to json-serialize it

[adamziel]

great API

[adamziel]

I'm confused why this test passes given my comment above.

[JanJakes]

@adamziel Calling ensureWpConfig with a new site (no existing wp-config.php nor wp-config-sample.php) will be a noop, so this passes and creates a new wp-config.php.