Posted by chx on
Problem/Motivation
Good luck changing the bootstrap config storage during installation.
Proposed resolution
Well, I used reflection but I'd rather not. Full disclosure: this new method is very likely to be only needed by the MongoDB project. So far I believe I have managed to avoid these. However, it's on the InstallerKernel and extremely short. If this is not desirable, I can keep the reflection-based reset in my project.
Remaining tasks
Decide whether we are adding this method. If not, I will reroll with just the doxygen change.
User interface changes
Nothing.
API changes
New method on the InstallerKernel . I am sure both people who will ever need this ;) will be quite happy but it doesn't need a change record.
Beta phase evaluation
| Issue category | Bug because it is impossible to swap bootstrap config factory (aka overcaching) |
|---|---|
| Prioritized changes | The main goal of this issue is bug fixes (first one under https://www.drupal.org/core/beta-changes#prioritized) |
| Disruption | Absolutely none. |
Files:
| Comment | File | Size | Author |
|---|---|---|---|
| #6 |  2382239_6.patch | 1.39 KB | chx |
| PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,989 pass(es). [ View ] | |||
Comments
Comment #1
benjy commentedLooks good to me. Nice catch with the docs for initializeContainer, I don't like the idea of overridden methods having different default params, that is just confusing!
Comment #2
jibran commentedWhy it is a bug report and not a task? @chx do you think we should add some tests for this method?
Comment #3
chx commentedAbsolutely not. I would rather not touch the installer tests with a ten feet barge pole especially not for something as trivial as this.
And it's a bug report because it hardwires a thing that needs to be not hardwired but I won't harp on this. The impact of this patch on running Drupals is zero, the software maintenance cost is probably increased by 0.0000001% by adding this method.
Edit: to clarify you'd need to write a database driver that also overrides the bootstrap config storage. MongoDB does that, I verified the patch... but an automated test doing that, I don't think so.
Comment #4
alexpott commented+++ b/core/lib/Drupal/Core/Installer/InstallerKernel.php@@ -19,10 +16,21 @@ class InstallerKernel extends DrupalKernel {
+ /**
+ * Reset the config storage.
+ */
Can we get a bit more documentation why this is necessary so it does not look dead code.
Comment #5
chx commentedDead core? More like a dead coder! :P (It took an awful lot of effort to figure what needs to be reset and then use reflection to get it reset...)
Comment #6
chx commentedLeft out a backslash between @see and Drupal
Comment #7
chx commentedUpdated the issue summary.
Comment #8
alexpott commentedThis issue is a normal task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary.
I think making things possible for contrib without using reflection is a good reason and adding this method is 0 risk.
Comment #11
chx commentedComment #12
alexpott commentedCommitted 7631bca and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation for to the issue summary.