Problem/Motivation

When BootstrapConfigStorageFactory::get is called modules are not yet loaded because that function returns the very storage holding that information. So if contrib wants to provide a config storage then it needs to adds it own namespace to the classloader in settings.php. And while the classloader is in DrupalKernel it is not accessible. This got broken in #2325197: Remove drupal_classloader() when drupal_classloader was taken away.

Proposed resolution

Pass the classloader around. Note: the doxygen and the lack of type hint is copied from DrupalKernel. However, it's only needed once. Also, while at it refactor the function very slightly to work for reinstall as well when settings.php contains a reference to an alternative backend but it is not installed yet.

CommentFileSizeAuthor
#3 2370733_1.patch2 KBchx
BootstrapConfigStorageFactoryClassLoaderShortesPatchName.patch1.87 KBchx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, BootstrapConfigStorageFactoryClassLoaderShortesPatchName.patch, failed testing.

chx’s picture

chx CreditAttribution: chx commented
Issue summary: View changes
Status: Needs work » Needs review
chx’s picture

chx CreditAttribution: chx commented
FileSize
2370733_1.patch2 KB
chx’s picture

chx CreditAttribution: chx commented
Issue summary: View changes
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Needs review » Reviewed & tested by the community

@chx explained on irc that is is too hard to test - and breaks will only impact modules like mongodb anyway, so that will come back to @chx to fix most likely anyway.
Also there is no typehint for $class_loader, because it could be many varied things.
I can't find anything else to change.
Manually tested install and a few pages - all fine.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Reviewed & tested by the community » Needs review

Yes, this makes a lot of sense. Sorry for breaking it.

In general, passing the class loader down to places that need it is good practice and something we should be doing more (I'm looking at you, install.php...) because it is an inherently global object.

The lack of typehinting is #2023325: Add interface for classloader so it can be cleanly swapped.

I only have one small question:

+++ b/core/lib/Drupal/Core/Config/BootstrapConfigStorageFactory.php
@@ -18,16 +18,22 @@ class BootstrapConfigStorageFactory {
+  public static function get($class_loader = NULL) {

Why is $class_loader optional?

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented

RTBC + 1, once that question is answered

chx’s picture

chx CreditAttribution: chx commented

Because the installer and LanguageServiceProvider calls BootstrapConfigStorageFactory::get . By the time LanguageServiceProvider calls this, DrupalKernel have added all modules to the classloader. Same for the installer, the InstallerKernel is built before that call.

chx’s picture

chx CreditAttribution: chx commented

Opsie, didn't want to hide that.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Needs review » Reviewed & tested by the community

OK, yeah that makes sense. Both the installer and the language system never cease to amaze me in their weirdness, but that's not this issue's fault.

Thanks a lot!

chx’s picture

chx CreditAttribution: chx commented
Related issues: +#2044137: No longer possible to configure cache backends (and similar) without enabling a module
alexpott’s picture

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

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase?. Committed 0506d2c and pushed to 8.0.x. Thanks!

I thought about test coverage and whilst it is possible to add by writing an installer test with a custom settings.php it probably is not worth it.

  • alexpott committed 0506d2c on 8.0.x 
    Issue #2370733 by chx: Fixed Contrib can not provide config storage.

Status: Fixed » Closed (fixed)

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