Adjust the order of container yamls to override settings per environment

Do we have existing tests for environment overrides? I would assume we don't have at all.

Seems legit

Actually, I have an idea: let's remove sites/default/services.yml as a special thing and let's just add $settings['container_yamls][] = __DIR__ . '/services.yml'; into default.settings.php and then everyone can order whichever way they want.

Note that after that adding $settings['container_yamls][] = 'development.services.yml'; to the bottom of settings.php will override the sites/default/services.yml just fine. It's just simpler yet more flexible code.

Like this.

Thank you for updating the issue summary.

+++ b/core/tests/Drupal/Tests/Core/DrupalKernel/DiscoverServiceProvidersTest.php
@@ -0,0 +1,47 @@
+      'container_yamls' => array(
+        __DIR__ . '/fixtures/custom.yml'
+      ),
...
+      'site' => array(
+        __DIR__ . '/fixtures/custom.yml',
+      ),

It feels better to have the normal services.yml file included in the test, even it doesn't matter technically.

There's no "normal services.yml" any more, that's sort of the point.

This will break sites upgrading from beta to beta, so tagging. Also we should add the exception to handle that case (or other situations where a settings.php gets copied around).

There's no interdiff as the interdiff is not significantly smaller than the whole patch.

Covering the new exception with a test.

Thank you for adding the helpful exception as well as the test.

Oh installer tests. How do I love thee.

Looks still good. Someone could fix the "s" after "Add" on commit.

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -557,12 +557,8 @@ public function discoverServiceProviders() {
   +      throw new \Exception('"Please add $settings["container_yamls"][] = __DIR__ . "/services.yml"; to settings.php');
   

   Please? The exception message should just say that the container_yamls setting is missing.

2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -1320,4 +1316,20 @@ protected static function setupTrustedHosts(Request $request, $hostPatterns) {
   +   * @param $service_yamls
   +   *   A list of service files.
   +   * @return
   +   *   TRUE if the list was an array, FALSE otherwise.
   

   Space between @param and @return and missing type on @return.

@webflo Thanks for adding the CR. Committed 6d05fc8 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

diff --git a/core/lib/Drupal/Core/DrupalKernel.php b/core/lib/Drupal/Core/DrupalKernel.php
index 370cfba..a3ae42d 100644
--- a/core/lib/Drupal/Core/DrupalKernel.php
+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -531,7 +531,7 @@ public function discoverServiceProviders() {
       }
     }
     if (!$this->addServiceFiles(Settings::get('container_yamls'))) {
-      throw new \Exception('The container_yamls settings is missing.');
+      throw new \Exception('The container_yamls setting is missing from settings.php');
     }
   }

Small fix to the exception message on commit.

Just a word of caution we were looking at something very similar in #2363597: services.yml load order and one issue that needs note is that the ParameterBag used to collect the values from the yaml are overridden not merged! So if you need to change one twig service option you'll need to COPY ALL of the other options (except for the services's defaults) from the default.services.yml

@joelpittet you probably want to edit the CR. Thanks!

I just got bit by #33. Adding the CR update tag and hoping to come back to this later today to investigate.

Considering that you can have any number of services.yml files that override each other, a setting could easily get lost and buried because it wasn't propagated to the next services.yml. Ideally, we would fix this when consuming the yamls (fetch existing options, merge new options, set merged version) as copy/pasting a bunch of settings that you don't want to change is a terrible, horrible, no good, very bad DX.

We might want to do a followup where we switch to using foo.bar: value in these YAMLs , ditch the Symfony YAML loader and load and merge ourselves.

Added warning to CR, hopefully it's not to verbose: https://www.drupal.org/node/2414149

So the foo.bar notation is not necessary but writing a merging YAML loader IMO is. Just a suggestion.

Well it's either we write a merging yaml loader or live with it as-is. Unlikely upstream will change the ParameterBag setter to be merging in Symfony. @mikeker interested in opening up a new issue to resolve this?

@joelpittet: Doubt I'll have time -- I'm heading out for two weeks of vacation! But you never know, I might find some free wifi at the airport... :)

I believe Symfony recommends loading the existing settings, adjusting the few that are changing, and then resaving to avoid the override issue. So, yeah, won't be changed upstream.

If someone could give me some pointers on how all that generated PHP stuff in service_container/service_container_prod/*.php is built, I can look into this when I get back. I haven't poked around in that part of core yet...

Oh, and thanks for updating the CR!

The documentation on turning on twig debugging needs to be updated with this: https://www.drupal.org/node/1906392, https://www.drupal.org/node/1903374.

Followup: #2549591: Installer does not ensure presence of container_yamls setting, breaking sites installing from an empty settings.php