New Bedrock Configuration Model #380

austinpray · 2018-08-05T20:41:58Z

tag roots/wp-config 1.0.0 before merge

Problem

The current Bedrock configuration system does not strictly adhere to one of the most important Roots core values:

All environments should match production as close as possible.

Stated another way and scoped specifically for Bedrock:

Environment configuration values should default to production values and only
deviate if absolutely necessary.

This is not how bedrock currently works. the current bedrock configuration process described by the docs in general works like so:

Which seems fine at first. However, upon further examination it falls apart.

The docs describe this intended usage:

The environment configs are required before the main application config so anything in an environment config takes precedence over application.

Note: You can't re-define constants in PHP. So if you have a base setting in application.php and want to override it in production.php for example, you have a few options:

Remove the base option and be sure to define it in every environment it's needed

Only define the constant in application.php if it isn't already defined.

At best the config process:

is not DRY
does not fail safe in situations of an unexpected WP_ENV value.
will silently reject configuration values if a user is not clear on how define()ing constants in php works
is not consistent with our environment parity recommendations

Not DRY

If you want all environments to match production you would have to copy paste the contents of environments/production.php to everywhere except environments/development.php. Obvious DRY violations aside, this is error prone. environments/staging.php can easily get out of sync with environments/production.php and congratulations your staging environment no longer reflects production or vice versa.

Does Not Fail Safe

You could perhaps require_once __dir__ . '/production.php'; in every file except development.php. However now the second Bedrock encounters an unexpected WP_ENV variable (staging-experimental, productoin, or undefined to name a few): Bedrock will fail open and stuff like WP_DEBUG_DISPLAY will default to true (!!!). No web framework should behave this way when undefined values make their way into the system.

Silent Configuration Rejections

In ruby

irb(main):001:0> WP_DEBUG_DISPLAY = true
=> true
irb(main):002:0> WP_DEBUG_DISPLAY = false
(irb):2: warning: already initialized constant WP_DEBUG_DISPLAY
(irb):1: warning: previous definition of WP_DEBUG_DISPLAY was here
=> false

Clearly I am trying to do something bad and the logs will reflect this.

In PHP

php > define('WP_DEBUG_DISPLAY', true);
php > define('WP_DEBUG_DISPLAY', false);
php > var_dump(WP_DEBUG_DISPLAY);
bool(true)

Yikes.

Not Consistent With Roots Recommendations

If we recommend:

All environments should match production as close as possible.

Then why does a user have to take explicit action to make this happen for every environment they define? Why doesn't it just default to this behavior? In the current model production configuration is "opt-in" instead of "opt-out". This is perpendicular to what we recommend.

Solution

