Skip to content
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

New Bedrock Configuration Model #380

Merged
merged 12 commits into from
Aug 11, 2018
5 changes: 5 additions & 0 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@
"url": "https://wpackagist.org"
}
],
"autoload": {
"psr-4": {
"Roots\\Bedrock\\": "lib/Bedrock"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be necessary anymore now that Config has been moved to a package.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoops! I'm sloppy haha

}
},
"require": {
"php": ">=5.6",
"composer/installers": "^1.4",
Expand Down
71 changes: 43 additions & 28 deletions config/application.php
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
<?php

use Roots\Bedrock\Config;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We didn't have it before either, but I think a comment block explaining the config a little bit would be useful. I know we have docs, but I think it's important in here as well.

Mostly around explaining how the application config defaults to production settings and the env specific files should only be used when necessary to change those.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep this needs to be in here

/** @var string Directory containing all of the site's files */
$root_dir = dirname(__DIR__);

Expand All @@ -26,54 +28,67 @@
*/
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'));
Config::define('WP_HOME', env('WP_HOME'));
Config::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);
Config::define('CONTENT_DIR', '/app');
Config::define('WP_CONTENT_DIR', $webroot_dir . Config::get('CONTENT_DIR'));
Config::define('WP_CONTENT_URL', Config::get('WP_HOME') . Config::get('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', '');
Config::define('DB_NAME', env('DB_NAME'));
Config::define('DB_USER', env('DB_USER'));
Config::define('DB_PASSWORD', env('DB_PASSWORD'));
Config::define('DB_HOST', env('DB_HOST') ?: 'localhost');
Config::define('DB_CHARSET', 'utf8mb4');
Config::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'));
Config::define('AUTH_KEY', env('AUTH_KEY'));
Config::define('SECURE_AUTH_KEY', env('SECURE_AUTH_KEY'));
Config::define('LOGGED_IN_KEY', env('LOGGED_IN_KEY'));
Config::define('NONCE_KEY', env('NONCE_KEY'));
Config::define('AUTH_SALT', env('AUTH_SALT'));
Config::define('SECURE_AUTH_SALT', env('SECURE_AUTH_SALT'));
Config::define('LOGGED_IN_SALT', env('LOGGED_IN_SALT'));
Config::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);
Config::define('AUTOMATIC_UPDATER_DISABLED', true);
Config::define('DISABLE_WP_CRON', env('DISABLE_WP_CRON') ?: false);
// Disable the plugin and theme file editor in the admin
Config::define('DISALLOW_FILE_EDIT', true);
// Disable plugin and theme updates and installation from the admin
Config::define('DISALLOW_FILE_MODS', true);

/**
* Safe Debugging Settings
* Override these in config/{WP_ENV}.php
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these "safe" and why do only these have a comment about overriding? Every setting can be overridden technically.

*/
Config::define('WP_DEBUG_DISPLAY', false);
Config::define('SCRIPT_DEBUG', false);
ini_set('display_errors', 0);

$env_config = __DIR__ . '/environments/' . WP_ENV . '.php';

if (file_exists($env_config)) {
require_once $env_config;
}

Config::apply();

/**
* Bootstrap WordPress
Expand Down
13 changes: 9 additions & 4 deletions config/environments/development.php
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
<?php
/** Development */
define('SAVEQUERIES', true);
define('WP_DEBUG', true);
define('SCRIPT_DEBUG', true);
/** Configuration Overrides for Development */

use Roots\Bedrock\Config;

Config::define('SAVEQUERIES', true);
Config::define('WP_DEBUG', true);
Config::define('SCRIPT_DEBUG', true);

ini_set('display_errors', 1);
9 changes: 3 additions & 6 deletions config/environments/production.php
Original file line number Diff line number Diff line change
@@ -1,7 +1,4 @@
<?php
/** Production */
ini_set('display_errors', 0);
define('WP_DEBUG_DISPLAY', false);
define('SCRIPT_DEBUG', false);
/** Disable all file modifications including updates and update notifications */
define('DISALLOW_FILE_MODS', true);
/** Configuration Overrides for Production */

use Roots\Bedrock\Config;
12 changes: 6 additions & 6 deletions config/environments/staging.php
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<?php
/** Staging */
ini_set('display_errors', 0);
define('WP_DEBUG_DISPLAY', false);
define('SCRIPT_DEBUG', false);
/** Disable all file modifications including updates and update notifications */
define('DISALLOW_FILE_MODS', true);
/** Configuration Overrides for Staging */

use Roots\Bedrock\Config;

// Config::define('WP_DEBUG', true);
// ini_set('display_errors', 1);
108 changes: 108 additions & 0 deletions lib/Bedrock/Config.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
<?php

namespace Roots\Bedrock;

