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 incore.services.yml
with exactly the same values, because 99% of sites don't modifysites/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
toservices.yml
and mess with file system permissions. - Have the
services.yml
be an opt-in thing: usefile_exists()
to determine whetherservices.yml
's container parameters should be used when building the container. (Thisfile_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.
Comment | File | Size | Author |
---|---|---|---|
#16 | interdiff.txt | 1.48 KB | Wim Leers |
#16 | 2547447-16.patch | 9.14 KB | Wim Leers |
#12 | interdiff.txt | 872 bytes | Wim Leers |
#12 | 2547447-12.patch | 8.65 KB | Wim Leers |
#9 | interdiff.txt | 3.66 KB | Wim Leers |
Comments
Comment #2
Wim LeersThe 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.)
Comment #3
Wim LeersNote 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".
Comment #4
dawehnerLet's see whether this is all we need.
Comment #5
almaudoh CreditAttribution: almaudoh commentedDoes this patch now mean that I only need to specify things I want to override in
services.yml
?Comment #6
dawehnerFixed the unit test failure.
Comment #7
Wim LeersShouldn't we also update associated documentation? Or perhaps we don't have any?
Comment #8
dawehnerWell yeah, we better do. It is great that the tests are actually passing already!
Comment #9
Wim LeersIndeed!
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
.Comment #12
Wim LeersTurns out we had a long-standing bug in
WebTestBase
. This should come back green.Comment #13
dawehnerWell 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)
Comment #14
alexpott+1 to this issue - less writing of files at install time is a good thing.
This change could be more elegant...
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.
Comment #15
alexpottCreated #2553733: BrowserTestBase should be adding the config schema checker like WebTestBase
Comment #16
Wim LeersDone.
Comment #17
dawehnerPerfect
Comment #18
alexpottSo 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!
Comment #21
aquibrashad CreditAttribution: aquibrashad as a volunteer and commentedComment #22
osopolarThe 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: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:Which is different to the output with no services.yml which is:
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:
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.