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

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

Closed
rfay opened this Issue Sep 19, 2017 · 22 comments

Comments

Projects
None yet
8 participants
@rfay
Member

rfay commented Sep 19, 2017

What happened (or feature request):

Currently, we generate a settings.php or wp-config.php if one doesn't exist, and we have some explicit support for settings.local.php. However, we don't handle all cases, and we don't communicate to the user what the situation for their particular site.

I'll speak just in Drupal terms here but we need to document and notify the user what is happening if:

  • They have a settings.php that does include settings.local.php
  • They have a settings.php that does not include settings.local.php
  • They have no settings.php file (and we generate it for them)

Also we need to be clear about ownership of the settings.php and settings.local.php; we may want to have a settings.ddev.php as well for ddev-only settings.

And our generated settings.php should include settings.local.php. It does not currently.

In general: Try for flexible and clearly communicated management of settings files, even though different sites manage these differently and different platforms like Pantheon manage them differently.

What you expected to happen:

How to reproduce this:

Anything else do we need to know:

Related source links or issues:

#407 - Provide a warning if settings.local.php not included

@ultimike

This comment has been minimized.

ultimike commented Mar 8, 2018

Big +1 to a settings.ddev.php file for all the stuff that DDEV generates and currently puts in settings.local.php.

I always put my own local environment settings in settings.local.php including settings for:

  • Stage File Proxy
  • Environment Indicator
  • All of the stuff from Drupal 8's example.settings.local.php

When using DDEV, I have to create a settings.local2.php for all of my stuff, otherwise DDEV will overwrite it.

@ultimike

This comment has been minimized.

ultimike commented Mar 8, 2018

Another other option would be for DDEV to not overwrite the entire settings.local.php file - just its own section.

@rickmanelius rickmanelius added this to the v0.16.0 milestone Mar 19, 2018

@rickmanelius

This comment has been minimized.

Contributor

rickmanelius commented Mar 19, 2018

@ultimike Technically if you remove the #ddev-generated label in settings.local.php, it will no longer attempt to overwrite it if you re-run config. This will be a change in v0.16.0 as a result of #692, which creates the file during config instead of an import-db. We'll leave this open and link because this will be something that will block the v0.16.0 from a docs perspective.

@rickmanelius rickmanelius self-assigned this Mar 19, 2018

@ultimike

This comment has been minimized.

ultimike commented Mar 22, 2018

After reading #692 I'm still not 100% sure that will solve the issue for everyone. I can envision and edge case where someone re-runs ddev config (either on purpose or by accident) and blows away any customizations they have in settings.local.php.

I'm still of the mind that DDEV should only rewrite its own section of settings.local.php on ddev config and not the entire file.

@rfay

This comment has been minimized.

Member

rfay commented Mar 23, 2018

@ultimike ddev will never ever touch a settings file except one that it generated and that has its own signature in it. There's a big warning about that in every generated file.

/**
 #ddev-generated: Automatically generated Drupal settings.php file.
 ddev manages this file and may delete or overwrite the file unless this comment is removed.
 */
@rickmanelius

This comment has been minimized.

Contributor

rickmanelius commented Mar 27, 2018

With #752 in place, I believe we can close this out for now and modify based on end-user feedback if this continues to be an issue. @ultimike I know you mentioned that this might not solve the issue, but if the #ddev-generated comment is removed, I'm curious what your experience is after that.

@ultimike

This comment has been minimized.

ultimike commented Mar 29, 2018

Sorry - been busy the last couple of days updating client sites 😄

I'll play with this a bit (I just updated to 0.16.0) and report back.

@ultimike

This comment has been minimized.

ultimike commented Apr 8, 2018

