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

CommentFileSizeAuthor
#7 3425692-nr-bot.txt697 bytesneeds-review-queue-bot

Issue fork drupal-3425692

Command icon 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

joachim created an issue. See original summary.

karimb’s picture

We are going to work on this issue as part of the Novice contribution workshop of Symetris.

karimb’s picture

Status: Active » Needs review

We added documentation in development.services.yml about the Shallow Merge of the files that provide container definitions.

joachim’s picture

Status: Needs review » Needs work

Wow, 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.

karimb’s picture

Status: Needs work » Needs review

No worries. We've learned a lot about container definitions and shallow merge ;)

Here is a shorter version of the documentation.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new697 bytes

The 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.

KarimB changed the visibility of the branch 11.x to hidden.

Lenenba made their first commit to this issue’s fork.

karimb’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Needs work

Believe it needs to be rebased.

poker10’s picture

It seems to me we need to change the scaffold file as well (see the pipeline failure).

mradcliffe’s picture

I 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.

Novice issue reserved for the Mentored Contribution during the DrupalCon Portland 2024 contribution day. After 2024.05.08, this issue returns to being open to all. Thanks
farnoosh’s picture

We 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

bember’s picture

Here'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 description

The correct procedure would be:

-> Correct example from ticket description

Could someone please weigh in? Thanks!

Nvm, totally missed the merge request. Apologies!

msandoval made their first commit to this issue’s fork.

msandoval’s picture

Working on this during DrupalCon 2024.

The pipeline is failing during the Test: "PHPUnit Unit".

Amirez made their first commit to this issue’s fork.

amirez’s picture

rebased 3425692--development-services-docs against 11.x

julieelman’s picture

We 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!

mradcliffe’s picture

Issue tags: +Portland2024

I retroactively added the Portland2024 tag to the issue because contributors were working on this issue on 2024.05.08.

bember’s picture

Status: Needs work » Needs review

Rebased again and all tests pass.

julieelman’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Portland2024

I have reviewed the merge request. The code changes look good. All of the tests are passing.

longwave’s picture

Status: Reviewed & tested by the community » Needs work

Thanks 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.

bember changed the visibility of the branch drupal-3425692--msandoval to hidden.

bember changed the visibility of the branch 3425692--development-services-docs to hidden.

bember changed the visibility of the branch 11.x to active.

bember’s picture

Status: Needs work » Reviewed & tested by the community

@longwave thanks for catching this. It's now fixed and since @julieelman already reviewed 6979, I'm moving this back to RTBC.

joachim’s picture

Status: Reviewed & tested by the community » Needs work

One small typo. Other than that looks good.

msandoval’s picture

I think I can handle this.

msandoval’s picture

Status: Needs work » Reviewed & tested by the community

CI pipeline still passes. Moving back to RTBC.

longwave’s picture

Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs issue summary update

Committed 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!

  • longwave committed 2bbb3d82 on 10.3.x
    Issue #3425692 by msandoval, bember, KarimB, Amirez, Lenenba, joachim,...

  • longwave committed b7542fbc on 10.4.x
    Issue #3425692 by msandoval, bember, KarimB, Amirez, Lenenba, joachim,...

  • longwave committed 00317c88 on 11.0.x
    Issue #3425692 by msandoval, bember, KarimB, Amirez, Lenenba, joachim,...

  • longwave committed fe3721e8 on 11.x
    Issue #3425692 by msandoval, bember, KarimB, Amirez, Lenenba, joachim,...
longwave’s picture

Issue tags: +Portland2024

xjm credited anand48.

xjm credited arparker.

xjm credited chloewebcraft.

xjm’s picture

Adding credit for contribution day contributors per #14. Thanks everyone!

xjm’s picture

Also properly crediting @longwave for the commit, since d.o ate his credit.

Status: Fixed » Closed (fixed)

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