/**
* Class Config
* @package Roots\Bedrock
*/
class Config
{
/**
* @var array
*/
private static $configMap = [];
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, I'm not a fan of private properties or methods. I'd suggest using protected instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 changed over at https://github.com/roots/wp-config


/**
* @param string $key
* @param $value
* @throws ConstantAlreadyDefinedException
*/
public static function define($key, $value)
{
self::defined($key) or self::$configMap[$key] = $value;
}

/**
* @param string $key
* @return mixed
* @throws UndefinedConfigKeyException
*/
public static function get($key)
{
if (!array_key_exists($key, self::$configMap)) {
$class = self::class;
throw new UndefinedConfigKeyException("'$key' has not been set by $class::define('$key', ...)");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe: "'$key' has not been defined. Use $class::define('$key', ...)"

}

return self::$configMap[$key];
}

/**
* @param string $key
*/
public static function remove($key)
{
unset(self::$configMap[$key]);
}

/**
* define() all values in $configMap and throw an exception if we are attempting to redefine a constant.
*
* We throw the exception because a silent rejection of a configuration value is unacceptable. This method fails
* fast before undefined behavior propagates due to unexpected configuration sneaking through.
*
* ```
* define('BEDROCK', 'no');
* define('BEDROCK', 'yes');
* echo BEDROCK;
* // output: 'no'
* ```
*
* vs.
*
* ```
* define('BEDROCK', 'no');
* Config::define('BEDROCK', 'yes');
* Config::apply();
* // output: Fatal error: Uncaught Roots\Bedrock\ConstantAlreadyDefinedException ...
* ```
*
* @throws ConstantAlreadyDefinedException
*/
public static function apply()
{
// Scan configMap to see if user is trying to redefine any constants.
// We do this because we don't want to 'half apply' the configMap. The user should be able to catch the
// exception, repair their config, and run apply() again
foreach (self::$configMap as $key => $value) {
try {
self::defined($key);
} catch (ConstantAlreadyDefinedException $e) {
if (constant($key) !== $value) {
throw $e;
}
}
}

// If all is well, apply the configMap ignoring entries that have already been applied
foreach (self::$configMap as $key => $value) {
defined($key) or define($key, $value);
}
}

/**
* @param string $key
* @return bool
* @throws ConstantAlreadyDefinedException
*/
private static function defined($key)
{
if (defined($key)) {
$message = "Bedrock aborted trying to redefine constant '$key'";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I think we can be a little more clear about what's going on here. Something like:

Config aborted trying to define '$key' . The constant has already been defined.

throw new ConstantAlreadyDefinedException($message);
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're trying to replicate Ruby's behavior, then you can trigger a warning with

trigger_error($message, E_USER_WARNING);

It might be helpful since it would give developers a full list of constants that they're attempting to redefine instead of failing one at a time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The user should be able to recover from the exception without incurring a warning though. Lemme think about this

}

return false;
}
}
13 changes: 13 additions & 0 deletions lib/Bedrock/ConstantAlreadyDefinedException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<?php

namespace Roots\Bedrock;

/**
* Class ConstantAlreadyDefinedException
* This should be thrown when a user attempts to define() a constant that has already been defined
* @package Roots\Bedrock
*/
class ConstantAlreadyDefinedException extends \RuntimeException
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use LogicException instead?

Because LogicException is

Exception that represents error in the program logic. This kind of exception should lead directly to a fix in your code.

https://secure.php.net/manual/en/class.logicexception.php

Same goes to UndefinedConfigKeyException

Copy link
Contributor Author

@austinpray austinpray Aug 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! Thank you for taking a look at this.

I understand where you are coming from when suggesting \LogicException. However, I am still going to advocate for \RuntimeException.

In general when trying to determine what exception class to extend I always look at what other exceptions subclass it. Let's look at the exception hierarchy with some cherry-picked examples:

  • LogicException (extends Exception): Exception that represents error in the program logic. This kind of exception should lead directly to a fix in your code.
    • BadFunctionCallException:
    • BadMethodCallException
    • DomainException
    • InvalidArgumentException
    • LengthException
    • OutOfRangeException: Exception thrown when an illegal index was requested. This represents errors that should be detected at compile time.
  • RuntimeException (extends Exception): Exception thrown if an error which can only be found on runtime occurs.
    • OutOfBoundsException: Exception thrown if a value is not a valid key. This represents errors that cannot be detected at compile time.
    • OverflowException
    • RangeException
    • UnderflowException
    • UnexpectedValueException

The fact that the runtime has been polluted with unexpected constants is something that cannot be detected at compile time. Especially since we dynamically build this configuration based on environment variables.

Take a look at the get method of \DS\Map: https://secure.php.net/manual/en/ds-map.get.php. This throws an OutOfBoundsException (extends RuntimeException) if the key could not be found and a default value was not provided.

Lastly consider the following completely valid use case:

JankyHomebrewDBClient.php

if (!defined('DB_PORT')) {
  define('DB_PORT', 3306)
}

class JankyHomebrewDBClient {
  // ...
}

config/application.php

// ...
Config::define('DB_PORT', env('DB_PORT'));
// ...
Config::apply();

one-off-db-migration-script.php

// I just want to connect to the DB I don't need WordPress
require_once __DIR__ . '/vendor/autoload.php';
require_once __DIR__ . '/config/application.php';

$client = new JankyHomebrewDBClient(/* ... */);

This will throw a ConstantAlreadyDefinedException at runtime. I don't necessarily consider this a logical error, since JankyHomebrewDBClient can never tell if it is going to define DB_PORT at compile time.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! This reply should go into textbooks as "best code review reply example".
Thanks!!!

{

}
8 changes: 8 additions & 0 deletions lib/Bedrock/UndefinedConfigKeyException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?php

namespace Roots\Bedrock;

class UndefinedConfigKeyException extends \RuntimeException
{

}