Problem/Motivation

#2543332: Auto-placeholdering for #lazy_builder without bubbling added an extra key-value pair to the renderer.config container parameter. But, because most site owners don't ever upgrade their sites/default/services.yml to stay in sync with updates to sites/default/default.services.yml, they were left with a services.yml without this new key-value pair. And since services.yml continues to override core.services.yml's container parameters (as intended), this effectively means that the new key-value pair is missing. See #2547127-14: [regression] Empty messages container appearing when a Notice occurs for details. See https://www.drupal.org/node/2547351 for the CR that's encouraging people to do the right thing.

This is a specific example, but it points to a larger problem: people are not updating their services.yml file.

Proposed resolution

  • sites/default/services.yml is useless for 99% of sites in its current form: it overrides the defaults in core.services.yml with exactly the same values, because 99% of sites don't modify sites/default/services.yml.
  • But, when the defaults (in sites/default/default.services.yml) are updated, problems occur.
  • So: stop creating sites/default/services.yml by default.
  • It'd also simplify the installation process: no need for the user to copy default.services.yml to services.yml and mess with file system permissions.
  • Have the services.yml be an opt-in thing: use file_exists() to determine whether services.yml's container parameters should be used when building the container. (This file_exists() check would only happen during container rebuilds.)

Other solutions discussed: A) have everything in default.services.yml and hence also services.yml be commented; B) have services.yml be merged instead of overridden (this is supported by Symfony, we never actually configured it).

Remaining tasks

Implement it.

User interface changes

None.

API changes

None.

Data model changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

The above is the conclusion from the D8 EU criticals call we just had, with Alex Pott, catch and dawehner helping to come to this point

(I hope I captured everything correctly.)

Wim Leers’s picture

Note that it's a happy coincidence that we detect this now, during the beta phase. Because this will remain a problem going forward, in future core releases. That's why this is tagged with "D8 upgrade path".

dawehner’s picture

Status: Active » Needs review
FileSize
3.09 KB

Let's see whether this is all we need.

almaudoh’s picture

Does this patch now mean that I only need to specify things I want to override in services.yml?

dawehner’s picture

Fixed the unit test failure.

Wim Leers’s picture

Shouldn't we also update associated documentation? Or perhaps we don't have any?

dawehner’s picture

Well yeah, we better do. It is great that the tests are actually passing already!

Wim Leers’s picture

Indeed!

Updating the docs, by restoring those from before #2251113: Use container parameters instead of settings. Also updated KTB & WTB to no longer copy default.services.yml.

The last submitted patch, 4: 2547447-4.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 9: 2547447-9.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
8.65 KB
872 bytes

Turns out we had a long-standing bug in WebTestBase. This should come back green.

dawehner’s picture

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -711,7 +711,6 @@ protected function prepareSettings() {
-    copy(DRUPAL_ROOT . '/sites/default/default.services.yml', $directory . '/services.yml');

@@ -757,6 +756,10 @@ protected function prepareSettings() {
+    else {
+      // Otherwise, use the default services as a starting point for overrides.
+      copy(DRUPAL_ROOT . '/sites/default/default.services.yml', $directory . '/services.yml');
+    }

Well if we want to we just keep HEAD as it is ... I mean technically its the same kind of code, just a bit more complex to read now (the patch)

alexpott’s picture

+1 to this issue - less writing of files at install time is a good thing.

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -757,6 +756,10 @@ protected function prepareSettings() {
       // Copy the testing-specific service overrides in place.
       copy($settings_services_file, $directory . '/services.yml');
     }
+    else {
+      // Otherwise, use the default services as a starting point for overrides.
+      copy(DRUPAL_ROOT . '/sites/default/default.services.yml', $directory . '/services.yml');
+    }

This change could be more elegant...

    $settings_services_file = DRUPAL_ROOT . '/' . $this->originalSite . '/testing.services.yml';
    if (!file_exists($settings_services_file)) {
      // Otherwise, use the default services as a starting point for overrides.
      $settings_services_file = DRUPAL_ROOT . '/sites/default/default.services.yml';
    }
    // Copy the testing-specific service overrides in place.
    copy($settings_services_file, $directory . '/services.yml');

We also should be making this change in BrowserTestBase... ah I see why you don't have to do that... it is not registering the config schema checker - gonna file an issue for that.

alexpott’s picture

Wim Leers’s picture

Done.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Perfect

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

So this only half solves the problem as we have existing services files with out-dated information. But it is a good step in the right direction. Committed ba2268f and pushed to 8.0.x. Thanks!

  • alexpott committed ba2268f on 8.0.x
    Issue #2547447 by Wim Leers, dawehner: Stop creating services.yml by...

Status: Fixed » Closed (fixed)

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

aquibrashad’s picture

osopolar’s picture

The question in #5 from @almaudoh was not answered. AFAICS It's not necessary to copy the complete default.services.yml to services.yml but it might be necessary to copy/set all options for a parameter and not just the one which should get overridden.

Example:

Set cookie_lifetime: 0 with following services.yml:

parameters:
  session.storage.options:
    cookie_lifetime: 0

After setting above code in services.yml and rebuilding cache the output of drush php:eval "var_export(\Drupal::getContainer()->getParameter('session.storage.options')) is:

array (
  'cookie_lifetime' => 0,
)

Which is different to the output with no services.yml which is:

array (
  'gc_probability' => 1,
  'gc_divisor' => 100,
  'gc_maxlifetime' => 200000,
  'cookie_lifetime' => 2000000,
  'cookie_samesite' => 'Lax',
  'sid_length' => 48,
  'sid_bits_per_character' => 6,
)

Therefore I assume that it might be necessary to define all options below for one parameter, at least if no defaults are defined by the corresponding service:

parameters:
  session.storage.options:
    gc_probability: 1
    gc_divisor: 100
    gc_maxlifetime: 200000
    cookie_lifetime: 0
    cookie_samesite: Lax
    sid_length: 48
    sid_bits_per_character: 6

Is that assumption right?

Anyway, looking at \Drupal\Core\Session\SessionConfiguration::__construct() shows that defaults are defined, so at least in this case the missing options shouldn't be problematic.