Replace $GLOBALS['conf']['container_service_providers'] in DrupalKernel with Settings

In light of #2160705: settings.php setting names are inconsistent (and undocumented), I think we should cover both of these here:

$conf['container_service_providers'] → array map of "name" to FQCN
$conf['container_yamls'] → array of service yaml filepaths

If I may ask, why do we provide two ways to register custom services?

If we're going to keep that, could we at least shorten + simplify the names?

$settings['service_providers']['MyServiceProvider'] = 'Drupal\Yada\MyServiceProvider';
$settings['service_files'][] = $conf_path . '/services.yml';

Also, why do I need to manually specify the basename of a custom service provider class?

$ php -r "var_dump(basename('Drupal\Yada\MyServiceProvider'));"
string(17) "MyServiceProvider"

Long story short, I'd like to see more or less this:

$settings['service_providers'][] = 'Drupal\Yada\MyServiceProvider';
$settings['service_files'][] = $conf_path . '/services.yml';

So actually the $conf['container_yamls'] is already a setting.

$conf['container_service_providers'] not only is class names but can be full objects. Not sure why. However I think we can convert it to a setting and break the extremely undocumented API.

Fixing installer.

Ah I see...

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -586,11 +586,9 @@ public function discoverServiceProviders() {
-    if (!empty($GLOBALS['conf']['container_service_providers'])) {
-      foreach ($GLOBALS['conf']['container_service_providers'] as $class) {
-        if ((is_string($class) && class_exists($class)) || (is_object($class) && ($class instanceof ServiceProviderInterface || $class instanceof ServiceModifierInterface))) {
-          $this->serviceProviderClasses['site'][] = $class;
-        }
+    foreach (Settings::get('container_service_providers', []) as $class) {
+      if ((is_string($class) && class_exists($class)) || (is_object($class) && ($class instanceof ServiceProviderInterface || $class instanceof ServiceModifierInterface))) {
+        $this->serviceProviderClasses['site'][] = $class;
       }
     }

So this means exising sites with stored $conf will break?

@dawehner how are they setting $conf... if you just to $conf['blah'] in settings .php it is not enough. We already broke that. See Settings::initialise().

Added a CR https://www.drupal.org/node/2568747

I think we should do this because having something around using D7's $GLOBALS['conf'] is super confusing.

@dawehner how are they setting $conf... if you just to $conf['blah'] in settings .php it is not enough. We already broke that. See Settings::initialise().

WOW, I wasn't aware of that

Rerolled

Damn you pifr

Discussed with @catch - we both feel that since you have to do global $conf or access $GLOBALS['conf'] directly providing a BC layer is not worth it.

Rerolled

Discussed with @catch - we both feel that since you have to do global $conf or access $GLOBALS['conf'] directly providing a BC layer is not worth it.

Can you elaborate that? What will happen with sites currently using this feature?

@tstoeckler they need to change. How are your sites using this feature?

Patch in #25 still applies to 8.1.x and 8.2.x.

Setting to 8.2.x and re-running the test.

re-roll patch, many things have changed since this last applied, so fingers crossed!

Small nit: there was still a $conf in the annotation.
This was RTBC, I just re-rolled it to 8.6 so, can still be RTBC in my opinion.

I'm not sure if I agree with myself on BC anymore. I think we should provide BC but deprecate the usage of the global.

1. +++ b/core/globals.api.php
   @@ -42,18 +42,6 @@
   - * To define a site-specific service provider class, use code like this:
   - * @code
   - * $GLOBALS['conf']['container_service_providers']['MyClassName'] = 'Drupal\My\Namespace\MyClassName';
   - * @endcode
   - *
   

   We're telling people how to do it. So I now think we need a BC layer.

2. +++ b/core/includes/install.core.inc
   @@ -137,7 +137,9 @@ function install_drupal($class_loader, $settings = []) {
   +   $settings = Settings::getAll();
   +   unset($settings['container_service_providers']['InstallerServiceProvider']);
   +   new Settings($settings);
   

   Wrong indentation.

I agree, we do need BC.
We use this in some cases for testing so it is definitely good to not break horribly.
I also moved the documentation part to settings.php as one would use it there.

Do we need to exercise the BC path in a test?

@bircher yep and ideally we need to trigger a silenced deprecation notice too.

1. +++ b/core/includes/install.core.inc
   @@ -137,7 +137,9 @@ function install_drupal($class_loader, $settings = []) {
   +    new Settings($settings);
   
   +++ b/core/includes/install.inc
   @@ -610,10 +610,12 @@ function drupal_verify_profile($install_state) {
   +  new Settings($settings);
   
   +++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
   @@ -332,12 +332,10 @@ private function bootKernel() {
   +    new Settings($settings);
   

   oof this pattern is terrible... but yeah that's right :(

2. +++ b/core/includes/install.core.inc
   @@ -403,20 +405,22 @@ function install_begin_request($class_loader, &$install_state) {
   +  // Only allow dumping the container once the hash salt has been created.
   +  $environment = $install_state['base_system_verified'] ? 'prod' : 'install';
   +  // Only allow dumping the container once the hash salt has been created.
   +  $kernel = InstallerKernel::createFromRequest($request, $class_loader, $environment, (bool) Settings::get('hash_salt', FALSE));
   ...
   -  // Only allow dumping the container once the hash salt has been created.
   -  $kernel = InstallerKernel::createFromRequest($request, $class_loader, $environment, (bool) Settings::get('hash_salt', FALSE));
   

   "Only allow dumping the container once the hash salt has been created." is duplicated in the new code.

   Seems moving this has something to do with fixing the installer in #5 and #8 but its not clear to me how it does. Is there something we should document about why the code has to be before and how its fixing the dumping?

   Also we call Settings::get('hash_salt', FALSE)) and then immediately after call Settings::getAll(). Should we just be using the fetched settings or is this connected to all the Setting hacks we have going on somehow?

Needs reroll

Added reroll of patch #37 on Drupal 9.5.x.