So, creating a brand new local Drupal 8 site (based on https://github.com/drupal-composer/drupal-project/) using DDEV leads to a web/sites/default/settings.local.php file with only the DDEV-injected stuff in it.

This is okay, but I'm going to be picky and always want a bit more. One issue with how things work in 0.16.0 is that while the settings.local.php file is created with the database connection information, by default the conditional include at the bottom of settings.php is commented out by default, so the user is still prompted for the database credentials during install 😢

In my ideal situation, the following would happen during/after ddev config:

  1. The web/sites/example.settings.local.php file would be copied/renamed to web/sites/default/settings.local.php
  2. A new web/sites/default/settings.ddev.php file would be created with the DDEV-injected stuff.
  3. A conditional PHP include would be added to either the settings.php or settings.local.php that brings in the settings.ddev.php file.

This way, the DDEV stuff is off in its own little sandbox with a file namespaced as such and developers can utilize the settings.local.php file normally without worrying about messing up the DDEV stuff.

-mike

@ultimike

This comment has been minimized.

ultimike commented Apr 14, 2018

I just repeated the process, here's the steps I took to achieve what I proposed in my previous comment after running ddev config:

mv web/sites/default/settings.local.php web/sites/default/settings.ddev.php
cp web/sites/example.settings.local.php web/sites/default/settings.local.php

Edit settings.php and uncomment:

if (file_exists($app_root . '/' . $site_path . '/settings.local.php')) {
  include $app_root . '/' . $site_path . '/settings.local.php';
}

and then add the following:

if (file_exists($app_root . '/' . $site_path . '/settings.ddev.php')) {
  include $app_root . '/' . $site_path . '/settings.ddev.php';
}

That last bit can be added to the bottom of either settings.local.php or settings.php - I'm not sure if there's any pros/cons of doing it either way.

-mike

@ultimike

This comment has been minimized.

ultimike commented Apr 14, 2018

The other alternative would be to do all this stuff in an imaginary (but future?) post-config DDEV hook...

@rickmanelius

This comment has been minimized.

Contributor

rickmanelius commented Apr 14, 2018

Going to look at this again in the upcoming sprint.

@rickmanelius rickmanelius reopened this Apr 14, 2018

@rickmanelius rickmanelius modified the milestones: v0.16.0, v0.18.0 Apr 14, 2018

@rickmanelius rickmanelius modified the milestones: v0.18.0, v0.19.0 May 8, 2018

@rfay rfay assigned andrewfrench and unassigned rickmanelius May 14, 2018

@jenlampton

This comment has been minimized.

jenlampton commented May 18, 2018

I'd also prefer the ddev specific settings live in a settings.ddev.php file and allow me to keep all my other things in the settings.local.php file, as usual. For Backdrop, the inclusion of settings.local.php is already uncommented, so an additional section to include settings.ddev.php could always be written to the end of the file in the same way.

@rfay

This comment has been minimized.

Member

rfay commented Jun 7, 2018

After team meeting today Rick has given his go ahead with this as I think it's been outlined here:

  1. If not settings.php, create a settings.php that does nothing but (conditionally) include the settings.ddev.php
  2. If there is a settings.php, detect if it already has our conditional settings.ddev.php include in it. If it does not have the conditional settings.ddev.php include, add that to the bottom of the settings.php (even though that's touching the user's file)
  3. Always create a settings.ddev.php (unless the user has one that we're not managing). This will have the contents that we've used in the past in settings.local.php
  4. settings.ddev.php should be able to detect if it's already been included and exit in that case.
  5. Add to sites/default a .gitignore that ignores settings.ddev.php (UNLESS there's already a .gitignore in that directory)
  6. Wordpress and Backdrop done the same.
  7. TYPO3 we think we don't have to do anything, but it would be worth asking the powers that be and running this by them

We know that there's some risk here, and this might have to be improved over time.

It will be lovely if we can get reviews on this PR from some of the people who have chimed in here and in d.o slack.

@rfay rfay changed the title from User documentation/support: settings.php/settings.local.php support to Always create settings.ddev.php, Add conditional include to user's settings.php Jun 7, 2018

@ultimike

This comment has been minimized.

ultimike commented Jun 7, 2018

@rfay For step 1 above, I'd suggest, "If not settings.php, create a settings.php from default.settings.php that does nothing but (conditionally) include the settings.ddev.php"

I can live with having to manually copy example.settings.local.php to settings.local.php :)

Glad this is getting some attention!

-mike

@rfay rfay added this to the v1.0.0 milestone Jun 26, 2018

@alkymst

This comment has been minimized.

Collaborator

alkymst commented Jun 27, 2018

@rfay I want to throw out there that for WordPress and the use of Constants for it's config values. We would want to add the WP DDEV config before any values are set, if we need to override any default settings that already exist in the wp-config.php.

It's pretty standard to have a conditional check in your production wp-config.php in WP, so you can over-write the value for local or staging.

/** The name of the database for WordPress */
if (!defined('DB_NAME'))
	define( 'DB_NAME', 'db' );

If this issue is purely for additional settings then adding it the end works for WP as planned.

Environment Based WP Config Examples:
https://abandon.ie/notebook/wordpress-configuration-for-multiple-environments
https://github.com/studio24/wordpress-multi-env-config

@alkymst

This comment has been minimized.

Collaborator

alkymst commented Jun 27, 2018

It relates to what I was doing over here - #932

@rfay

This comment has been minimized.

Member

rfay commented Jun 27, 2018

@alkymst so what we'd prefer to do is be able to override anything (trusted host patterns, whatever) that might normally be set in a prod environment. On Drupal settings, you can do that with an include at the bottom. You're saying that's not possible in WP? In general, we want to make only one surgical change to the user's settings file, and put all our needed stuff in settings.ddev.php or equivalent. When we generate a settings file, we're in control, and we don't ever want them to edit it, so we don't need conditionals or the like. When we do that here, we'll be generating a settings file that does nothing but include settings.ddev.php

@alkymst

This comment has been minimized.

Collaborator

alkymst commented Jun 27, 2018

@rfay I'm saying WP made a really bad design decision that the community has had to build weird workarounds that are not really standard. So Yes, it's not possible in the same way in WP. The closest we will get is env vars written to .env which has become the best way to handle it for WP, much like the TYPO3 way of handling it. Laravel does the same thing, but by default.

@alkymst

This comment has been minimized.

Collaborator

alkymst commented Jun 27, 2018

<?php

/** @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'));

/**
 * Custom Content Directory
 */
define('CONTENT_DIR', '/app');
define('WP_CONTENT_DIR', $webroot_dir . CONTENT_DIR);
define('WP_CONTENT_URL', WP_HOME . CONTENT_DIR);

/**
 * 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_';

/**
 * Authentication Unique Keys and Salts
 */
define('AUTH_KEY', env('AUTH_KEY'));
define('SECURE_AUTH_KEY', env('SECURE_AUTH_KEY'));
define('LOGGED_IN_KEY', env('LOGGED_IN_KEY'));
define('NONCE_KEY', env('NONCE_KEY'));
define('AUTH_SALT', env('AUTH_SALT'));
define('SECURE_AUTH_SALT', env('SECURE_AUTH_SALT'));
define('LOGGED_IN_SALT', env('LOGGED_IN_SALT'));
define('NONCE_SALT', env('NONCE_SALT'));

/**
 * Custom Settings
 */
define('AUTOMATIC_UPDATER_DISABLED', true);
define('DISABLE_WP_CRON', env('DISABLE_WP_CRON') ?: false);
define('DISALLOW_FILE_EDIT', true);

/**
 * Bootstrap WordPress
 */
if (!defined('ABSPATH')) {
    define('ABSPATH', $webroot_dir . '/wp/');
}
@jeffsheltren

This comment has been minimized.

Contributor

jeffsheltren commented Jul 18, 2018

+1 on this approach from the Drupal perspective. Thanks!

@rickmanelius rickmanelius referenced this issue Jul 18, 2018

Closed

v1.0.0 Release Checklist Due 2018-07-19 #950

8 of 8 tasks complete

@rickmanelius rickmanelius modified the milestones: v1.0.0, v1.1.0 Jul 24, 2018

@rickmanelius

This comment has been minimized.

Contributor

rickmanelius commented Jul 30, 2018

@rfay I believe this one is complete as per Drupal/Backdrop behavior and we have other open PRs and issues to address WP. If you'd like to keep open, feel free. I believe this one is complete as specified.

@rfay

This comment has been minimized.

Member

rfay commented Aug 14, 2018

This is complete! It was released with no drama in v1.0.0

@rfay rfay closed this Aug 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment