Update WordPress settings management, fixes #756, fixes #1156 #932

alkymst · 2018-06-18T17:51:49Z

The Problem/Issue/Bug:

#756
#1156

How this PR Solves The Problem:

Adds checks for isWordpressApp.
It properly checks for WordPress config files and doesn't overwrite them if they are user managed.
Instructs the user on how to complete configuration if a user-managed wp-config.php file exists.

Manual Testing Instructions:

Base WordPress Install

From a new WordPress install, without a wp-config.php file, run ddev config
Ensure that ddev has written both a wp-config.php and a wp-config-ddev.php
Ensure that specific ddev database connection information exists in wp-config-ddev.php
Ensure that wp-config.php contains other required non-ddev-specific configurations

A Previously Configured WordPress Install

In the same directory, edit wp-config.php to remove the #ddev-generated signature
Edit wp-config-ddev.php in some noticeable way
Run ddev config
Ensure that ddev warns the user that a user-managed wp-config.php already exists and must be edited to include the PHP snipped written to the terminal
Ensure that ddev does not indicate that configuration is complete or that the user can run ddev start to start the project
Ensure that wp-config.php has not been overwritten
Ensure that wp-config-ddev.php has been overwritten

WordPress-in-a-directory

From a new WordPress-in-a-directory install, run ddev config
Ensure that ddev can detect the correct app type (wordpress)
Ensure that wp-config.php and wp-config-ddev.php have been written to the docroot
Ensure that wp-config.php includes the appropriate ABSPATH value

Release/Deployment notes:

The DDEV docs have been updated to reflect the changes made here.

CLAassistant · 2018-06-18T17:51:57Z

All committers have signed the CLA.

andrewfrench

At @alkymst's request, I ran through the functional changes here and thought I'd capture the results in a review.

I was able to confirm the following behavior starting with a clean Wordpress 4.9.6 installation:

Run ddev config
No errors or warnings are generated
Remove #ddev-generated from wp-config.php
Run ddev config
A prompt to edit the now user-maintained wp-config.php to include wp-config-ddev.php is presented to the user
Remove #ddev-generated from wp-config-ddev.php
Run ddev config
A warning is presented stating settings files already exist and are being managed by the user

rfay · 2018-06-25T18:40:19Z

pkg/ddevapp/wordpress.go

