-
Notifications
You must be signed in to change notification settings - Fork 362
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
Open
JanJakes
wants to merge
8
commits into
trunk
Choose a base branch
from
wp-auto-configure
base: trunk
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
fcbc5db
Safely auto-configure "wp-config.php" when some config is missing
JanJakes 1569d23
Implement a token-based WP config transformer
JanJakes cee8ba8
Use WP config transformer in "defineWpConfigConstants"
JanJakes ac1668e
Use WP config transformer to inject required WP config constants
JanJakes 846af18
Improve file naming and organization
JanJakes 3ff280e
Allow configuring default constants in WP config
JanJakes b44e409
Do not use LOCK_EX due to "Exclusive locks are not supported for this…
JanJakes 75d0346
Add CLI tests
JanJakes File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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-sitemode, right? For a new site, we always seem to copy thewp-config-sample.phpfile. If none ofwp-config.phpandwp-config-sample.phpexists, 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
ensureWpConfigmatters only for existing sites (--mode=apply-to-existing-site). That is—you have a site dump in your directory, and maybe thewp-config.phpdoesn'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
ensureWpConfigshould 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 -Ssite export– a WordPress directory structure that requires processing before it can runI'd say
ensureWpConfigis not needed for asiteat 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 -Swould 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-nowor 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.phpfor 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.