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.
Comment | File | Size | Author |
---|---|---|---|
#19 | 1921818_19.patch | 15.29 KB | chx |
#19 | interdiff.txt | 8.35 KB | chx |
#12 | 1921818_11.patch | 15.06 KB | chx |
#12 | interdiff.txt | 2.12 KB | chx |
#9 | 1921818_9.patch | 14.63 KB | chx |
Comments
Comment #1
chx CreditAttribution: chx commentedComment #1.0
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #2
chx CreditAttribution: chx commentedAdded tests.
Comment #3
damiankloip CreditAttribution: damiankloip commentedNice!
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.
This helper could do with a description.
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:
(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?
Comment #4
chx CreditAttribution: chx commentedThe 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:
Suggested:
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.
Comment #6
chx CreditAttribution: chx commentedComment #8
chx CreditAttribution: chx commentedComment #9
chx CreditAttribution: chx commentedbeejeebus 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.
Comment #10
Anonymous (not verified) CreditAttribution: Anonymous commentedthis looks good to me, if it comes back green from the bot, i'll RTBC it.
Comment #11
Anonymous (not verified) CreditAttribution: Anonymous commentedok, discussed this with chx, and he's addressed my issues, RTBC.
Comment #12
chx CreditAttribution: chx commentedReordered $config_directories to get a good example.
Comment #13
alexpott@chx: thanks for the new patch in … makes my inner pedant smile
+1 for this patch and it's approach.
Comment #14
webchickI 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.
Comment #15
gddMy 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.
Comment #16
chx CreditAttribution: chx commentedI 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.
Comment #17
gddchx 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.
Comment #18
webchickSorry, 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.
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. :)
Needs doxygen.
Comment #19
chx CreditAttribution: chx commentedComment #20
gddOh this is much better. I'm pleased. If the bot is happy so am I.
Comment #21
webchickDid some more fanangling with the docs w/ chx, and went ahead and committed and pushed to 8.x. Cool!
Comment #22
larowlanThis 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!
Comment #24
derjochenmeyer CreditAttribution: derjochenmeyer commentedThe 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?
Regarding the dumps of whole arrays mentioned here there is an issue that has an updated patch:
#698236: Settings.php $databases example improvement
Comment #25
chx CreditAttribution: chx commentedThat'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).
Comment #26
derjochenmeyer CreditAttribution: derjochenmeyer commentedI'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
Comment #27
derjochenmeyer CreditAttribution: derjochenmeyer commentedI created a follow-up issue and attached a patch.
#1988138: Modify drupal_rewrite_settings() to allow sticking settings to their comment block
Comment #27.0
derjochenmeyer CreditAttribution: derjochenmeyer commentedUpdated issue summary.