@@ -184,7 +286,7 @@ func setWordpressSiteSettingsPaths(app *DdevApp) {
 	settingsFileBasePath := filepath.Join(app.AppRoot, app.Docroot)
 	var settingsFilePath, localSettingsFilePath string
 	settingsFilePath = filepath.Join(settingsFileBasePath, "wp-config.php")
-	localSettingsFilePath = filepath.Join(settingsFileBasePath, "wp-config-local.php")
+	localSettingsFilePath = filepath.Join(settingsFileBasePath, "wp-config-ddev.php")


Will this break any existing projects? If so, should we even worry about that? Might as well change the variable name to ddevSettingsFilePath, your editor should be able to do this easily and quickly.

I can rename the var localSettingsFilePath to ddevSettingsFilePath, that makes sense.

It will not break anything necessarily, because additional/environment configs requires manual work in wp-config.php to be included in the first place. Before these changes when DDEV created wp-config-local.php it was not be used unless a user added it by hand.

So wp-config-local.php is a non-standard WP way to handle multiple environments. If it exist, and we didn't create it. It's being used for a different local setup most likely. The user has to do work to include and additional configs by hand in the wp-config.php. That's why I felt for users trying out DDEV with an existing site, this naming convention made more sense. I believe passed on past PR's I have sen from @cweagans I feel other team members felt the same way.

Environment Based WP Config Examples:
https://abandon.ie/notebook/wordpress-configuration-for-multiple-environments
https://jonsuh.com/blog/configure-wordpress-for-multiple-environments/
https://github.com/studio24/wordpress-multi-env-config
https://github.com/Abban/WordPress-Config-Bootstrap/blob/master/wp-config.php
https://gist.github.com/ahaywood/c11f1d756521c3edee9a822c0b9cccc7

I'm totally open to including both or using wp-config-local.php. Ideally we use env vars.

rfay · 2018-06-25T18:42:30Z

pkg/ddevapp/wordpress.go

@@ -194,6 +296,10 @@ func isWordpressApp(app *DdevApp) bool {
 	if _, err := os.Stat(filepath.Join(app.AppRoot, app.Docroot, "wp-login.php")); err == nil {
 		return true
 	}
+	// TODO: Add wildcard or ENV var to make more flexible, ie wordpress/


Please add a comment to explain why this stanza exists. Why is wp/wp-login.php important to us? Why does its existence mean we return a true? If we're about to return a false, how do we explain to people why we're doing that?

I'll add better notes as to why we are doing the additional check.

We are looking for WP in a sub-directory, the earlier check was looking for it only in the docroot. Ideally it's a config settings we can control. All I'm adding is a check for WordPress in wp, but it could be in wordpress or whatever they want to call it by defining WP_SITEURL.

define('WP_SITEURL', 'https://www.sitename.com/wp);

rfay · 2018-06-25T18:48:32Z

pkg/ddevapp/wordpress.go

+if (file_exists($docroot . '/wp-config-ddev.php')) {
+	require_once $docroot . '/wp-config-ddev.php';
+}
+
 // ** MySQL settings - You can get this info from your web host ** //


I think this comment is incorrect, true? We're generating this file. It has to have the right stuff in it doesn't it? And doesn't that mean we can skip the "if" statements below, and just define('DB_NAME', 'db') for example? Simpler code is better, so if we're generating and we know we can use simpler code, we should.

Basically, in a generated wp-config.php, none of the below can happen. And we would never want them to be overriding DB_NAME in any way. So all the below doesn't work correctly. But you may need to teach me some more.

We don't want this file pushed to any other environment therefor it only exist when running locally. That is why we are conditionally including it. Most users will not version their wp-config.php if they do they are 100% using it for production settings, and conditionally loading environment specific configs. Now if we generate the wp-config.php I agree we wouldn't want to include it, however if we generate the wp-config-ddev.php we need to tell the user to include it, or include it for them.

I don't think you understood the comment. I was referring to the "MySQL settings - You can get this info from your web host". We are actually providing this information here, there need be no reference to a web host.

As a general strategy: Every single thing we generate is intended to be used only in the context of ddev, it's not for general-purpose WP work of any kind. So when we generate, we may explain why but never ask people to change anything.

rfay · 2018-06-25T18:50:16Z

pkg/ddevapp/wordpress.go


 /** Database Charset to use in creating database tables. */
-define('DB_CHARSET', 'utf8mb4');
+define( 'DB_CHARSET', 'utf8mb4' );


utf8mb4 is already defined at the DB level (my.cnf), do we need it here?

I'm not 100% sure, it's something that I have always set with DB creds.

* DB settings */ define('DB_NAME', env('DB_NAME')); define('DB_USER', env('DB_USER')); define('DB_PASSWORD', env('DB_PASSWORD')); define('DB_HOST', env('DB_HOST') ?: 'localhost'); define('DB_CHARSET', 'utf8mb4'); define('DB_COLLATE', ''); $table_prefix = env('DB_PREFIX') ?: 'wp_';

It's likely irrelevant, because people who are going to break as a result will break both ways. (Long indexes often break on utf8mb4, if the db wasn't previously set up that way). So we can leave this in.

rfay · 2018-06-25T18:50:53Z

pkg/ddevapp/wordpress.go


 /** The Database Collate type. Don't change this if in doubt. */
-define('DB_COLLATE', '');
+define( 'DB_COLLATE', '' );


We don't want people changing anything in this file, right? So let's not make comments about "changing". Let me know if I misunderstand.

I agree. It's carried over from the default WP config

rfay · 2018-06-25T18:52:22Z

pkg/ddevapp/wordpress.go


 /**
 * For developers: WordPress debugging mode.
 */
-define('WP_DEBUG', false);
+ if (!defined('WP_DEBUG'))
+	define('WP_DEBUG', false);


Wouldn't we want this always enabled?

Only in development. Also some plugins have warnings and devs want to turn them off so it doesn't mess up their site when they are CSS'ing.

This is development. ddev is for development. In general, I think we should default to having debug on. So unless huge objections, let's set it to TRUE.

rfay · 2018-06-25T18:53:01Z

pkg/ddevapp/wordpress.go

+	define('WP_HOME', '{{ $config.DeployURL }}');
+
+/** WP_ENV */
+define('WP_ENV', getenv('DDEV_ENV_NAME') ? getenv('DDEV_ENV_NAME') : 'production');

 /* That's all, stop editing! Happy blogging. */


Quite sure this comment isn't useful in ddev context

WP_ENV is a fairly standard way of dealing with environment based settings. WP plugins use it, Trellis, etc.

/** @var string Directory containing all of the site's files */ $root_dir = dirname(__DIR__); /** @var string Document Root */ $webroot_dir = $root_dir . '/web'; /** * Expose global env() function from oscarotero/env */ Env::init(); /** * Use Dotenv to set required environment variables and load .env file in root */ $dotenv = new Dotenv\Dotenv($root_dir); if (file_exists($root_dir . '/.env')) { $dotenv->load(); $dotenv->required(['DB_NAME', 'DB_USER', 'DB_PASSWORD', 'WP_HOME', 'WP_SITEURL']); } /** * Set up our global environment constant and load its config first * Default: production */ define('WP_ENV', env('WP_ENV') ?: 'production'); $env_config = __DIR__ . '/environments/' . WP_ENV . '.php'; if (file_exists($env_config)) { require_once $env_config; } /** * URLs */ define('WP_HOME', env('WP_HOME')); define('WP_SITEURL', env('WP_SITEURL'));

Again, you seem to have reacted to something completely different from the context of the comment. The comment was on the line " /* That's all, stop editing! Happy blogging. */"

Nobody should be editing. And we don't need a comment like this if they were.

rfay · 2018-06-25T18:53:39Z

pkg/ddevapp/wordpress.go

+
+const (
+	settingsHelpText = `
+	// Include local settings if it exists.


Rather than "local settings" should read "ddev settings file" right?

I would agree.

rfay · 2018-06-25T18:55:05Z

pkg/ddevapp/wordpress.go

+	if strings.Contains(filepath.Base(settingsFilePath), "wp-config-ddev.php") {
+		output.UserOut.Printf("Looks like you are maintaining a wp-config.php as part of your repo. To get DDEV to connect to the DB add the below snippet to your wp-config.php at the very top of your config. \n %s \n You will also want to add wp-config-ddev.php to your .gitignore", settingsHelpText)
+	} else {
+		output.UserOut.Printf("")


What's the use of this file? Printf("") does nothing, I don't think. Might want Println().

Yeah, agree. Totally new to Golang.

rfay · 2018-06-25T18:56:26Z

pkg/ddevapp/wordpress.go

 }

 // WriteWordpressConfig dynamically produces valid wp-config.php file by combining a configuration
 // object with a data-driven template.
 func WriteWordpressConfig(wordpressConfig *WordpressConfig, filePath string) error {
-	tmpl, err := template.New("wordpressConfig").Funcs(sprig.TxtFuncMap()).Parse(wordpressTemplate)
+


Please explain the difference between wordpressTemplate and worpressTemplateSimple, and the strategy behind them. If you do that elsewhere I missed it.

Well all that really matters to get the site working on DDEV is what's in worpressTemplateSimple. The wordpressTemplate is more closely mirroring an end to end wp-config. worpressTemplateSimple is a way to set what DDEV needs to connect.

What makes this weird is really how WP handles config settings. Being constants makes it really weird to "overwrite" settings. Environment variables being the best option really.

https://github.com/studio24/wordpress-multi-env-config

Looks like you changed from wordpressTemplateSimple to wordpressTemplateDdev?

We really don't want more than one type of template do we?

AFAICT there should just be wordpressTemplateDdev, and it should not include the "If" statements, those should just be defines.

rfay · 2018-06-25T19:23:28Z

Hmm, I seem to have skipped by the comment form in "request changes" somehow, but basically the need is to recognize that we're generating this file and thus the comments and actions taken need to recognize that.

I also note this stanza at the end of the file:

wp-settings.php is typically included in wp-config.php. This check ensures it is not
included again if this file is written to wp-config-local.php.
*/
if (basename(__FILE__) == "wp-config.php") {
	require_once(ABSPATH . '/wp-settings.php');
}

That I don't think makes sense does it?

alkymst · 2018-06-28T04:09:21Z

@rfay I have made some of the requested updates and responded to all of your review questions. Let me know when you can take it for a spin again.

rfay

Sorry, this isn't doing what I expect at this point. My problem is with the generated settings file(s).

If no wp-config.php, ddev generates one, but it wants to include a wp-config-ddev.php. In that case... the wp-config-ddev.php should be generated and included, and all the stuff in wp-config.php should be unnecessary...
All of wp-config.php has if !defined() blocks, but it's "#ddev-generated",. meaning we own it, so there's no need to do that, it only makes the file ambiguous IMO.
The wp-config.php retains inexplicable comments like "/* That's all, stop editing! Happy blogging. */". But this is a ddev-generated, ddev-managed file. It should be the simplest possible file, and should not be suggesting edits (or carrying baggage from default settings files).
The last stanza seems not to be correct, and also implies that the file would be renamed to wp-config-local.php, which really must not happen. Over all it's assuming that this is a user-managed settings file, and it's not.

Sets up WordPress vars and included files.

wp-settings.php is typically included in wp-config.php. This check ensures it is not
included again if this file is written to wp-config-local.php.

if (basename(FILE) == "wp-config.php") {
require_once(ABSPATH . '/wp-settings.php');
}

For simplicity, here's the generated wp-config-ddev and wp-config: https://gist.github.com/rfay/906e28cb6b29631847b1a7bcd3e165a6

(BTW, I understand this is hard, and that you just landed in this without much context or background. Let's either just keep working on the understanding, or back off and just do the fix in the actual OP #756 )

We do very much appreciate having a WP expert in our midst. Just trying to get it to fit into the overall usage environment of ddev here.

rfay · 2018-07-09T23:02:40Z

Moved this to v1.1.0, since it's not going to get short-term attention.

rfay · 2018-08-24T15:23:44Z

I rebased this. It still shows what @alkymst was thinking, still things to be addressed, but we ought to be able to drive it home.

rfay

So I think we agree that this

Solves the problem as we originally understood it.
Is a first step
Doesn't directly support either Bedrock or wordpress/skeleton, and maybe some other things.

So let's pull it (after rebase and tests succeeding)

And let's not announce it at this point. We can let our favorite devs that use this exact technique use it over the course of this release and hopefully add explicit support for particular wp variants before the next release.

Does that work?

andrewfrench · 2018-10-10T21:07:34Z

Agreed on all points, we can use this as a starting point for wider support of WordPress variants.

- Remove wordpressPostImportDBAction() - Renamed function to wordpressGetRelativeAbsPath() - Update wordpressGetRelativeAbsPath to use filepath.Glob() - Improve error handling when finding ABSPATH - Emit specific instructions to the user for config editing

…exit code

[ci skip]

dclear requested a review from rfay June 18, 2018 17:54

rfay changed the title ~~WP Settings updates for - #756~~ WP Settings updates and make sure unmanaged settings are respected, fixes #756 Jun 19, 2018

rickmanelius self-requested a review June 20, 2018 20:51

andrewfrench approved these changes Jun 22, 2018

View reviewed changes

rfay requested changes Jun 25, 2018

View reviewed changes

alkymst mentioned this pull request Jun 27, 2018

Always create settings.ddev.php, Add conditional include to user's settings.php #468

Closed

alkymst force-pushed the master branch from 7d116ef to 627ae9c Compare June 27, 2018 22:02

rfay requested changes Jul 1, 2018

View reviewed changes

rfay assigned alkymst Jul 9, 2018

rfay added this to the v1.1.0 milestone Jul 9, 2018

rickmanelius modified the milestones: v1.1.0, v1.2.0 Jul 30, 2018

rfay force-pushed the master branch from 37b5469 to 9241225 Compare July 31, 2018 03:38

dclear modified the milestones: v1.1.0, v1.2.0 Aug 15, 2018

rfay force-pushed the master branch from 9241225 to 766a0d5 Compare August 24, 2018 01:26

andrewfrench closed this Aug 29, 2018

andrewfrench force-pushed the master branch from 766a0d5 to 4bd1348 Compare August 29, 2018 20:18

andrewfrench reopened this Aug 29, 2018

andrewfrench assigned andrewfrench and unassigned alkymst Sep 5, 2018

dclear removed the request for review from rickmanelius September 11, 2018 20:25

dclear removed this from the v1.2.0 milestone Sep 12, 2018

rfay mentioned this pull request Oct 10, 2018

Add a test stanza for WordPress in subdirectory in ddevapp_test.go #1167

Closed

rfay approved these changes Oct 10, 2018

View reviewed changes

alkymst and others added 24 commits October 10, 2018 15:12

Add check for isWordPressApp to look inside a wp sub-folder

2214295

settings updates for WP ddev#756

382cace

fix error

de334ff

fix error in tests

956a1ba

small updates to naming of variables, and added some notes

a17ca6b

Fix simple rebase fail

81c2d4b

Update Wordpress settings management to reflect Drupal/Backdrop flow.

2c7c5d4

Update test expectations.

92bf511

Move wp-settings.php include to wp-config.php.

c60d039

Update WP settings file logic, update settings template.

1c691e2

WP-in-subdir configurations are now supported by ddev config.

951c5fb

Fix TestAppTypeDetection()

bc46dde

Fix TestWriteConfig()

1a1c77a

Write other non-ddev config when no wp-config.php exists

025636a

Fix linting

7705ddc

Upgrade gofmt.

e71cb19

Address comments

cafabee

- Remove wordpressPostImportDBAction() - Renamed function to wordpressGetRelativeAbsPath() - Update wordpressGetRelativeAbsPath to use filepath.Glob() - Improve error handling when finding ABSPATH - Emit specific instructions to the user for config editing

Fail instead of error to prevent success message and return non-zero …

58c37dc

…exit code

Revert back to simpler error handling.

dedd0bf

A nonexistent wp-settings.php file should not be an error.

60b03e5

Fix test expectations

aaf6fbd

Fix PHP syntax error.

cbe6c51

Update WordPress config management docs

18e03ac

[ci skip]

Update output help text

0187943

andrewfrench force-pushed the master branch from c8b6876 to 0187943 Compare October 10, 2018 21:13

andrewfrench merged commit c350d63 into ddev:master Oct 11, 2018

andrewfrench mentioned this pull request Dec 12, 2018

Wordpress settings creation improvements #742

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update WordPress settings management, fixes #756, fixes #1156 #932

Update WordPress settings management, fixes #756, fixes #1156 #932

alkymst commented Jun 18, 2018 •

edited by andrewfrench

CLAassistant commented Jun 18, 2018 •

edited

andrewfrench left a comment

rfay Jun 25, 2018

alkymst Jun 27, 2018

rfay Jun 25, 2018

alkymst Jun 27, 2018

rfay Jun 25, 2018

alkymst Jun 27, 2018

rfay Jul 1, 2018

rfay Jun 25, 2018

alkymst Jun 27, 2018

rfay Jul 1, 2018

rfay Jun 25, 2018

alkymst Jun 27, 2018

rfay Jun 25, 2018

alkymst Jun 27, 2018

rfay Jul 1, 2018

rfay Jun 25, 2018

alkymst Jun 27, 2018

rfay Jul 1, 2018

rfay Jun 25, 2018

alkymst Jun 27, 2018

rfay Jun 25, 2018

alkymst Jun 27, 2018

rfay Jun 25, 2018

alkymst Jun 27, 2018

rfay Jul 1, 2018

rfay commented Jun 25, 2018

alkymst commented Jun 28, 2018

rfay left a comment •

edited

rfay commented Jul 9, 2018

rfay commented Aug 24, 2018

rfay left a comment

andrewfrench commented Oct 10, 2018

Update WordPress settings management, fixes #756, fixes #1156 #932

Update WordPress settings management, fixes #756, fixes #1156 #932

Conversation

alkymst commented Jun 18, 2018 • edited by andrewfrench

The Problem/Issue/Bug:

How this PR Solves The Problem:

Manual Testing Instructions:

Related Issue Link(s):

Release/Deployment notes:

CLAassistant commented Jun 18, 2018 • edited

andrewfrench left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfay commented Jun 25, 2018

alkymst commented Jun 28, 2018

rfay left a comment • edited

Choose a reason for hiding this comment

rfay commented Jul 9, 2018

rfay commented Aug 24, 2018

rfay left a comment

Choose a reason for hiding this comment

andrewfrench commented Oct 10, 2018

alkymst commented Jun 18, 2018 •

edited by andrewfrench

CLAassistant commented Jun 18, 2018 •

edited

rfay left a comment •

edited