"Production" configuration should be specified in its entirety in config/application.php and files in config/environments/*.php should only be used to override production behavior. For instance in development.php you would want to show error messages and define debug flags and stuff.

To do this we should

Collect all the configuration values in application.php as key value pairs in a map
Collect all the configuration values in the desired config/environments/WP_ENV.php file and merge them with the key value pairs collected in application.php
After all configuration values are collected we should apply them to the environment via define()

At every point in every step above we need to protect the users from redefining constants.

Implementation

NOTE: Before your eyes glaze over looking at crazy UML diagrams: go read the code. What we are doing here is very simple and takes less than 100 lines of code.

Lines of Code (LOC)                              127
Comment Lines of Code (CLOC)                      56 (44.09%)
Non-Comment Lines of Code (NCLOC)                 71 (55.91%)

The reason I am going so overboard with explaining the implementation is because I need a second pair of eyes to sign off on the UX I am proposing here. I want to validation on if throwing these exceptions in these places makes sense etc. Also in writing this explanation I have already fixed a couple edgecases.

I have defined these classes:

Who's usage looks like:

To produce:

Exceptions

I throw a ConstantAlreadyDefinedException when a user attempts to enter a situation described in the "Problem" section where PHP would silently reject the configuration value.

I throw a UndefinedConfigKeyException when a user attempts to fetch a value with Config::get(k) that doesn't exist in Config::$configMap. I do this because the control flow is nice and return null is not working because null is a viable configuration value. We don't use Config::get(k) anywhere but it is a part of the API because I can see users needing it.

Backwards Compatibility

All of this Config class madness is opt-in. Nothing stops a user from just going back to using define(). Config::define() is just an alternative API that behaves in a safe and sane way with regards to overriding environment configurations.

Conclusion

I am out of time for today so can't elaborate much further. Interested to discuss this with you guys!

tangrufus · 2018-08-06T06:57:34Z

lib/Bedrock/ConstantAlreadyDefinedException.php

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


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

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?

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

swalkinshaw

This is great 🎉. Couple minor copy questions/suggestions.

What about turning this into a Composer package? It's a bit more overheard, but I might prefer that over including these files in a Bedrock install. It might encourage people to modify the code or put other stuff in lib (which may or may be good).

swalkinshaw · 2018-08-06T14:30:26Z

config/application.php

+
+/**
+ * Safe Debugging Settings
+ * Override these in config/{WP_ENV}.php


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

swalkinshaw · 2018-08-06T14:32:34Z

config/application.php

@@ -1,5 +1,7 @@
 <?php

+use Roots\Bedrock\Config;
+


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.

Yep this needs to be in here

swalkinshaw · 2018-08-06T14:38:39Z

lib/Bedrock/Config.php

+    {
+        if (!array_key_exists($key, self::$configMap)) {
+            $class = self::class;
+            throw new UndefinedConfigKeyException("'$key' has not been set by $class::define('$key', ...)");


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

swalkinshaw · 2018-08-06T15:08:32Z

lib/Bedrock/Config.php

+    private static function defined($key)
+    {
+        if (defined($key)) {
+            $message = "Bedrock aborted trying to redefine constant '$key'";


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.

QWp6t

I do like how the problem has been identified, but I'm not sure if this is the best solution to address it for our purposes. There a few issues that I have with the class; some are minor and some are more significant.

The most significant issue that I have is that it feels overengineered. We're basically just providing a way for developers to set a key-value pair and then override them later and ultimately pass them all to define(). There are already plenty of ways for doing this in PHP, and I'm not sold on why we wouldn't just go with one of those.

An example would be using a simple $config array and array_merge($config, include __DIR__ . '/' . WP_ENV . '.php'); or some such to override values and then at the end just loop over the array and pass the pairs to define(). Developers can work with the array just like they would any other array. This is the simplest solution and should be taken under consideration.

I'm not a fan of the class full of static members. You can just instantiate the class and accomplish the same thing using $config->define() and $config->apply(). I usually only see classes like this take the form of utility classes (such as Illuminate\Support\Str), but that's not what we're doing here. This has all the markings of a regular ol' class instance, but it's unnecessarily being made static instead.

QWp6t · 2018-08-06T19:27:06Z

lib/Bedrock/Config.php

+    {
+        if (defined($key)) {
+            $message = "Bedrock aborted trying to redefine constant '$key'";
+            throw new ConstantAlreadyDefinedException($message);


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.

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

QWp6t · 2018-08-06T19:28:22Z

lib/Bedrock/Config.php

+    /**
+     * @var array
+     */
+    private static $configMap = [];


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

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

austinpray · 2018-08-06T21:42:44Z

Moving this over to https://github.com/roots/wp-config

https://github.com/roots/wp-config

aaemnnosttv · 2018-08-08T16:27:33Z

composer.json

@@ -32,13 +32,19 @@
      "url": "https://wpackagist.org"
    }
  ],
