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

Reference: https://www.drupal.org/core/beta-changes
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.
CommentFileSizeAuthor
#6 2382239_6.patch1.39 KBchx
#5 2382239_5.patch1.38 KBchx
installerkernel.patch1.06 KBchx

Comments

benjy’s picture

benjy commented
Status: Needs review » Reviewed & tested by the community

Looks 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!

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

Why it is a bug report and not a task? @chx do you think we should add some tests for this method?

chx’s picture

chx commented
Category: Bug report » Task

Absolutely 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.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+++ 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.

chx’s picture

chx commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new 2382239_5.patch1.38 KB

Dead 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...)

chx’s picture

chx commented
StatusFileSize
new 2382239_6.patch1.39 KB

Left out a backslash between @see and Drupal

chx’s picture

chx commented
Issue summary: View changes

Updated the issue summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

This 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.

  • alexpott committed 6389cda on 8.0.x 
    Issue #2382239 by chx: InstallerKernel is undocumented and hardwires...

  • alexpott committed 4fbd17d on 8.0.x 
    Revert "Issue #2382239 by chx: InstallerKernel is undocumented and...
chx’s picture

chx commented
Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs issue summary update
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Category: Task » Bug report
Status: Reviewed & tested by the community » Fixed

Committed 7631bca and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation for to the issue summary.

  • alexpott committed 7631bca on 8.0.x 
    Issue #2382239 by chx: InstallerKernel is undocumented and hardwires...

Status: Fixed » Closed (fixed)

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