felixarntz / wordpressdev

WordPress core development environment based on Lando.
GNU General Public License v2.0
13 stars 0 forks source link

Refactor config #24

Closed westonruter closed 5 years ago

westonruter commented 5 years ago
westonruter commented 5 years ago

The only thing I would like to revert is the manual writing of PHP files - I'm not sure which purpose it serves, I think having actual files to copy is more straightforward.

The reason is that it avoids having additional copies of wp-config.php in the filesystem which confuses users (at least me) when trying to locate the main wp-config.php. At the moment there are three, and with this PR there are two. Actually, I need to try the symlink approach that I initially had in mind.

westonruter commented 5 years ago

ln: failed to create symbolic link 'wp-config.php': File exists

Humm. I'm not getting this. 😕

if the links only resolve in the Docker container, but not the local machine, it would cause unexpected issues

This is why the link explicitly is using a relative path of ../wp-config.php as opposed to an absolute path like /app/public/wp-config.php. This is the approach I'd always take in VVV to ensure I could run WP-CLI inside and outside the VM. Similarly I'd use relative paths when registering installed_path for PHPCS so that I could use it inside and outside the VM. So I think the that the symlink approach should work. I'm just confused as to why you're getting an error.

westonruter commented 5 years ago

If the symlink isn'g going to work, we could also avoid re-adding the config directory by instead doing:

diff --git a/bin/setup-wordpress.sh b/bin/setup-wordpress.sh
index a521a00..eae9bc0 100755
--- a/bin/setup-wordpress.sh
+++ b/bin/setup-wordpress.sh
@@ -23,10 +23,10 @@ fi
 cd "$LANDO_MOUNT/public/core-dev"

 if [[ ! -e "wp-config.php" ]]; then
-    ln -s ../wp-config.php wp-config.php
+    echo "<?php require dirname( __FILE__ ) . '/../wp-config.php';" > wp-config.php
 fi
 if [[ ! -e "wp-tests-config.php" ]]; then
-    ln -s ../wp-tests-config.php wp-tests-config.php
+    echo "<?php require dirname( __FILE__ ) . '/../wp-tests-config.php';" > wp-tests-config.php
 fi

 if [[ ! -e "$LANDO_MOUNT/public/core-dev/vendor" ]]; then

In other words, a Windows symlink 😄

felixarntz commented 5 years ago

@westonruter like the last approach you suggest. Can we go with that?

westonruter commented 5 years ago

Unfortunately, this does not work. WP-CLI doesn't like it at all:

Error: Strange wp-config.php file: wp-settings.php is not loaded directly.

felixarntz commented 5 years ago

Ah right, I noticed that too. In that case I'd prefer going back to having PHP files we copy from config - they could have the .tmpl suffix to not cause confusion. The developer using this shouldn't need to worry about the config directory anyway.

westonruter commented 5 years ago

I found a fix for the WP-CLI problem. I just had to move the requiring of wp-settings.php in c58143a since this is what WP-CLI looks for:

https://github.com/wp-cli/wp-cli/blob/57c2c9649226fe3d99c7bd408d737a63585b3398/php/WP_CLI/Runner.php#L572