+  "autoload": {
+    "psr-4": {
+      "Roots\\Bedrock\\": "lib/Bedrock"


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

whoops! I'm sloppy haha

aaemnnosttv · 2018-08-08T16:36:02Z

composer.json

  "require": {
    "php": ">=5.6",
    "composer/installers": "^1.4",
    "vlucas/phpdotenv": "^2.0.1",
    "johnpbloch/wordpress": "4.9.8",
    "oscarotero/env": "^1.1.0",
-    "roots/wp-password-bcrypt": "1.0.0"
+    "roots/wp-password-bcrypt": "1.0.0",
+    "roots/wp-config": "dev-master"


+1 for extracting this as a package!

How about ^1.0 instead of dev-master?

Yep! I am going tag 1.0.0 before this is merged

austinpray · 2018-08-10T05:23:59Z

Did roots/wp-config#2

and 1e6ff6c

Anyone have any comments about my comments? lol.

Removing config/environments/production.php makes a lot of sense to me. It makes so sense to do all this and then still provide a place by default to put production specific config. Nothing is stopping people from creating that file but by default it shouldn't be there.

I was debating removing staging.php as well but I will defer to you guys.

swalkinshaw · 2018-08-11T00:18:26Z

@austinpray not a bad idea. Let's consider/do that separately.

swalkinshaw · 2018-08-11T00:19:04Z

Looks good to go once 1.0.0 is done and updated 🚀

mike-source · 2018-09-24T13:48:29Z

Has this change been reflected in the docs?

https://roots.io/bedrock/docs/folder-structure/
https://roots.io/bedrock/docs/configuration-files/

Not sure if this is the best place to mention it, and probably bad practice on my part but I was copying in an old composer.json (from before August 2018, with my preferred packages) and missed the changes.

Just posting here in case anyone else is doing the same (I suspect quite a few people do this with bedrock + composer) and got confused. Was pulling my hair out for a couple hours!

austinpray · 2018-09-24T13:53:39Z

Oh not yet

https://github.com/roots/docs/blob/docs/bedrock/folder-structure.md
https://github.com/roots/docs/blob/docs/bedrock/configuration-files.md

mike-source · 2018-09-24T13:57:30Z

I'd offer to do it myself, but will confess I haven't got my head round the changes yet!

tangrufus · 2018-09-24T17:44:54Z

probably bad practice on my part but I was copying in an old composer.json (from before August 2018, with my preferred packages) and missed the changes.

A simpler solution: Don't copy and paste the JSON; Use $ composer require instead.

mike-source · 2018-09-24T17:50:22Z

@tangrufus hmmm maybe I could write myself a CLI script in future, I needed to add 8+ plugins to my composer.json at once and also add a couple of custom repositories... ACF pro is incredibly annoying to use via composer.

I'll probably just remember to check Bedrock latest changes!

austinpray · 2018-09-24T18:05:01Z

I can walk y'all boyos through the changes if you wanna contribute docs. Would be more fun that way

MikeiLL · 2018-09-24T19:02:50Z

I'd like to contribute, but am also a little glassy-eyed about where this codebase goes. Is it a plugin that would potentially be installed with composer? Would that be $ composer require roots/wp-config?

austinpray · 2018-09-24T19:29:34Z

@MikeiLL (and anyone else reading this) wanna hop on a google hangout later today and I can walk ya through it in like 10 mins? Can record and then share here for posterity

austinpray · 2018-09-24T19:47:49Z

https://doodle.com/poll/mxa2ma2cmdef9aib If y’all want to vote on a time that works

mike-source · 2018-09-24T20:03:35Z

@austinpray I'm UK based so those times are a bit tricky (been a long day!) If something happens definitely share it though I'll take a look. Cheers.

austinpray · 2018-09-24T21:24:54Z

@MikeiLL 7PM CDT work for you? https://www.timeanddate.com/worldclock/fixedtime.html?msg=Bedrock+Chat&iso=20180924T19&p1=70&am=15

MikeiLL · 2018-09-24T21:30:30Z

@austinpray yes see you then.

austinpray · 2018-09-25T00:04:00Z

https://hangouts.google.com/call/4noypIUSvl1ljNNgs47mAAEE

Edit: RIP I totally messed up the recording

austinpray added 6 commits August 4, 2018 18:47

Use static configuration class on top of define()

ddd6e29

Import clobbered docblock

b61ebf1

Add examples to staging

89007a4

Additional safety and idempotence

7cc6b3e

Use imported class name

15ca97c

Don't half-apply config

caa7029

austinpray requested review from QWp6t, swalkinshaw and retlehs and removed request for QWp6t August 5, 2018 20:42

austinpray added the enhancement label Aug 5, 2018

tangrufus reviewed Aug 6, 2018

View reviewed changes

swalkinshaw approved these changes Aug 6, 2018

View reviewed changes

QWp6t requested changes Aug 6, 2018

View reviewed changes

Moves Config and friends to roots/wp-config

ca5728c

https://github.com/roots/wp-config

This was referenced Aug 7, 2018

Making disallow indexing safer #371

Closed

Undo #59 and never prevent indexing or move it to a separate config #368

Closed

QWp6t approved these changes Aug 7, 2018

View reviewed changes

aaemnnosttv reviewed Aug 8, 2018

View reviewed changes

Stray autoload

12de86e

retlehs approved these changes Aug 9, 2018

View reviewed changes

austinpray mentioned this pull request Aug 10, 2018

Change exception text roots/wp-config#2

Merged

austinpray added 2 commits August 10, 2018 00:19

Updates comments

1e6ff6c

Merge branch 'master' of github.com:roots/bedrock into class-static

38efa1c

Comment style

532ed8f

Use wp-config 1.0.0

b4521ab

austinpray merged commit 0042307 into master Aug 11, 2018

austinpray deleted the class-static branch August 11, 2018 03:47

This was referenced Oct 26, 2018

Update multisite docs to reflect new bedrock config api roots/docs#168

Merged

tmp_multisite_constants.php defines constant that may be redefined afterwards roots/trellis#1025

Closed

tangrufus mentioned this pull request Mar 14, 2020

Discussion: Should we encourage Config::get? roots/wp-config#5

Open

4 tasks

austinpray mentioned this pull request Mar 28, 2021

production.php missing? #585

Closed

4 tasks

SteelWagstaff mentioned this pull request Aug 31, 2021

Upstream improvements pressbooks/bedrock#93

Merged

QWp6t mentioned this pull request Nov 10, 2023

Add file to configure production environments #691

Closed

New Bedrock Configuration Model #380

New Bedrock Configuration Model #380

Conversation

austinpray commented Aug 5, 2018 • edited Loading

Problem

Not DRY

Does Not Fail Safe

Silent Configuration Rejections

Not Consistent With Roots Recommendations

Solution

Implementation

Exceptions

Backwards Compatibility

Conclusion

Choose a reason for hiding this comment

austinpray Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swalkinshaw 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

QWp6t 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

austinpray commented Aug 6, 2018

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

austinpray commented Aug 10, 2018

swalkinshaw commented Aug 11, 2018

swalkinshaw commented Aug 11, 2018

mike-source commented Sep 24, 2018 • edited Loading

austinpray commented Sep 24, 2018

mike-source commented Sep 24, 2018

tangrufus commented Sep 24, 2018

mike-source commented Sep 24, 2018

austinpray commented Sep 24, 2018

MikeiLL commented Sep 24, 2018

austinpray commented Sep 24, 2018

austinpray commented Sep 24, 2018

mike-source commented Sep 24, 2018

austinpray commented Sep 24, 2018

MikeiLL commented Sep 24, 2018

austinpray commented Sep 25, 2018 • edited Loading

austinpray commented Aug 5, 2018 •

edited

Loading

austinpray Aug 6, 2018 •

edited

Loading

mike-source commented Sep 24, 2018 •

edited

Loading

austinpray commented Sep 25, 2018 •

edited

Loading