Problem/Motivation

In #1833516: Add a new top-level global for settings.php - move things out of $conf we added the new $settings array for storing largely static values that only live in settings.php and are generally needed very early in bootstrap, making them inappropriate for the config or state systems. However right now, there is no good way to get those values into settings.php other than manually adding them. This makes issues like upgrade path very difficult, as some of these values existed in the variable system in D7.

In order to make that easier we should add the ability to write these values into drupal_rewrite_settings(). I haven't looked at that function in too much depth so I'm not really sure how hard it is going to be to accomplish this.

Proposed resolution

The current code dumps whole arrays like $database = array('default' => array(.... Instead let's dump

$settings['database']['default']['default']['driver'] = 'mysql';
$settings['database']['default']['default']['database'] = 'drupal';

This, in itself is not terribly problematic. However, the comment blocks in settings.php are helpful and we want the settings to stick to those blocks. So if the default contains $conf['system.performance']['css']['gzip'] = FALSE; and a new TRUE is written for css compression, ideally this row would be replaced. This needs a little complicated code but nothing a small state machine can't handle.

Remaining tasks

Reviews needed. Tests are being written.

User interface changes

None.

API changes

drupal_rewrite_settings() now expects arrays with .value, .required, .comment keys instead of just value, required, comment because otherwise you couldn't dump $foo['bar']['value'] = 2;. To dump this, do

$settings['bar']['value'] = array(
  '.value' => 2,
  '.required' => TRUE,
);

Note that while this is an API change the previous was completely undocumented and very likely unused outside of core so while perhaps a change notice is warranted, it's not too important.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

Status: Active » Needs review
FileSize
10.32 KB
chx’s picture

Issue summary: View changes

Updated issue summary.

chx’s picture

FileSize
13.89 KB

Added tests.

damiankloip’s picture

Nice!

+++ b/core/includes/install.core.incundefined
@@ -1103,12 +1103,12 @@ function install_settings_form_submit($form, &$form_state) {
+    '.value'    => array('default' => array('default' => $form_state['storage']['database'])),

I guess using '.value' et all is the best choice, over '_value' or something? Some values may want to use underscores in their keys? Otherwise it would be nice, as it would be consistent with the symfony routing etc.. using _ to prefix keys.

+++ b/core/includes/install.incundefined
@@ -231,6 +307,86 @@ function drupal_rewrite_settings($settings = array()) {
+function _drupal_rewrite_settings_is_scalar($type, $value) {

This helper could do with a description.

+++ b/core/includes/install.incundefined
@@ -231,6 +307,86 @@ function drupal_rewrite_settings($settings = array()) {
+  if (!empty($variable['.comment'])) {
+    $return .= ' // ' . $variable['.comment'];
+  }

You may have already thought about this, I don't know, but would it be better to move the comment dumping before the var export and making a newline. So we get:

// Comment
$settings['comments']['meh'] = TRUE;
+++ b/core/modules/system/lib/Drupal/system/Tests/System/SettingsRewriteTest.phpundefined
@@ -0,0 +1,105 @@
+ * Definition of Drupal\system\Tests\System\SettingsRewriteTest.

(Sorry) we need "Contains \Drupal\system\Tests\System\SettingsRewriteTest" now.

Also, should we also add a test that uses an array as the '.value' too?

chx’s picture

FileSize
14.28 KB

The dot was chosen because CMI bans dots in the config keys so while we might see an underscore in the $conf arrays, there will be no dots in that array. It is still possible that some keys dumped by this function will contain dots because, after all, $settings does not ban them but still starting with a dot is extremely unlikely even in $settings.

Bugger, I wrote a description but it's true it's in @return :)

> Would it be better to move the comment dumping before the var export?

Nope. Current patch:

$conf['system.performance']['css']['gzip'] = TRUE; // comment

Suggested:

$conf['system.performance']['css']['gzip'] = // comment
TRUE;

would be the result. We do not want that. I doubt this is important enough to keep a secondary buffer just so that the comment can be inserted before the variable. Note that this is not new, HEAD puts the comment between the semicolon and the linebreak too.

> (Sorry) we need "Contains \Drupal\system\Tests\System\SettingsRewriteTest" now.

Sure but note that this is, strictly speaking, off topic because I never write my test from scratch, I copypaste something so this is present somewhere in System already :)

Array test added.

Status: Needs review » Needs work

The last submitted patch, 1921818_4.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
14.02 KB
chx’s picture

FileSize
1.48 KB
chx’s picture

FileSize
4.88 KB
14.63 KB

beejeebus pointed out that a) we are better off only supporting numbers and strings for array indexes (if you use $a[TRUE] in your settings.php, your problem) b) on the other hand, for right hand side we want NULL support.

Anonymous’s picture

this looks good to me, if it comes back green from the bot, i'll RTBC it.

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

ok, discussed this with chx, and he's addressed my issues, RTBC.

chx’s picture

FileSize
2.12 KB
15.06 KB

Reordered $config_directories to get a good example.

alexpott’s picture

@chx: thanks for the new patch in … makes my inner pedant smile

+1 for this patch and it's approach.

webchick’s picture

Assigned: Unassigned » gdd

I think this probably makes sense to have heyrocker eyeball before it goes in. To mine naive eye, that is an awful lot of code we're adding for something we already have tokenized parsing elsewhere for our standard "#" property for differentiating IDs from keys.

gdd’s picture

Status: Reviewed & tested by the community » Needs review

My initial reaction to this is kind of 'ick', if it were just ugliness in the installer it would be one thing but we're introducing ugliness into settings.php with the addition of the token to the values. Then again, I don't have a better answer either although like webchick I am curious why we didn't use the parsing for '#'. That was the first thing I was thinking as I was looking through the patch.

chx’s picture

I can't make heads or tails out of #14 or #15. "We already have tokenized parsing elsewhere for our standard #" ? What...? I know Drupal changed quite a lot but I believe I still know it enough to say with authority that there is no such thing at all, anywhere. Yes, Doctrine uses the PHP tokenizer when looking for class comments to feed the annotation parser but I really fail to see how on earth could we reuse that. I know quite well what that does because it's actually my contribution to Doctrine :) Symfony and Composer have a few tokenizer loops too but none applicable here.

Also, this is not a render array. It'd mighty confusing to use # when it has nothing to do with renders... of course, if you insist I will change the dot but I think there's just misunderstanding here.

gdd’s picture

Status: Needs review » Reviewed & tested by the community

chx and beejeebus pointed out to me in IRC that there is a basic confusion at play here in that we are tokenizing PHP code as opposed to doing our normal array parsing. Chx also pointed out that we are using a dot because it is banned in CMI key names, which makes it perfect for the task.

So I'm on board with this, and am putting it back to RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/install.core.incundefined
@@ -1103,12 +1103,12 @@ function install_settings_form_submit($form, &$form_state) {
-    'value'    => drupal_hash_base64(drupal_random_bytes(55)),
-    'required' => TRUE,
+    '.value'    => drupal_hash_base64(drupal_random_bytes(55)),
+    '.required' => TRUE,

Sorry, but this is just really weird. :\ If we need a way to differentiate between an index and a value of an array, let's make the values classes so we can read properties.

+++ b/core/includes/install.incundefined
@@ -167,57 +167,138 @@ function drupal_get_database_types() {
  * Replaces values in settings.php with values in the submitted array.

It wasn't remotely obvious to me why we needed ~300 lines of code to do what this is doing until chx explained it to me on a whiteboard.

We should expand this documentation to explain this to people coming into this function, so that someone doesn't try to "simplify" it later. :)

+++ b/core/modules/system/lib/Drupal/system/Tests/System/SettingsRewriteTest.phpundefined
@@ -0,0 +1,107 @@
+class SettingsRewriteTest extends UnitTestBase {
...
+  function testDrupalRewriteSettings() {

Needs doxygen.

chx’s picture

Status: Needs work » Needs review
FileSize
8.35 KB
15.29 KB
gdd’s picture

Status: Needs review » Reviewed & tested by the community

Oh this is much better. I'm pleased. If the bot is happy so am I.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Did some more fanangling with the docs w/ chx, and went ahead and committed and pushed to 8.x. Cool!

larowlan’s picture

This will be great for distributions, sometimes we need to collect additional settings ($conf in D7) in the installer setup before the database is available, drupal_rewrite_settings was our only hope in D7, but it required duplicating a lot of existing core installer code. This is excellent, great job all!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

derjochenmeyer’s picture

Status: Closed (fixed) » Needs work

The settings $databases and $config_directories are not rewritten but overwritten at the end of settings.php, i.e. they are not sticking to their comment blocks as proposed in the issue description, or is this by design?

However, the comment blocks in settings.php are helpful and we want the settings to stick to those blocks.

Regarding the dumps of whole arrays mentioned here there is an issue that has an updated patch:
#698236: Settings.php $databases example improvement

chx’s picture

Status: Needs work » Closed (fixed)

That's a followup. We can stick to $foo['bar'] = '' but not $foo = array() yet because the right hand side array is not recognized as overrideable -- it needs one more state to make a distinction between empty and nonempty arrays (and leave nonempty alone).

derjochenmeyer’s picture

I'd like to understand this better but at this point I don't. 698236-47.patch changes the output of $databases. What kind of patch would be necessary to make it possible to stick $databases to its comments block?
#698236: Settings.php $databases example improvement

derjochenmeyer’s picture

derjochenmeyer’s picture

Issue summary: View changes

Updated issue summary.