Problem/Motivation
development.services.yml allows the developer to override container parameters for debugging. However, \Drupal\Core\DependencyInjection\YamlFileLoader only does a shallow merge of these.
So for example, setting this to enable render cache debug output will crash the site:
parameters:
renderer.config:
debug: true
because the renderer requires the required_cache_contexts.
This means that if you want to override only one key in a parameter, you have to specify the defaults for the others, thus:
renderer.config:
required_cache_contexts: ['languages:language_interface', 'theme', 'user.permissions']
auto_placeholder_conditions:
max-age: 0
contexts: ['session', 'user']
tags: []
debug: true # This is the only value I am overriding.
This should be documented in development.services.yml.
Steps to reproduce
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | 3425692-nr-bot.txt | 697 bytes | needs-review-queue-bot |
Issue fork drupal-3425692
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
karimb commentedWe are going to work on this issue as part of the Novice contribution workshop of Symetris.
Comment #4
karimb commentedWe added documentation in development.services.yml about the Shallow Merge of the files that provide container definitions.
Comment #5
joachim commentedWow, that's a lot of docs! It's great work, but I think it's rather too much for here.
I think we need to say that parameters here are shallow merged (no need to capitalise), and say that if overriding any values, the whole of the array needs to be copied from sites/default/default.services.yml.
I don't think we need code examples, though what you've written for the MR could be moved to https://www.drupal.org/docs/develop/development-tools as a new page about setting up the development.services.yml file.
Comment #6
karimb commentedNo worries. We've learned a lot about container definitions and shallow merge ;)
Here is a shorter version of the documentation.
Comment #7
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #10
karimb commentedComment #11
smustgrave commentedBelieve it needs to be rebased.
Comment #12
poker10 commentedIt seems to me we need to change the scaffold file as well (see the pipeline failure).
Comment #13
mradcliffeI performed Novice Triage on this issue. I am leaving the Novice tag on this issue because we should be able to update the issue summary with the work done so far and what is remaining to be done and complete any additional work and provide a review of the merge request.
Comment #14
farnoosh commentedWe are working on this issue on Drupalcon Portland 2024 contribution day!
@arparker @msandoval @anand48 @farnoosh @amriez @bember @julieelman @chloewebcraft
First step is to rebase the patch against Drupal 11
Comment #15
bember commentedHere's a draft for the helper text:Please note that overriding a setting requires defining default values for all its sub-settings (not only the things you want to override). For example, if you want to enable render cache debug output, the following code will crash the site:-> Bad example from ticket descriptionThe correct procedure would be:-> Correct example from ticket descriptionCould someone please weigh in? Thanks!Nvm, totally missed the merge request. Apologies!
Comment #17
msandoval commentedWorking on this during DrupalCon 2024.
The pipeline is failing during the Test: "PHPUnit Unit".
Comment #19
amirez commentedrebased 3425692--development-services-docs against 11.x
Comment #20
julieelman commentedWe are making progress at Drupalcon Portland 2024 contribution day on this issue with DrupalPod! The team has identified the two files development.services.yml that exist and updated them to match. We are working on testing the build and are investigating a new set of failures in the build with nightwatch and javascript test failures. @howard.lewitt is helping, too!
Comment #21
mradcliffeI retroactively added the Portland2024 tag to the issue because contributors were working on this issue on 2024.05.08.
Comment #22
bember commentedRebased again and all tests pass.
Comment #23
julieelman commentedI have reviewed the merge request. The code changes look good. All of the tests are passing.
Comment #24
longwaveThanks for working on this! There are two branches but no open merge requests; please open a merge request on the branch that has the correct changes, and hide the other branch so as not to confuse reviewers.
Comment #28
bember commented@longwave thanks for catching this. It's now fixed and since @julieelman already reviewed 6979, I'm moving this back to RTBC.
Comment #29
joachim commentedOne small typo. Other than that looks good.
Comment #30
msandoval commentedI think I can handle this.
Comment #31
msandoval commentedCI pipeline still passes. Moving back to RTBC.
Comment #32
longwaveCommitted and pushed fe3721e8a2 to 11.x and 00317c88e0 to 11.0.x and b7542fbcf4 to 10.4.x and 2bbb3d82f3 to 10.3.x. Thanks!
Comment #37
longwaveComment #41
xjmAdding credit for contribution day contributors per #14. Thanks everyone!
Comment #42
xjmAlso properly crediting @longwave for the commit, since d.o ate his credit.